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

From: Coiby Xu <coiby.xu@gmail.com>
To: Takashi Iwai <tiwai@suse.de>
Cc: Jaroslav Kysela <perex@perex.cz>, Takashi Iwai <tiwai@suse.com>,
	"moderated list:SOUND" <alsa-devel@alsa-project.org>,
	open list <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH 01/25] ALSA: core: pcm: remove unnecessary CONFIG_PM_SLEEP
Date: Thu, 29 Oct 2020 22:37:33 +0800	[thread overview]
Message-ID: <20201029143733.d53pu2ri5ykqlff2@Rk> (raw)
In-Reply-To: <s5hpn51wjyw.wl-tiwai@suse.de>

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@gmail.com>
>
>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