| Message ID | 20211207002102.26414-1-paul@crapouillou.net |
|---|---|
| Headers | show |
| Series | Rework pm_ptr() and *_PM_OPS macros | expand |
On Tue, 7 Dec 2021 00:21:00 +0000 Paul Cercueil <paul@crapouillou.net> 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@crapouillou.net> 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@huawei.com> > --- > 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
On Tue, Dec 7, 2021 at 10:22 AM Arnd Bergmann <arnd@arndb.de> wrote: > > On Tue, Dec 7, 2021 at 1:20 AM Paul Cercueil <paul@crapouillou.net> wrote: > > > > This patchset reworks the pm_ptr() macro I introduced a few versions > > ago, so that it is not conditionally defined. > > > > It applies the same treatment to the *_PM_OPS macros. Instead of > > modifying the existing ones, which would mean a 2000+ patch bomb, this > > patchset introduce two new macros to replace the now deprecated > > UNIVERSAL_DEV_PM_OPS() and SIMPLE_DEV_PM_OPS(). > > > > The point of all of this, is to progressively switch from a code model > > where PM callbacks are all protected behind CONFIG_PM guards, to a code > > model where PM callbacks are always seen by the compiler, but discarded > > if not used. > > > > Patch [4/5] and [5/5] are just examples to illustrate the use of the new > > macros. As such they don't really have to be merged at the same time as > > the rest and can be delayed until a subsystem-wide patchset is proposed. > > > > - Patch [4/5] modifies a driver that already used the pm_ptr() macro, > > but had to use the __maybe_unused flag to avoid compiler warnings; > > - Patch [5/5] modifies a driver that used a #ifdef CONFIG_PM guard > > around its suspend/resume functions. > > This is fantastic, I love the new naming and it should provide a great path > towards converting all drivers eventually. I've added the patches to > my randconfig test build box to see if something breaks, but otherwise > I think these are ready to get into linux-next, at least patches 1-3, > so subsystem > maintainers can start queuing up the conversion patches once the > initial set is merged. > > Reviewed-by: Arnd Bergmann <arnd@arndb.de> Patches [0-3/5] applied as 5.17 material. The mmc patches need ACKs, but I can take them too.
On Fri, 17 Dec 2021 at 16:07, Rafael J. Wysocki <rafael@kernel.org> wrote: > > On Tue, Dec 7, 2021 at 10:22 AM Arnd Bergmann <arnd@arndb.de> wrote: > > > > On Tue, Dec 7, 2021 at 1:20 AM Paul Cercueil <paul@crapouillou.net> wrote: > > > > > > This patchset reworks the pm_ptr() macro I introduced a few versions > > > ago, so that it is not conditionally defined. > > > > > > It applies the same treatment to the *_PM_OPS macros. Instead of > > > modifying the existing ones, which would mean a 2000+ patch bomb, this > > > patchset introduce two new macros to replace the now deprecated > > > UNIVERSAL_DEV_PM_OPS() and SIMPLE_DEV_PM_OPS(). > > > > > > The point of all of this, is to progressively switch from a code model > > > where PM callbacks are all protected behind CONFIG_PM guards, to a code > > > model where PM callbacks are always seen by the compiler, but discarded > > > if not used. > > > > > > Patch [4/5] and [5/5] are just examples to illustrate the use of the new > > > macros. As such they don't really have to be merged at the same time as > > > the rest and can be delayed until a subsystem-wide patchset is proposed. > > > > > > - Patch [4/5] modifies a driver that already used the pm_ptr() macro, > > > but had to use the __maybe_unused flag to avoid compiler warnings; > > > - Patch [5/5] modifies a driver that used a #ifdef CONFIG_PM guard > > > around its suspend/resume functions. > > > > This is fantastic, I love the new naming and it should provide a great path > > towards converting all drivers eventually. I've added the patches to > > my randconfig test build box to see if something breaks, but otherwise > > I think these are ready to get into linux-next, at least patches 1-3, > > so subsystem > > maintainers can start queuing up the conversion patches once the > > initial set is merged. > > > > Reviewed-by: Arnd Bergmann <arnd@arndb.de> > > Patches [0-3/5] applied as 5.17 material. > > The mmc patches need ACKs, but I can take them too. Sure, please add my ack for them! Kind regards Uffe
On Fri, Dec 17, 2021 at 6:17 PM Ulf Hansson <ulf.hansson@linaro.org> wrote: > > On Fri, 17 Dec 2021 at 16:07, Rafael J. Wysocki <rafael@kernel.org> wrote: > > > > On Tue, Dec 7, 2021 at 10:22 AM Arnd Bergmann <arnd@arndb.de> wrote: > > > > > > On Tue, Dec 7, 2021 at 1:20 AM Paul Cercueil <paul@crapouillou.net> wrote: > > > > > > > > This patchset reworks the pm_ptr() macro I introduced a few versions > > > > ago, so that it is not conditionally defined. > > > > > > > > It applies the same treatment to the *_PM_OPS macros. Instead of > > > > modifying the existing ones, which would mean a 2000+ patch bomb, this > > > > patchset introduce two new macros to replace the now deprecated > > > > UNIVERSAL_DEV_PM_OPS() and SIMPLE_DEV_PM_OPS(). > > > > > > > > The point of all of this, is to progressively switch from a code model > > > > where PM callbacks are all protected behind CONFIG_PM guards, to a code > > > > model where PM callbacks are always seen by the compiler, but discarded > > > > if not used. > > > > > > > > Patch [4/5] and [5/5] are just examples to illustrate the use of the new > > > > macros. As such they don't really have to be merged at the same time as > > > > the rest and can be delayed until a subsystem-wide patchset is proposed. > > > > > > > > - Patch [4/5] modifies a driver that already used the pm_ptr() macro, > > > > but had to use the __maybe_unused flag to avoid compiler warnings; > > > > - Patch [5/5] modifies a driver that used a #ifdef CONFIG_PM guard > > > > around its suspend/resume functions. > > > > > > This is fantastic, I love the new naming and it should provide a great path > > > towards converting all drivers eventually. I've added the patches to > > > my randconfig test build box to see if something breaks, but otherwise > > > I think these are ready to get into linux-next, at least patches 1-3, > > > so subsystem > > > maintainers can start queuing up the conversion patches once the > > > initial set is merged. > > > > > > Reviewed-by: Arnd Bergmann <arnd@arndb.de> > > > > Patches [0-3/5] applied as 5.17 material. > > > > The mmc patches need ACKs, but I can take them too. > > Sure, please add my ack for them! Both applied as 5.17 material with your ACKs, thanks!