Re: [PATCH v2 1/6] PM: core: Remove DEFINE_UNIVERSAL_DEV_PM_OPS() macro

[Paul Cercueil <paul@crapouillou.net>]

Le ven., janv. 7 2022 at 17:40:57 +0100, Ulf Hansson 
<ulf.hansson@linaro.org> a écrit :
> On Fri, 7 Jan 2022 at 17:37, Paul Cercueil <paul@crapouillou.net> 
> wrote:
>> 
>>  Hi Ulf,
>> 
>>  Le ven., janv. 7 2022 at 17:26:07 +0100, Ulf Hansson
>>  <ulf.hansson@linaro.org> a écrit :
>>  > On Wed, 5 Jan 2022 at 19:29, Paul Cercueil <paul@crapouillou.net>
>>  > wrote:
>>  >>
>>  >>  The deprecated UNIVERSAL_DEV_PM_OPS() macro uses the provided
>>  >> callbacks
>>  >>  for both runtime PM and system sleep, which is very likely to 
>> be a
>>  >>  mistake, as a system sleep can be triggered while a given 
>> device is
>>  >>  already PM-suspended, which would cause the suspend callback to 
>> be
>>  >>  called twice.
>>  >>
>>  >>  The amount of users of UNIVERSAL_DEV_PM_OPS() is also tiny (16
>>  >>  occurences) compared to the number of places where
>>  >>  SET_SYSTEM_SLEEP_PM_OPS() is used with 
>> pm_runtime_force_suspend()
>>  >> and
>>  >>  pm_runtime_force_resume(), which makes me think that none of 
>> these
>>  >> cases
>>  >>  are actually valid.
>>  >>
>>  >>  As this macro is currently unused, remove it before someone 
>> starts
>>  >> to
>>  >>  use it in yet another invalid case.
>>  >
>>  > I assume you refer to DEFINE_UNIVERSAL_DEV_PM_OPS here. Can you
>>  > perhaps make that more clear?
>> 
>>  I can.
>> 
>>  >>
>>  >>  Signed-off-by: Paul Cercueil <paul@crapouillou.net>
>>  >>  Acked-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
>>  >>  ---
>>  >>
>>  >>  Notes:
>>  >>      v2: No change
>>  >>
>>  >>   include/linux/pm.h | 19 ++++++-------------
>>  >>   1 file changed, 6 insertions(+), 13 deletions(-)
>>  >>
>>  >>  diff --git a/include/linux/pm.h b/include/linux/pm.h
>>  >>  index e1e9402180b9..31bbaafb06d2 100644
>>  >>  --- a/include/linux/pm.h
>>  >>  +++ b/include/linux/pm.h
>>  >>  @@ -366,6 +366,12 @@ static const struct dev_pm_ops name = { \
>>  >>          SYSTEM_SLEEP_PM_OPS(suspend_fn, resume_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) \
>>  >>  +}
>>  >>  +
>>  >>   /*
>>  >>    * Use this for defining a set of PM operations to be used in 
>> all
>>  >> situations
>>  >>    * (system suspend, hibernation or runtime PM).
>>  >>  @@ -379,19 +385,6 @@ static const struct dev_pm_ops 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. */
>>  >
>>  > Shouldn't this macro be deprecated any more?
>> 
>>  I can only deprecate it if there is an alternative for it. The
>>  alternative is DEFINE_RUNTIME_DEV_PM_OPS() which is added in patch 
>> 4/6.
> 
> I don't think we need an immediate alternative to leave it 
> deprecated, do we?
> 
> My point is, a user can still combine the macros in a way so that it
> doesn't need to use the UNIVERSAL_DEV_PM_OPS.

Ok. I'll leave it deprecated in that patch then.

Cheers,
-Paul

>>  >>   #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) \
>>  >>  --
> 
> Kind regards
> Uffe