diff mbox series

[5.10.y-cip,14/23] PM: core: Add new *_PM_OPS macros, deprecate old ones

Message ID 20230519082058.109760-15-biju.das.jz@bp.renesas.com (mailing list archive)
State Not Applicable, archived
Delegated to: Pavel Machek
Headers show
Series Add RZ/V2M I2C and WDT support | expand

Commit Message

Biju Das May 19, 2023, 8:20 a.m. UTC 
From: Paul Cercueil <paul@crapouillou.net>

commit 1a3c7bb088266fa2db017be299f91f1c1894c857 upstream.

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.

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@crapouillou.net>
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
---
 include/linux/pm.h | 74 +++++++++++++++++++++++++++++++---------------
 1 file changed, 50 insertions(+), 24 deletions(-)

Comments

Pavel Machek May 19, 2023, 10:13 p.m. UTC | #1 
Hi!

I'll have comments on 19/, but this one is kind of important:

> From: Paul Cercueil <paul@crapouillou.net>
> 
> commit 1a3c7bb088266fa2db017be299f91f1c1894c857 upstream.
> 
> 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()

Changing interfaces / deprecating macros is not good thing to do in
-cip kernel. How sure are we this will not cause build failures or
build warnings in unrelated piece of code?

Thanks and best regards,
								Pavel
Biju Das May 20, 2023, 3 p.m. UTC | #2 
Hi Pavel,

> Subject: Re: [PATCH 5.10.y-cip 14/23] PM: core: Add new *_PM_OPS macros,
> deprecate old ones
> 
> Hi!
> 
> I'll have comments on 19/, but this one is kind of important:
> 
> > From: Paul Cercueil <paul@crapouillou.net>
> >
> > commit 1a3c7bb088266fa2db017be299f91f1c1894c857 upstream.
> >
> > 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()
> 
> Changing interfaces / deprecating macros is not good thing to do in -cip
> kernel. How sure are we this will not cause build failures or build
> warnings in unrelated piece of code?

Please see the commit [1] carefully,

Deprecated is just a comment and it is just single header file change and
There is no existing macros changed.

Please let me know, if I am missing anything.

[1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/include/linux/pm.h?h=v6.4-rc2&id=1a3c7bb088266fa2db017be299f91f1c1894c857 


Cheers,
Biju
Pavel Machek May 23, 2023, 1:19 p.m. UTC | #3 
Hi!

> > > 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()
> > 
> > Changing interfaces / deprecating macros is not good thing to do in -cip
> > kernel. How sure are we this will not cause build failures or build
> > warnings in unrelated piece of code?
> 
> Please see the commit [1] carefully,
> 
> Deprecated is just a comment and it is just single header file change and
> There is no existing macros changed.

I'll need to look at the commit more carefuly.

However previous patch redefines pm_ptr macro, which may have
implications elsewhere. And indeed

commit 5ef11c56b2332613cacd5e7ac17cfb1a073b66ab
    r8169: Avoid misuse of pm_ptr() macro

is example of such effect, and we'd need to pick it up.

I believe it would be less intrusive to make i2c driver work w/o core
changes.

Thanks and best regards,
								Pavel
Biju Das May 23, 2023, 2:05 p.m. UTC | #4 
Hi Pavel,

Thanks for the feedback.

> -----Original Message-----
> From: Pavel Machek <pavel@denx.de>
> Sent: Tuesday, May 23, 2023 2:20 PM
> To: Biju Das <biju.das.jz@bp.renesas.com>
> Cc: Pavel Machek <pavel@denx.de>; cip-dev@lists.cip-project.org;
> Nobuhiro Iwamatsu <nobuhiro1.iwamatsu@toshiba.co.jp>; Chris Paterson
> <Chris.Paterson2@renesas.com>; Fabrizio Castro
> <fabrizio.castro.jz@renesas.com>
> Subject: Re: [PATCH 5.10.y-cip 14/23] PM: core: Add new *_PM_OPS macros,
> deprecate old ones
> 
> Hi!
> 
> > > > 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()
> > >
> > > Changing interfaces / deprecating macros is not good thing to do in
> > > -cip kernel. How sure are we this will not cause build failures or
> > > build warnings in unrelated piece of code?
> >
> > Please see the commit [1] carefully,
> >
> > Deprecated is just a comment and it is just single header file change
> > and There is no existing macros changed.
> 
> I'll need to look at the commit more carefuly.
> 
> However previous patch redefines pm_ptr macro, which may have
> implications elsewhere. And indeed
> 
> commit 5ef11c56b2332613cacd5e7ac17cfb1a073b66ab
>     r8169: Avoid misuse of pm_ptr() macro
> 
> is example of such effect, and we'd need to pick it up.

Oops missed it.

> 
> I believe it would be less intrusive to make i2c driver work w/o core
> changes.

What is your suggestion for adding RZ/V2M driver support 5.10.y-cip?

Modify the original mainline driver by using the available PM macros in 5.10.y-cip ?

Or

Port commit 5ef11c56b2332613cacd5e7ac17cfb1a073b66ab as well??

Cheers,
Biju
Pavel Machek May 24, 2023, 7:43 a.m. UTC | #5 
Hi!

> > I believe it would be less intrusive to make i2c driver work w/o core
> > changes.
> 
> What is your suggestion for adding RZ/V2M driver support 5.10.y-cip?
> 
> Modify the original mainline driver by using the available PM macros in 5.10.y-cip ?

This is the preferred option.

Thanks and best regards,
								Pavel
Biju Das May 24, 2023, 7:46 a.m. UTC | #6 
Hi Pavel,

> Subject: Re: [PATCH 5.10.y-cip 14/23] PM: core: Add new *_PM_OPS macros,
> deprecate old ones
> 
> Hi!
> 
> > > I believe it would be less intrusive to make i2c driver work w/o
> > > core changes.
> >
> > What is your suggestion for adding RZ/V2M driver support 5.10.y-cip?
> >
> > Modify the original mainline driver by using the available PM macros
> in 5.10.y-cip ?
> 
> This is the preferred option.

Thanks for you suggestion. Will modify the driver to adapt 5.10.y-cip.

Cheers,
Biju
diff mbox series

Patch

diff --git a/include/linux/pm.h b/include/linux/pm.h
index 5ac2c9ba5baf..b4974dc83703 100644
--- a/include/linux/pm.h
+++ b/include/linux/pm.h
@@ -301,47 +301,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
@@ -350,9 +362,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) \
 }
 
 /*
@@ -368,6 +380,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) \
@@ -375,6 +400,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