| Message ID | 20170607231026.23383-2-o-takashi@sakamocchi.jp (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, 08 Jun 2017 01:10:19 +0200, Takashi Sakamoto wrote: > > Application of constraints to mask-type parameters for PCM substream is > done in a call of snd_pcm_hw_refine(), while the function includes much > codes and is not enough friendly to readers. > > This commit splits the codes to a separated function so that readers can > get it easily. I leave desicion into compilers to merge the function into > its callee. > > Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp> > --- > sound/core/pcm_native.c | 57 ++++++++++++++++++++++++++++++++----------------- > 1 file changed, 38 insertions(+), 19 deletions(-) > > diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c > index 2bde07a4a87f..b3e8aab3915e 100644 > --- a/sound/core/pcm_native.c > +++ b/sound/core/pcm_native.c > @@ -253,6 +253,40 @@ static bool hw_support_mmap(struct snd_pcm_substream *substream) > return true; > } > > +static int constrain_mask_params(struct snd_pcm_substream *substream, > + struct snd_pcm_hw_params *params) > +{ > + struct snd_pcm_hw_constraints *constrs = > + &substream->runtime->hw_constraints; > + struct snd_mask *m; > + unsigned int k; > + int changed; > + A strange blank line is here. > + struct snd_mask __maybe_unused old_mask; Do we really need __maybe_unused? Drop it as much as possible. IMO, it can be uglier than ifdef, since you don't know why it's unused. With ifdef, at least, you have an idea about the condition. thanks, Takashi > + > + for (k = SNDRV_PCM_HW_PARAM_FIRST_MASK; k <= SNDRV_PCM_HW_PARAM_LAST_MASK; k++) { > + m = hw_param_mask(params, k); > + if (snd_mask_empty(m)) > + return -EINVAL; > + if (!(params->rmask & (1 << k))) > + continue; > + > + if (trace_hw_mask_param_enabled()) > + old_mask = *m; > + > + changed = snd_mask_refine(m, constrs_mask(constrs, k)); > + > + trace_hw_mask_param(substream, k, 0, &old_mask, m); > + > + if (changed) > + params->cmask |= 1 << k; > + if (changed < 0) > + return changed; > + } > + > + return 0; > +} > + > int snd_pcm_hw_refine(struct snd_pcm_substream *substream, > struct snd_pcm_hw_params *params) > { > @@ -265,6 +299,7 @@ int snd_pcm_hw_refine(struct snd_pcm_substream *substream, > unsigned int vstamps[SNDRV_PCM_HW_PARAM_LAST_INTERVAL + 1]; > unsigned int stamp = 2; > int changed, again; > + int err; > > struct snd_mask __maybe_unused old_mask; > struct snd_interval __maybe_unused old_interval; > @@ -278,25 +313,9 @@ int snd_pcm_hw_refine(struct snd_pcm_substream *substream, > params->rate_den = 0; > } > > - for (k = SNDRV_PCM_HW_PARAM_FIRST_MASK; k <= SNDRV_PCM_HW_PARAM_LAST_MASK; k++) { > - m = hw_param_mask(params, k); > - if (snd_mask_empty(m)) > - return -EINVAL; > - if (!(params->rmask & (1 << k))) > - continue; > - > - if (trace_hw_mask_param_enabled()) > - old_mask = *m; > - > - changed = snd_mask_refine(m, constrs_mask(constrs, k)); > - > - trace_hw_mask_param(substream, k, 0, &old_mask, m); > - > - if (changed) > - params->cmask |= 1 << k; > - if (changed < 0) > - return changed; > - } > + err = constrain_mask_params(substream, params); > + if (err < 0) > + return err; > > for (k = SNDRV_PCM_HW_PARAM_FIRST_INTERVAL; k <= SNDRV_PCM_HW_PARAM_LAST_INTERVAL; k++) { > i = hw_param_interval(params, k); > -- > 2.11.0 >
Hi, On Jun 8 2017 16:10, Takashi Iwai wrote: > On Thu, 08 Jun 2017 01:10:19 +0200, > Takashi Sakamoto wrote: >> >> Application of constraints to mask-type parameters for PCM substream is >> done in a call of snd_pcm_hw_refine(), while the function includes much >> codes and is not enough friendly to readers. >> >> This commit splits the codes to a separated function so that readers can >> get it easily. I leave desicion into compilers to merge the function into >> its callee. >> >> Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp> >> --- >> sound/core/pcm_native.c | 57 ++++++++++++++++++++++++++++++++----------------- >> 1 file changed, 38 insertions(+), 19 deletions(-) >> >> diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c >> index 2bde07a4a87f..b3e8aab3915e 100644 >> --- a/sound/core/pcm_native.c >> +++ b/sound/core/pcm_native.c >> ... >> + struct snd_mask __maybe_unused old_mask; > > Do we really need __maybe_unused? Drop it as much as possible. > IMO, it can be uglier than ifdef, since you don't know why it's > unused. With ifdef, at least, you have an idea about the condition. In kernel documentation[1], we can see below suggestion. "20) Conditional Compilation ... If you have a function or variable which may potentially go unused in a particular configuration, and the compiler would warn about its definition going unused, mark the definition as __maybe_unused rather than wrapping it in a preprocessor conditional. (However, if a function or variable *always* goes unused, delete it.)" I'll follow this. Regards Takashi Sakamoto
On 2017年06月08日 16:28, Takashi Sakamoto wrote: > Hi, > > On Jun 8 2017 16:10, Takashi Iwai wrote: >> On Thu, 08 Jun 2017 01:10:19 +0200, >> Takashi Sakamoto wrote: >>> >>> Application of constraints to mask-type parameters for PCM substream is >>> done in a call of snd_pcm_hw_refine(), while the function includes much >>> codes and is not enough friendly to readers. >>> >>> This commit splits the codes to a separated function so that readers can >>> get it easily. I leave desicion into compilers to merge the function >>> into >>> its callee. >>> >>> Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp> >>> --- >>> sound/core/pcm_native.c | 57 >>> ++++++++++++++++++++++++++++++++----------------- >>> 1 file changed, 38 insertions(+), 19 deletions(-) >>> >>> diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c >>> index 2bde07a4a87f..b3e8aab3915e 100644 >>> --- a/sound/core/pcm_native.c >>> +++ b/sound/core/pcm_native.c >>> ... >>> + struct snd_mask __maybe_unused old_mask; >> >> Do we really need __maybe_unused? Drop it as much as possible. >> IMO, it can be uglier than ifdef, since you don't know why it's >> unused. With ifdef, at least, you have an idea about the condition. > > In kernel documentation[1], we can see below suggestion. > > "20) Conditional Compilation > ... > If you have a function or variable which may potentially go unused in a > particular configuration, and the compiler would warn about its > definition going unused, mark the definition as __maybe_unused rather > than wrapping it in a preprocessor conditional. (However, if a function > or variable *always* goes unused, delete it.)" > > I'll follow this. Oops. This is URL for the [1]: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/coding-style.rst#n993 Regards Takashi Sakamoto
On Thu, 08 Jun 2017 09:28:37 +0200, Takashi Sakamoto wrote: > > Hi, > > On Jun 8 2017 16:10, Takashi Iwai wrote: > > On Thu, 08 Jun 2017 01:10:19 +0200, > > Takashi Sakamoto wrote: > >> > >> Application of constraints to mask-type parameters for PCM substream is > >> done in a call of snd_pcm_hw_refine(), while the function includes much > >> codes and is not enough friendly to readers. > >> > >> This commit splits the codes to a separated function so that readers can > >> get it easily. I leave desicion into compilers to merge the function into > >> its callee. > >> > >> Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp> > >> --- > >> sound/core/pcm_native.c | 57 ++++++++++++++++++++++++++++++++----------------- > >> 1 file changed, 38 insertions(+), 19 deletions(-) > >> > >> diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c > >> index 2bde07a4a87f..b3e8aab3915e 100644 > >> --- a/sound/core/pcm_native.c > >> +++ b/sound/core/pcm_native.c > >> ... > >> + struct snd_mask __maybe_unused old_mask; > > > > Do we really need __maybe_unused? Drop it as much as possible. > > IMO, it can be uglier than ifdef, since you don't know why it's > > unused. With ifdef, at least, you have an idea about the condition. > > In kernel documentation[1], we can see below suggestion. > > "20) Conditional Compilation > ... > If you have a function or variable which may potentially go unused in a > particular configuration, and the compiler would warn about its > definition going unused, mark the definition as __maybe_unused rather > than wrapping it in a preprocessor conditional. (However, if a > function or variable *always* goes unused, delete it.)" > > I'll follow this. It doesn't answer my question. Without __maybe_unused in your code, do you get the compile warning? If yes, why? Well, I see the usage already in your patch for the tracepoints. But __maybe_unused is really ugly, and should be avoided as much as possible. The text above doesn't recommend to use it blindly. It's the last resort. thanks, Takashi
Hi, On Jun 8 2017 16:35, Takashi Iwai wrote: > On Thu, 08 Jun 2017 09:28:37 +0200, > Takashi Sakamoto wrote: >> >> Hi, >> >> On Jun 8 2017 16:10, Takashi Iwai wrote: >>> On Thu, 08 Jun 2017 01:10:19 +0200, >>> Takashi Sakamoto wrote: >>>> >>>> Application of constraints to mask-type parameters for PCM substream is >>>> done in a call of snd_pcm_hw_refine(), while the function includes much >>>> codes and is not enough friendly to readers. >>>> >>>> This commit splits the codes to a separated function so that readers can >>>> get it easily. I leave desicion into compilers to merge the function into >>>> its callee. >>>> >>>> Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp> >>>> --- >>>> sound/core/pcm_native.c | 57 ++++++++++++++++++++++++++++++++----------------- >>>> 1 file changed, 38 insertions(+), 19 deletions(-) >>>> >>>> diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c >>>> index 2bde07a4a87f..b3e8aab3915e 100644 >>>> --- a/sound/core/pcm_native.c >>>> +++ b/sound/core/pcm_native.c >>>> ... >>>> + struct snd_mask __maybe_unused old_mask; >>> >>> Do we really need __maybe_unused? Drop it as much as possible. >>> IMO, it can be uglier than ifdef, since you don't know why it's >>> unused. With ifdef, at least, you have an idea about the condition. >> >> In kernel documentation[1], we can see below suggestion. >> >> "20) Conditional Compilation >> ... >> If you have a function or variable which may potentially go unused in a >> particular configuration, and the compiler would warn about its >> definition going unused, mark the definition as __maybe_unused rather >> than wrapping it in a preprocessor conditional. (However, if a >> function or variable *always* goes unused, delete it.)" >> >> I'll follow this. > > It doesn't answer my question. Without __maybe_unused in your code, > do you get the compile warning? If yes, why? Oh, I cannot see any warnings when disabling tracepoints or SND_DEBUG, as you said... I surely have a bias from my experiences for firewire-motu driver... > Well, I see the usage already in your patch for the tracepoints. > But __maybe_unused is really ugly, and should be avoided as much as > possible. > > The text above doesn't recommend to use it blindly. It's the last > resort. OK. We can avoid using __maybe_unused for function local variable in this case. I'll drop the keyword in next v2 patchset. Thank you Takashi Sakamoto
diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c index 2bde07a4a87f..b3e8aab3915e 100644 --- a/sound/core/pcm_native.c +++ b/sound/core/pcm_native.c @@ -253,6 +253,40 @@ static bool hw_support_mmap(struct snd_pcm_substream *substream) return true; } +static int constrain_mask_params(struct snd_pcm_substream *substream, + struct snd_pcm_hw_params *params) +{ + struct snd_pcm_hw_constraints *constrs = + &substream->runtime->hw_constraints; + struct snd_mask *m; + unsigned int k; + int changed; + + struct snd_mask __maybe_unused old_mask; + + for (k = SNDRV_PCM_HW_PARAM_FIRST_MASK; k <= SNDRV_PCM_HW_PARAM_LAST_MASK; k++) { + m = hw_param_mask(params, k); + if (snd_mask_empty(m)) + return -EINVAL; + if (!(params->rmask & (1 << k))) + continue; + + if (trace_hw_mask_param_enabled()) + old_mask = *m; + + changed = snd_mask_refine(m, constrs_mask(constrs, k)); + + trace_hw_mask_param(substream, k, 0, &old_mask, m); + + if (changed) + params->cmask |= 1 << k; + if (changed < 0) + return changed; + } + + return 0; +} + int snd_pcm_hw_refine(struct snd_pcm_substream *substream, struct snd_pcm_hw_params *params) { @@ -265,6 +299,7 @@ int snd_pcm_hw_refine(struct snd_pcm_substream *substream, unsigned int vstamps[SNDRV_PCM_HW_PARAM_LAST_INTERVAL + 1]; unsigned int stamp = 2; int changed, again; + int err; struct snd_mask __maybe_unused old_mask; struct snd_interval __maybe_unused old_interval; @@ -278,25 +313,9 @@ int snd_pcm_hw_refine(struct snd_pcm_substream *substream, params->rate_den = 0; } - for (k = SNDRV_PCM_HW_PARAM_FIRST_MASK; k <= SNDRV_PCM_HW_PARAM_LAST_MASK; k++) { - m = hw_param_mask(params, k); - if (snd_mask_empty(m)) - return -EINVAL; - if (!(params->rmask & (1 << k))) - continue; - - if (trace_hw_mask_param_enabled()) - old_mask = *m; - - changed = snd_mask_refine(m, constrs_mask(constrs, k)); - - trace_hw_mask_param(substream, k, 0, &old_mask, m); - - if (changed) - params->cmask |= 1 << k; - if (changed < 0) - return changed; - } + err = constrain_mask_params(substream, params); + if (err < 0) + return err; for (k = SNDRV_PCM_HW_PARAM_FIRST_INTERVAL; k <= SNDRV_PCM_HW_PARAM_LAST_INTERVAL; k++) { i = hw_param_interval(params, k);
Application of constraints to mask-type parameters for PCM substream is done in a call of snd_pcm_hw_refine(), while the function includes much codes and is not enough friendly to readers. This commit splits the codes to a separated function so that readers can get it easily. I leave desicion into compilers to merge the function into its callee. Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp> --- sound/core/pcm_native.c | 57 ++++++++++++++++++++++++++++++++----------------- 1 file changed, 38 insertions(+), 19 deletions(-)