Re: [PATCH 3/5] PM: core: Add new *_PM_OPS macros, deprecate old ones

[Jonathan Cameron]

On Tue,  7 Dec 2021 00:21:00 +0000
Paul Cercueil <paul@xxxxxxxxxxxxxxx> wrote:

> This commit introduces the following macros:
> SYSTEM_SLEEP_PM_OPS()
> LATE_SYSTEM_SLEEP_PM_OPS()
> NOIRQ_SYSTEM_SLEEP_PM_OPS()
> RUNTIME_PM_OPS()
> 
> These new macros are very similar to their SET_*_PM_OPS() equivalent.
> They however differ in the fact that the callbacks they set will always
> be seen as referenced by the compiler. This means that the callback
> functions don't need to be wrapped with a #ifdef CONFIG_PM guard, or
> tagged with __maybe_unused, to prevent the compiler from complaining
> about unused static symbols. The compiler will then simply evaluate at
> compile time whether or not these symbols are dead code.
> 
> The callbacks that are only useful with CONFIG_PM_SLEEP is enabled, are
> now also wrapped with a new pm_sleep_ptr() macro, which is inspired from
> pm_ptr(). This is needed for drivers that use different callbacks for
> sleep and runtime PM, to handle the case where CONFIG_PM is set and
> CONFIG_PM_SLEEP is not.
> 
> This commit also deprecates the following macros:
> SIMPLE_DEV_PM_OPS()
> UNIVERSAL_DEV_PM_OPS()
> 
> And introduces the following macros:
> DEFINE_SIMPLE_DEV_PM_OPS()
> DEFINE_UNIVERSAL_DEV_PM_OPS()
> 
> These macros are similar to the functions they were created to replace,
> with the following differences:
> - They use the new macros introduced above, and as such always reference
>   the provided callback functions;
> - They are not tagged with __maybe_unused. They are meant to be used
>   with pm_ptr() or pm_sleep_ptr() for DEFINE_UNIVERSAL_DEV_PM_OPS() and
>   DEFINE_SIMPLE_DEV_PM_OPS() respectively.
> - They declare the symbol static, since every driver seems to do that
>   anyway; and if a non-static use-case is needed an indirection pointer
>   could be used.

There are non static usecases e.g. drivers/iio/ad7606.c
where they are shared across a couple of different modules (typically when
we have a core / i2c / spi module split for a driver or similar).
As you say, there are ways of working around that.

So I guess it's a question of what feels more natural + common kernel
way of doing things.

I'll defer to your (+ anyone else who wishes to comment) judgement.

> 
> The point of this change, is to progressively switch from a code model
> where PM callbacks are all protected behind CONFIG_PM guards, to a code
> model where the PM callbacks are always seen by the compiler, but
> discarded if not used.
> 
> Signed-off-by: Paul Cercueil <paul@xxxxxxxxxxxxxxx>

Great work btw. When the holiday season gets boring I'll redo my IIO
set to use this + maybe the rest of IIO...

Reviewed-by: Jonathan Cameron <Jonathan.Cameron@xxxxxxxxxx>


> ---
>  include/linux/pm.h | 74 +++++++++++++++++++++++++++++++---------------
>  1 file changed, 50 insertions(+), 24 deletions(-)
> 
> diff --git a/include/linux/pm.h b/include/linux/pm.h
> index b88ac7dcf2a2..fc9691cb01b4 100644
> --- a/include/linux/pm.h
> +++ b/include/linux/pm.h
> @@ -300,47 +300,59 @@ struct dev_pm_ops {
>  	int (*runtime_idle)(struct device *dev);
>  };
>  
> +#define SYSTEM_SLEEP_PM_OPS(suspend_fn, resume_fn) \
> +	.suspend = pm_sleep_ptr(suspend_fn), \
> +	.resume = pm_sleep_ptr(resume_fn), \
> +	.freeze = pm_sleep_ptr(suspend_fn), \
> +	.thaw = pm_sleep_ptr(resume_fn), \
> +	.poweroff = pm_sleep_ptr(suspend_fn), \
> +	.restore = pm_sleep_ptr(resume_fn),
> +
> +#define LATE_SYSTEM_SLEEP_PM_OPS(suspend_fn, resume_fn) \
> +	.suspend_late = pm_sleep_ptr(suspend_fn), \
> +	.resume_early = pm_sleep_ptr(resume_fn), \
> +	.freeze_late = pm_sleep_ptr(suspend_fn), \
> +	.thaw_early = pm_sleep_ptr(resume_fn), \
> +	.poweroff_late = pm_sleep_ptr(suspend_fn), \
> +	.restore_early = pm_sleep_ptr(resume_fn),
> +
> +#define NOIRQ_SYSTEM_SLEEP_PM_OPS(suspend_fn, resume_fn) \
> +	.suspend_noirq = pm_sleep_ptr(suspend_fn), \
> +	.resume_noirq = pm_sleep_ptr(resume_fn), \
> +	.freeze_noirq = pm_sleep_ptr(suspend_fn), \
> +	.thaw_noirq = pm_sleep_ptr(resume_fn), \
> +	.poweroff_noirq = pm_sleep_ptr(suspend_fn), \
> +	.restore_noirq = pm_sleep_ptr(resume_fn),
> +
> +#define RUNTIME_PM_OPS(suspend_fn, resume_fn, idle_fn) \
> +	.runtime_suspend = suspend_fn, \
> +	.runtime_resume = resume_fn, \
> +	.runtime_idle = idle_fn,
> +
>  #ifdef CONFIG_PM_SLEEP
>  #define SET_SYSTEM_SLEEP_PM_OPS(suspend_fn, resume_fn) \
> -	.suspend = suspend_fn, \
> -	.resume = resume_fn, \
> -	.freeze = suspend_fn, \
> -	.thaw = resume_fn, \
> -	.poweroff = suspend_fn, \
> -	.restore = resume_fn,
> +	SYSTEM_SLEEP_PM_OPS(suspend_fn, resume_fn)
>  #else
>  #define SET_SYSTEM_SLEEP_PM_OPS(suspend_fn, resume_fn)
>  #endif
>  
>  #ifdef CONFIG_PM_SLEEP
>  #define SET_LATE_SYSTEM_SLEEP_PM_OPS(suspend_fn, resume_fn) \
> -	.suspend_late = suspend_fn, \
> -	.resume_early = resume_fn, \
> -	.freeze_late = suspend_fn, \
> -	.thaw_early = resume_fn, \
> -	.poweroff_late = suspend_fn, \
> -	.restore_early = resume_fn,
> +	LATE_SYSTEM_SLEEP_PM_OPS(suspend_fn, resume_fn)
>  #else
>  #define SET_LATE_SYSTEM_SLEEP_PM_OPS(suspend_fn, resume_fn)
>  #endif
>  
>  #ifdef CONFIG_PM_SLEEP
>  #define SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(suspend_fn, resume_fn) \
> -	.suspend_noirq = suspend_fn, \
> -	.resume_noirq = resume_fn, \
> -	.freeze_noirq = suspend_fn, \
> -	.thaw_noirq = resume_fn, \
> -	.poweroff_noirq = suspend_fn, \
> -	.restore_noirq = resume_fn,
> +	NOIRQ_SYSTEM_SLEEP_PM_OPS(suspend_fn, resume_fn)
>  #else
>  #define SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(suspend_fn, resume_fn)
>  #endif
>  
>  #ifdef CONFIG_PM
>  #define SET_RUNTIME_PM_OPS(suspend_fn, resume_fn, idle_fn) \
> -	.runtime_suspend = suspend_fn, \
> -	.runtime_resume = resume_fn, \
> -	.runtime_idle = idle_fn,
> +	RUNTIME_PM_OPS(suspend_fn, resume_fn, idle_fn)
>  #else
>  #define SET_RUNTIME_PM_OPS(suspend_fn, resume_fn, idle_fn)
>  #endif
> @@ -349,9 +361,9 @@ struct dev_pm_ops {
>   * Use this if you want to use the same suspend and resume callbacks for suspend
>   * to RAM and hibernation.
>   */
> -#define SIMPLE_DEV_PM_OPS(name, suspend_fn, resume_fn) \
> -const struct dev_pm_ops __maybe_unused name = { \
> -	SET_SYSTEM_SLEEP_PM_OPS(suspend_fn, resume_fn) \
> +#define DEFINE_SIMPLE_DEV_PM_OPS(name, suspend_fn, resume_fn) \
> +static const struct dev_pm_ops name = { \
> +	SYSTEM_SLEEP_PM_OPS(suspend_fn, resume_fn) \
>  }
>  
>  /*
> @@ -367,6 +379,19 @@ const struct dev_pm_ops __maybe_unused name = { \
>   * .resume_early(), to the same routines as .runtime_suspend() and
>   * .runtime_resume(), respectively (and analogously for hibernation).
>   */
> +#define DEFINE_UNIVERSAL_DEV_PM_OPS(name, suspend_fn, resume_fn, idle_fn) \
> +static const struct dev_pm_ops name = { \
> +	SYSTEM_SLEEP_PM_OPS(suspend_fn, resume_fn) \
> +	RUNTIME_PM_OPS(suspend_fn, resume_fn, idle_fn) \
> +}
> +
> +/* Deprecated. Use DEFINE_SIMPLE_DEV_PM_OPS() instead. */
> +#define SIMPLE_DEV_PM_OPS(name, suspend_fn, resume_fn) \
> +const struct dev_pm_ops __maybe_unused name = { \
> +	SET_SYSTEM_SLEEP_PM_OPS(suspend_fn, resume_fn) \
> +}
> +
> +/* Deprecated. Use DEFINE_UNIVERSAL_DEV_PM_OPS() instead. */
>  #define UNIVERSAL_DEV_PM_OPS(name, suspend_fn, resume_fn, idle_fn) \
>  const struct dev_pm_ops __maybe_unused name = { \
>  	SET_SYSTEM_SLEEP_PM_OPS(suspend_fn, resume_fn) \
> @@ -374,6 +399,7 @@ const struct dev_pm_ops __maybe_unused name = { \
>  }
>  
>  #define pm_ptr(_ptr) PTR_IF(IS_ENABLED(CONFIG_PM), (_ptr))
> +#define pm_sleep_ptr(_ptr) PTR_IF(IS_ENABLED(CONFIG_PM_SLEEP), (_ptr))
>  
>  /*
>   * PM_EVENT_ messages