diff mbox series

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

Message ID 20220105182939.106885-2-paul@crapouillou.net (mailing list archive)
State Not Applicable
Headers show
Series DEV_PM_OPS macros rework v2 | expand

Commit Message

Paul Cercueil Jan. 5, 2022, 6:29 p.m. UTC 
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.

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(-)

Comments

Ulf Hansson Jan. 7, 2022, 4:26 p.m. UTC | #1 
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?

>
> 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?

>  #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) \
> --
> 2.34.1
>

Kind regards
Uffe
Paul Cercueil Jan. 7, 2022, 4:37 p.m. UTC | #2 
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.

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) \
>>  --
>>  2.34.1
>> 
> 
> Kind regards
> Uffe
Ulf Hansson Jan. 7, 2022, 4:40 p.m. UTC | #3 
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.

>
> 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
Paul Cercueil Jan. 7, 2022, 5:39 p.m. UTC | #4 
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
diff mbox series

Patch

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. */
 #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) \