Re: [PATCH 01/25] ALSA: core: pcm: remove unnecessary CONFIG_PM_SLEEP

[Coiby Xu]

On Thu, Oct 29, 2020 at 08:48:55AM +0100, Takashi Iwai wrote:
> On Thu, 29 Oct 2020 08:42:37 +0100,
> Coiby Xu wrote:
> > SET_SYSTEM_SLEEP_PM_OPS has already took good care of CONFIG_PM_CONFIG.
> >
> > Signed-off-by: Coiby Xu <coiby.xu@xxxxxxxxx>
>
> It caused compile warnings.  Was it already addressed in general?
It hasn't been addressed in general. Thank you for the reminding!

> Or we may use __maybe_unused attribute instead, but it's just a matter
> of taste.
I'll add __maybe_unused in v2 since __maybe_unused should be preferred over
CONFIG_PM_SLEEP according to Arnd Bergmann [1],

> > By and large, drivers handle this by using a CONFIG_PM_SLEEP ifdef.
> >
> > Unless you can make an extremely convincing argument why not to do
> > so here, I'd like you to handle it that way instead.
>
> [adding linux-pm to Cc]
>
> The main reason is that everyone gets the #ifdef wrong, I run into
> half a dozen new build regressions with linux-next every week on
> average, the typical problems being:
>
> - testing CONFIG_PM_SLEEP instead of CONFIG_PM, leading to an unused
>   function warning
> - testing CONFIG_PM instead of CONFIG_PM_SLEEP, leading to a build
>   failure
> - calling a function outside of the #ifdef only from inside an
>   otherwise correct #ifdef, again leading to an unused function
>   warning
> - causing a warning inside of the #ifdef but only testing if that
>   is disabled, leading to a problem if the macro is set (this is
>   rare these days for CONFIG_PM as that is normally enabled)
>
> Using __maybe_unused avoids all of the above.

[1] https://lore.kernel.org/patchwork/comment/919944/
> thanks,
>
> Takashi
>
> > ---
> >  sound/core/pcm.c | 2 --
> >  1 file changed, 2 deletions(-)
> >
> > diff --git a/sound/core/pcm.c b/sound/core/pcm.c
> > index be5714f1bb58..5a281ac92958 100644
> > --- a/sound/core/pcm.c
> > +++ b/sound/core/pcm.c
> > @@ -599,7 +599,6 @@ static const struct attribute_group *pcm_dev_attr_groups[];
> >   * PM callbacks: we need to deal only with suspend here, as the resume is
> >   * triggered either from user-space or the driver's resume callback
> >   */
> > -#ifdef CONFIG_PM_SLEEP
> >  static int do_pcm_suspend(struct device *dev)
> >  {
> >  	struct snd_pcm_str *pstr = container_of(dev, struct snd_pcm_str, dev);
> > @@ -608,7 +607,6 @@ static int do_pcm_suspend(struct device *dev)
> >  		snd_pcm_suspend_all(pstr->pcm);
> >  	return 0;
> >  }
> > -#endif
> >
> >  static const struct dev_pm_ops pcm_dev_pm_ops = {
> >  	SET_SYSTEM_SLEEP_PM_OPS(do_pcm_suspend, NULL)
> > --
> > 2.28.0

--
Best regards,
Coiby