| Message ID | 20220104214214.198843-4-paul@crapouillou.net (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | DEV_PM_OPS macros rework | expand |
On Tue, 4 Jan 2022 21:42:09 +0000 Paul Cercueil <paul@crapouillou.net> wrote: > These macros are defined conditionally, according to CONFIG_PM: > - if CONFIG_PM is enabled, these macros resolve to > DEFINE_SIMPLE_DEV_PM_OPS(), and the dev_pm_ops symbol will be > exported. > > - if CONFIG_PM is disabled, these macros will result in a dummy static > dev_pm_ops to be created with the __maybe_unused flag. The dev_pm_ops > will then be discarded by the compiler, along with the provided > callback functions if they are not used anywhere else. > > In the second case, the symbol is not exported, which should be > perfectly fine - users of the symbol should all use the pm_ptr() or > pm_sleep_ptr() macro, so the dev_pm_ops marked as "extern" in the > client's code will never be accessed. > > Signed-off-by: Paul Cercueil <paul@crapouillou.net> > --- > include/linux/pm.h | 33 ++++++++++++++++++++++++++++++--- > 1 file changed, 30 insertions(+), 3 deletions(-) > > diff --git a/include/linux/pm.h b/include/linux/pm.h > index 389e600df233..a1ce29566aea 100644 > --- a/include/linux/pm.h > +++ b/include/linux/pm.h > @@ -8,6 +8,7 @@ > #ifndef _LINUX_PM_H > #define _LINUX_PM_H > > +#include <linux/export.h> > #include <linux/list.h> > #include <linux/workqueue.h> > #include <linux/spinlock.h> > @@ -357,14 +358,40 @@ struct dev_pm_ops { > #define SET_RUNTIME_PM_OPS(suspend_fn, resume_fn, idle_fn) > #endif > > +#define _DEFINE_DEV_PM_OPS(name, \ > + suspend_fn, resume_fn, \ > + runtime_suspend_fn, runtime_resume_fn, idle_fn) \ > +const struct dev_pm_ops name = { \ > + SYSTEM_SLEEP_PM_OPS(suspend_fn, resume_fn) \ > + RUNTIME_PM_OPS(runtime_suspend_fn, runtime_resume_fn, idle_fn) \ > +} > + one blank line probably enough. > + > /* > * Use this if you want to use the same suspend and resume callbacks for suspend > * to RAM and hibernation. > */ > #define DEFINE_SIMPLE_DEV_PM_OPS(name, suspend_fn, resume_fn) \ > -const struct dev_pm_ops name = { \ > - SYSTEM_SLEEP_PM_OPS(suspend_fn, resume_fn) \ > -} > + _DEFINE_DEV_PM_OPS(name, suspend_fn, resume_fn, NULL, NULL, NULL) > + > +#ifdef CONFIG_PM > +#define _EXPORT_DEV_PM_OPS(name, suspend_fn, resume_fn, runtime_suspend_fn, \ > + runtime_resume_fn, idle_fn, sec) \ > + _DEFINE_DEV_PM_OPS(name, suspend_fn, resume_fn, runtime_suspend_fn, \ > + runtime_resume_fn, idle_fn); \ > + _EXPORT_SYMBOL(name, sec) > +#else > +#define _EXPORT_DEV_PM_OPS(name, suspend_fn, resume_fn, runtime_suspend_fn, \ > + runtime_resume_fn, idle_fn, sec) \ > +static __maybe_unused _DEFINE_DEV_PM_OPS(__static_##name, suspend_fn, \ > + resume_fn, runtime_suspend_fn, \ > + runtime_resume_fn, idle_fn) > +#endif > + > +#define EXPORT_SIMPLE_DEV_PM_OPS(name, suspend_fn, resume_fn) \ > + _EXPORT_DEV_PM_OPS(name, suspend_fn, resume_fn, NULL, NULL, NULL, "") > +#define EXPORT_GPL_SIMPLE_DEV_PM_OPS(name, suspend_fn, resume_fn) \ > + _EXPORT_DEV_PM_OPS(name, suspend_fn, resume_fn, NULL, NULL, NULL, "_gpl") So you can get away with these two cases because the SYSTEM_SLEEP_PM_OPS() all have pm_sleep_ptr() wrappers. However, _EXPORT_DEV_PM_OPS() could be used directly and would require __maybe_unused for the RUNTIME_PM_OPS() parameters which isn't ideal. Maybe I'm missing some reason that isn't a problem though as easy to get lost in these macros. :) You could argue that the _ is meant to indicate that macro shouldn't be used directly but I'm not that optimistic. Jonathan > > /* Deprecated. Use DEFINE_SIMPLE_DEV_PM_OPS() instead. */ > #define SIMPLE_DEV_PM_OPS(name, suspend_fn, resume_fn) \
Hi Jonathan, Le mer., janv. 5 2022 at 10:03:32 +0000, Jonathan Cameron <Jonathan.Cameron@Huawei.com> a écrit : > On Tue, 4 Jan 2022 21:42:09 +0000 > Paul Cercueil <paul@crapouillou.net> wrote: > >> These macros are defined conditionally, according to CONFIG_PM: >> - if CONFIG_PM is enabled, these macros resolve to >> DEFINE_SIMPLE_DEV_PM_OPS(), and the dev_pm_ops symbol will be >> exported. >> >> - if CONFIG_PM is disabled, these macros will result in a dummy >> static >> dev_pm_ops to be created with the __maybe_unused flag. The >> dev_pm_ops >> will then be discarded by the compiler, along with the provided >> callback functions if they are not used anywhere else. >> >> In the second case, the symbol is not exported, which should be >> perfectly fine - users of the symbol should all use the pm_ptr() or >> pm_sleep_ptr() macro, so the dev_pm_ops marked as "extern" in the >> client's code will never be accessed. >> >> Signed-off-by: Paul Cercueil <paul@crapouillou.net> >> --- >> include/linux/pm.h | 33 ++++++++++++++++++++++++++++++--- >> 1 file changed, 30 insertions(+), 3 deletions(-) >> >> diff --git a/include/linux/pm.h b/include/linux/pm.h >> index 389e600df233..a1ce29566aea 100644 >> --- a/include/linux/pm.h >> +++ b/include/linux/pm.h >> @@ -8,6 +8,7 @@ >> #ifndef _LINUX_PM_H >> #define _LINUX_PM_H >> >> +#include <linux/export.h> >> #include <linux/list.h> >> #include <linux/workqueue.h> >> #include <linux/spinlock.h> >> @@ -357,14 +358,40 @@ struct dev_pm_ops { >> #define SET_RUNTIME_PM_OPS(suspend_fn, resume_fn, idle_fn) >> #endif >> >> +#define _DEFINE_DEV_PM_OPS(name, \ >> + suspend_fn, resume_fn, \ >> + runtime_suspend_fn, runtime_resume_fn, idle_fn) \ >> +const struct dev_pm_ops name = { \ >> + SYSTEM_SLEEP_PM_OPS(suspend_fn, resume_fn) \ >> + RUNTIME_PM_OPS(runtime_suspend_fn, runtime_resume_fn, idle_fn) \ >> +} >> + > > one blank line probably enough. > >> + >> /* >> * Use this if you want to use the same suspend and resume >> callbacks for suspend >> * to RAM and hibernation. >> */ >> #define DEFINE_SIMPLE_DEV_PM_OPS(name, suspend_fn, resume_fn) \ >> -const struct dev_pm_ops name = { \ >> - SYSTEM_SLEEP_PM_OPS(suspend_fn, resume_fn) \ >> -} >> + _DEFINE_DEV_PM_OPS(name, suspend_fn, resume_fn, NULL, NULL, NULL) >> + >> +#ifdef CONFIG_PM >> +#define _EXPORT_DEV_PM_OPS(name, suspend_fn, resume_fn, >> runtime_suspend_fn, \ >> + runtime_resume_fn, idle_fn, sec) \ >> + _DEFINE_DEV_PM_OPS(name, suspend_fn, resume_fn, >> runtime_suspend_fn, \ >> + runtime_resume_fn, idle_fn); \ >> + _EXPORT_SYMBOL(name, sec) >> +#else >> +#define _EXPORT_DEV_PM_OPS(name, suspend_fn, resume_fn, >> runtime_suspend_fn, \ >> + runtime_resume_fn, idle_fn, sec) \ >> +static __maybe_unused _DEFINE_DEV_PM_OPS(__static_##name, >> suspend_fn, \ >> + resume_fn, runtime_suspend_fn, \ >> + runtime_resume_fn, idle_fn) >> +#endif >> + >> +#define EXPORT_SIMPLE_DEV_PM_OPS(name, suspend_fn, resume_fn) \ >> + _EXPORT_DEV_PM_OPS(name, suspend_fn, resume_fn, NULL, NULL, NULL, >> "") >> +#define EXPORT_GPL_SIMPLE_DEV_PM_OPS(name, suspend_fn, resume_fn) \ >> + _EXPORT_DEV_PM_OPS(name, suspend_fn, resume_fn, NULL, NULL, NULL, >> "_gpl") > > So you can get away with these two cases because the > SYSTEM_SLEEP_PM_OPS() all have > pm_sleep_ptr() wrappers. However, _EXPORT_DEV_PM_OPS() could be used > directly and > would require __maybe_unused for the RUNTIME_PM_OPS() parameters > which isn't ideal. I don't see why. On both cases (CONFIG_PM enabled/disabled) the runtime-PM callbacks are referenced directly, so at no point do they appear as unused; therefore __maybe_unused is not needed. Cheers, -Paul > Maybe I'm missing some reason that isn't a problem though as easy to > get lost in > these macros. :) > > You could argue that the _ is meant to indicate that macro shouldn't > be used directly > but I'm not that optimistic. > > Jonathan > > > >> >> /* Deprecated. Use DEFINE_SIMPLE_DEV_PM_OPS() instead. */ >> #define SIMPLE_DEV_PM_OPS(name, suspend_fn, resume_fn) \ >
On Wed, 5 Jan 2022 10:15:36 +0000 Paul Cercueil <paul@crapouillou.net> wrote: > Hi Jonathan, > > Le mer., janv. 5 2022 at 10:03:32 +0000, Jonathan Cameron > <Jonathan.Cameron@Huawei.com> a écrit : > > On Tue, 4 Jan 2022 21:42:09 +0000 > > Paul Cercueil <paul@crapouillou.net> wrote: > > > >> These macros are defined conditionally, according to CONFIG_PM: > >> - if CONFIG_PM is enabled, these macros resolve to > >> DEFINE_SIMPLE_DEV_PM_OPS(), and the dev_pm_ops symbol will be > >> exported. > >> > >> - if CONFIG_PM is disabled, these macros will result in a dummy > >> static > >> dev_pm_ops to be created with the __maybe_unused flag. The > >> dev_pm_ops > >> will then be discarded by the compiler, along with the provided > >> callback functions if they are not used anywhere else. > >> > >> In the second case, the symbol is not exported, which should be > >> perfectly fine - users of the symbol should all use the pm_ptr() or > >> pm_sleep_ptr() macro, so the dev_pm_ops marked as "extern" in the > >> client's code will never be accessed. > >> > >> Signed-off-by: Paul Cercueil <paul@crapouillou.net> > >> --- > >> include/linux/pm.h | 33 ++++++++++++++++++++++++++++++--- > >> 1 file changed, 30 insertions(+), 3 deletions(-) > >> > >> diff --git a/include/linux/pm.h b/include/linux/pm.h > >> index 389e600df233..a1ce29566aea 100644 > >> --- a/include/linux/pm.h > >> +++ b/include/linux/pm.h > >> @@ -8,6 +8,7 @@ > >> #ifndef _LINUX_PM_H > >> #define _LINUX_PM_H > >> > >> +#include <linux/export.h> > >> #include <linux/list.h> > >> #include <linux/workqueue.h> > >> #include <linux/spinlock.h> > >> @@ -357,14 +358,40 @@ struct dev_pm_ops { > >> #define SET_RUNTIME_PM_OPS(suspend_fn, resume_fn, idle_fn) > >> #endif > >> > >> +#define _DEFINE_DEV_PM_OPS(name, \ > >> + suspend_fn, resume_fn, \ > >> + runtime_suspend_fn, runtime_resume_fn, idle_fn) \ > >> +const struct dev_pm_ops name = { \ > >> + SYSTEM_SLEEP_PM_OPS(suspend_fn, resume_fn) \ > >> + RUNTIME_PM_OPS(runtime_suspend_fn, runtime_resume_fn, idle_fn) \ > >> +} > >> + > > > > one blank line probably enough. > > > >> + > >> /* > >> * Use this if you want to use the same suspend and resume > >> callbacks for suspend > >> * to RAM and hibernation. > >> */ > >> #define DEFINE_SIMPLE_DEV_PM_OPS(name, suspend_fn, resume_fn) \ > >> -const struct dev_pm_ops name = { \ > >> - SYSTEM_SLEEP_PM_OPS(suspend_fn, resume_fn) \ > >> -} > >> + _DEFINE_DEV_PM_OPS(name, suspend_fn, resume_fn, NULL, NULL, NULL) > >> + > >> +#ifdef CONFIG_PM > >> +#define _EXPORT_DEV_PM_OPS(name, suspend_fn, resume_fn, > >> runtime_suspend_fn, \ > >> + runtime_resume_fn, idle_fn, sec) \ > >> + _DEFINE_DEV_PM_OPS(name, suspend_fn, resume_fn, > >> runtime_suspend_fn, \ > >> + runtime_resume_fn, idle_fn); \ > >> + _EXPORT_SYMBOL(name, sec) > >> +#else > >> +#define _EXPORT_DEV_PM_OPS(name, suspend_fn, resume_fn, > >> runtime_suspend_fn, \ > >> + runtime_resume_fn, idle_fn, sec) \ > >> +static __maybe_unused _DEFINE_DEV_PM_OPS(__static_##name, > >> suspend_fn, \ > >> + resume_fn, runtime_suspend_fn, \ > >> + runtime_resume_fn, idle_fn) > >> +#endif > >> + > >> +#define EXPORT_SIMPLE_DEV_PM_OPS(name, suspend_fn, resume_fn) \ > >> + _EXPORT_DEV_PM_OPS(name, suspend_fn, resume_fn, NULL, NULL, NULL, > >> "") > >> +#define EXPORT_GPL_SIMPLE_DEV_PM_OPS(name, suspend_fn, resume_fn) \ > >> + _EXPORT_DEV_PM_OPS(name, suspend_fn, resume_fn, NULL, NULL, NULL, > >> "_gpl") > > > > So you can get away with these two cases because the > > SYSTEM_SLEEP_PM_OPS() all have > > pm_sleep_ptr() wrappers. However, _EXPORT_DEV_PM_OPS() could be used > > directly and > > would require __maybe_unused for the RUNTIME_PM_OPS() parameters > > which isn't ideal. > > I don't see why. On both cases (CONFIG_PM enabled/disabled) the > runtime-PM callbacks are referenced directly, so at no point do they > appear as unused; therefore __maybe_unused is not needed. Ah. I'd miss followed things through. Indeed the 'magic' __static_xxx_pm_ops structure maintains a reference that the compiler can then remove. On the plus side, turned out I'd not done a full set of tests with my own patch set and found one bug in that :) Acked-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> > > Cheers, > -Paul > > > Maybe I'm missing some reason that isn't a problem though as easy to > > get lost in > > these macros. :) > > > > You could argue that the _ is meant to indicate that macro shouldn't > > be used directly > > but I'm not that optimistic. > > > > Jonathan > > > > > > > >> > >> /* Deprecated. Use DEFINE_SIMPLE_DEV_PM_OPS() instead. */ > >> #define SIMPLE_DEV_PM_OPS(name, suspend_fn, resume_fn) \ > > > >
diff --git a/include/linux/pm.h b/include/linux/pm.h index 389e600df233..a1ce29566aea 100644 --- a/include/linux/pm.h +++ b/include/linux/pm.h @@ -8,6 +8,7 @@ #ifndef _LINUX_PM_H #define _LINUX_PM_H +#include <linux/export.h> #include <linux/list.h> #include <linux/workqueue.h> #include <linux/spinlock.h> @@ -357,14 +358,40 @@ struct dev_pm_ops { #define SET_RUNTIME_PM_OPS(suspend_fn, resume_fn, idle_fn) #endif +#define _DEFINE_DEV_PM_OPS(name, \ + suspend_fn, resume_fn, \ + runtime_suspend_fn, runtime_resume_fn, idle_fn) \ +const struct dev_pm_ops name = { \ + SYSTEM_SLEEP_PM_OPS(suspend_fn, resume_fn) \ + RUNTIME_PM_OPS(runtime_suspend_fn, runtime_resume_fn, idle_fn) \ +} + + /* * Use this if you want to use the same suspend and resume callbacks for suspend * to RAM and hibernation. */ #define DEFINE_SIMPLE_DEV_PM_OPS(name, suspend_fn, resume_fn) \ -const struct dev_pm_ops name = { \ - SYSTEM_SLEEP_PM_OPS(suspend_fn, resume_fn) \ -} + _DEFINE_DEV_PM_OPS(name, suspend_fn, resume_fn, NULL, NULL, NULL) + +#ifdef CONFIG_PM +#define _EXPORT_DEV_PM_OPS(name, suspend_fn, resume_fn, runtime_suspend_fn, \ + runtime_resume_fn, idle_fn, sec) \ + _DEFINE_DEV_PM_OPS(name, suspend_fn, resume_fn, runtime_suspend_fn, \ + runtime_resume_fn, idle_fn); \ + _EXPORT_SYMBOL(name, sec) +#else +#define _EXPORT_DEV_PM_OPS(name, suspend_fn, resume_fn, runtime_suspend_fn, \ + runtime_resume_fn, idle_fn, sec) \ +static __maybe_unused _DEFINE_DEV_PM_OPS(__static_##name, suspend_fn, \ + resume_fn, runtime_suspend_fn, \ + runtime_resume_fn, idle_fn) +#endif + +#define EXPORT_SIMPLE_DEV_PM_OPS(name, suspend_fn, resume_fn) \ + _EXPORT_DEV_PM_OPS(name, suspend_fn, resume_fn, NULL, NULL, NULL, "") +#define EXPORT_GPL_SIMPLE_DEV_PM_OPS(name, suspend_fn, resume_fn) \ + _EXPORT_DEV_PM_OPS(name, suspend_fn, resume_fn, NULL, NULL, NULL, "_gpl") /* Deprecated. Use DEFINE_SIMPLE_DEV_PM_OPS() instead. */ #define SIMPLE_DEV_PM_OPS(name, suspend_fn, resume_fn) \
These macros are defined conditionally, according to CONFIG_PM: - if CONFIG_PM is enabled, these macros resolve to DEFINE_SIMPLE_DEV_PM_OPS(), and the dev_pm_ops symbol will be exported. - if CONFIG_PM is disabled, these macros will result in a dummy static dev_pm_ops to be created with the __maybe_unused flag. The dev_pm_ops will then be discarded by the compiler, along with the provided callback functions if they are not used anywhere else. In the second case, the symbol is not exported, which should be perfectly fine - users of the symbol should all use the pm_ptr() or pm_sleep_ptr() macro, so the dev_pm_ops marked as "extern" in the client's code will never be accessed. Signed-off-by: Paul Cercueil <paul@crapouillou.net> --- include/linux/pm.h | 33 ++++++++++++++++++++++++++++++--- 1 file changed, 30 insertions(+), 3 deletions(-)