| Message ID | 20230920084121.14131-1-raag.jadav@intel.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | [v3] PM: Fix symbol export for _SIMPLE_ variants of _PM_OPS() | expand |
Hi, Le mercredi 20 septembre 2023 à 14:11 +0530, Raag Jadav a écrit : > Currently EXPORT_*_SIMPLE_DEV_PM_OPS() use EXPORT_*_DEV_PM_OPS() set > of macros to export dev_pm_ops symbol, which export the symbol in > case > CONFIG_PM=y but don't take CONFIG_PM_SLEEP into consideration. > > Since _SIMPLE_ variants of _PM_OPS() do not include runtime PM > handles > and are only used in case CONFIG_PM_SLEEP=y, we should not be > exporting > their dev_pm_ops symbol in case CONFIG_PM_SLEEP=n. > > This can be fixed by having two distinct set of export macros for > both > _RUNTIME_ and _SIMPLE_ variants of _PM_OPS(), such that the export of > dev_pm_ops symbol used in each variant depends on CONFIG_PM and > CONFIG_PM_SLEEP respectively. > > Introduce _DEV_SLEEP_PM_OPS() set of export macros for _SIMPLE_ > variants > of _PM_OPS(), which export dev_pm_ops symbol only in case > CONFIG_PM_SLEEP=y > and discard it otherwise. > > Fixes: 34e1ed189fab ("PM: Improve EXPORT_*_DEV_PM_OPS macros") > Signed-off-by: Raag Jadav <raag.jadav@intel.com> > --- > PS: This is a standalone fix and works without updating any drivers. I had to double-check that, to make sure that none of the drivers using these macros also use pm_ptr() instead of pm_sleep_ptr() to access the exported dev_pm_ops. I did not check extensively but everything seems to use pm_sleep_ptr(), so it looks safe enough. > > Changes since v2: > - Drop redundant patches > > Changes since v1: > - Update drivers to new set of macros > > include/linux/pm.h | 43 +++++++++++++++++++++++++++++-------------- > 1 file changed, 29 insertions(+), 14 deletions(-) > > diff --git a/include/linux/pm.h b/include/linux/pm.h > index 1400c37b29c7..99a8146fa479 100644 > --- a/include/linux/pm.h > +++ b/include/linux/pm.h > @@ -374,24 +374,39 @@ const struct dev_pm_ops name = { \ > RUNTIME_PM_OPS(runtime_suspend_fn, runtime_resume_fn, > idle_fn) \ > } > > -#ifdef CONFIG_PM > -#define _EXPORT_DEV_PM_OPS(name, license, > ns) \ > +#define _EXPORT_PM_OPS(name, license, > ns) \ > const struct dev_pm_ops > name; \ > __EXPORT_SYMBOL(name, license, > ns); \ > const struct dev_pm_ops name > -#define EXPORT_PM_FN_GPL(name) EXPORT_SYMBOL_GPL(name) > -#define EXPORT_PM_FN_NS_GPL(name, ns) EXPORT_SYMBOL_NS_GPL(name, > ns) > -#else > -#define _EXPORT_DEV_PM_OPS(name, license, > ns) \ > + > +#define _PM_OPS(name, license, This macro creates a dev_pm_ops that's meant to be garbage-collected by the compiler; so maybe name it _USELESS_PM_OPS() or something like that. (I could have said that in your v2 patch, sorry) Cheers, -Paul > ns) \ > static __maybe_unused const struct dev_pm_ops __static_##name > + > +#ifdef CONFIG_PM > +#define _EXPORT_DEV_PM_OPS(name, license, > ns) _EXPORT_PM_OPS(name, license, ns) > +#define > EXPORT_PM_FN_GPL(name) EXPORT_SYMBOL_GPL(name > ) > +#define EXPORT_PM_FN_NS_GPL(name, > ns) EXPORT_SYMBOL_NS_GPL(name, ns) > +#else > +#define _EXPORT_DEV_PM_OPS(name, license, ns) _PM_OPS(name, > license, ns) > #define EXPORT_PM_FN_GPL(name) > #define EXPORT_PM_FN_NS_GPL(name, ns) > #endif > > -#define EXPORT_DEV_PM_OPS(name) _EXPORT_DEV_PM_OPS(name, "", "") > -#define EXPORT_GPL_DEV_PM_OPS(name) _EXPORT_DEV_PM_OPS(name, "GPL", > "") > -#define EXPORT_NS_DEV_PM_OPS(name, ns) _EXPORT_DEV_PM_OPS(name, "", > #ns) > -#define EXPORT_NS_GPL_DEV_PM_OPS(name, ns) _EXPORT_DEV_PM_OPS(name, > "GPL", #ns) > +#ifdef CONFIG_PM_SLEEP > +#define _EXPORT_DEV_SLEEP_PM_OPS(name, license, > ns) _EXPORT_PM_OPS(name, license, ns) > +#else > +#define _EXPORT_DEV_SLEEP_PM_OPS(name, license, ns) _PM_OPS(name, > license, ns) > +#endif > + > +#define > EXPORT_DEV_PM_OPS(name) _EXPORT_DEV_PM > _OPS(name, "", "") > +#define > EXPORT_GPL_DEV_PM_OPS(name) _EXPORT_DEV_PM_OPS(nam > e, "GPL", "") > +#define EXPORT_NS_DEV_PM_OPS(name, > ns) _EXPORT_DEV_PM_OPS(name, "", #ns) > +#define EXPORT_NS_GPL_DEV_PM_OPS(name, > ns) _EXPORT_DEV_PM_OPS(name, "GPL", #ns) > + > +#define > EXPORT_DEV_SLEEP_PM_OPS(name) _EXPORT_DEV_SLEEP_PM_O > PS(name, "", "") > +#define > EXPORT_GPL_DEV_SLEEP_PM_OPS(name) _EXPORT_DEV_SLEEP_PM_O > PS(name, "GPL", "") > +#define EXPORT_NS_DEV_SLEEP_PM_OPS(name, > ns) _EXPORT_DEV_SLEEP_PM_OPS(name, "", #ns) > +#define EXPORT_NS_GPL_DEV_SLEEP_PM_OPS(name, > ns) _EXPORT_DEV_SLEEP_PM_OPS(name, "GPL", #ns) > > /* > * Use this if you want to use the same suspend and resume callbacks > for suspend > @@ -404,19 +419,19 @@ const struct dev_pm_ops name = { \ > _DEFINE_DEV_PM_OPS(name, suspend_fn, resume_fn, NULL, NULL, > NULL) > > #define EXPORT_SIMPLE_DEV_PM_OPS(name, suspend_fn, resume_fn) \ > - EXPORT_DEV_PM_OPS(name) = { \ > + EXPORT_DEV_SLEEP_PM_OPS(name) = { \ > SYSTEM_SLEEP_PM_OPS(suspend_fn, resume_fn) \ > } > #define EXPORT_GPL_SIMPLE_DEV_PM_OPS(name, suspend_fn, resume_fn) \ > - EXPORT_GPL_DEV_PM_OPS(name) = { \ > + EXPORT_GPL_DEV_SLEEP_PM_OPS(name) = { \ > SYSTEM_SLEEP_PM_OPS(suspend_fn, resume_fn) \ > } > #define EXPORT_NS_SIMPLE_DEV_PM_OPS(name, suspend_fn, resume_fn, > ns) \ > - EXPORT_NS_DEV_PM_OPS(name, ns) = { \ > + EXPORT_NS_DEV_SLEEP_PM_OPS(name, ns) = { \ > SYSTEM_SLEEP_PM_OPS(suspend_fn, resume_fn) \ > } > #define EXPORT_NS_GPL_SIMPLE_DEV_PM_OPS(name, suspend_fn, resume_fn, > ns) \ > - EXPORT_NS_GPL_DEV_PM_OPS(name, ns) = { \ > + EXPORT_NS_GPL_DEV_SLEEP_PM_OPS(name, ns) = { \ > SYSTEM_SLEEP_PM_OPS(suspend_fn, resume_fn) \ > } >
On Wed, Sep 20, 2023 at 10:53:23AM +0200, Paul Cercueil wrote: > Le mercredi 20 septembre 2023 à 14:11 +0530, Raag Jadav a écrit : > > Currently EXPORT_*_SIMPLE_DEV_PM_OPS() use EXPORT_*_DEV_PM_OPS() set > > of macros to export dev_pm_ops symbol, which export the symbol in > > case > > CONFIG_PM=y but don't take CONFIG_PM_SLEEP into consideration. > > > > Since _SIMPLE_ variants of _PM_OPS() do not include runtime PM > > handles > > and are only used in case CONFIG_PM_SLEEP=y, we should not be > > exporting > > their dev_pm_ops symbol in case CONFIG_PM_SLEEP=n. > > > > This can be fixed by having two distinct set of export macros for > > both > > _RUNTIME_ and _SIMPLE_ variants of _PM_OPS(), such that the export of > > dev_pm_ops symbol used in each variant depends on CONFIG_PM and > > CONFIG_PM_SLEEP respectively. > > > > Introduce _DEV_SLEEP_PM_OPS() set of export macros for _SIMPLE_ > > variants > > of _PM_OPS(), which export dev_pm_ops symbol only in case > > CONFIG_PM_SLEEP=y > > and discard it otherwise. > > > > Fixes: 34e1ed189fab ("PM: Improve EXPORT_*_DEV_PM_OPS macros") > > Signed-off-by: Raag Jadav <raag.jadav@intel.com> > > --- > > PS: This is a standalone fix and works without updating any drivers. > > I had to double-check that, to make sure that none of the drivers using > these macros also use pm_ptr() instead of pm_sleep_ptr() to access the > exported dev_pm_ops. > > I did not check extensively but everything seems to use pm_sleep_ptr(), > so it looks safe enough. I have tested it against -rc2 without any problems. > > Changes since v2: > > - Drop redundant patches > > > > Changes since v1: > > - Update drivers to new set of macros > > > > include/linux/pm.h | 43 +++++++++++++++++++++++++++++-------------- > > 1 file changed, 29 insertions(+), 14 deletions(-) > > > > diff --git a/include/linux/pm.h b/include/linux/pm.h > > index 1400c37b29c7..99a8146fa479 100644 > > --- a/include/linux/pm.h > > +++ b/include/linux/pm.h > > @@ -374,24 +374,39 @@ const struct dev_pm_ops name = { \ > > RUNTIME_PM_OPS(runtime_suspend_fn, runtime_resume_fn, > > idle_fn) \ > > } > > > > -#ifdef CONFIG_PM > > -#define _EXPORT_DEV_PM_OPS(name, license, > > ns) \ > > +#define _EXPORT_PM_OPS(name, license, > > ns) \ > > const struct dev_pm_ops > > name; \ > > __EXPORT_SYMBOL(name, license, > > ns); \ > > const struct dev_pm_ops name > > -#define EXPORT_PM_FN_GPL(name) EXPORT_SYMBOL_GPL(name) > > -#define EXPORT_PM_FN_NS_GPL(name, ns) EXPORT_SYMBOL_NS_GPL(name, > > ns) > > -#else > > -#define _EXPORT_DEV_PM_OPS(name, license, > > ns) \ > > + > > +#define _PM_OPS(name, license, > > This macro creates a dev_pm_ops that's meant to be garbage-collected by > the compiler; so maybe name it _USELESS_PM_OPS() or something like > that. _USELESS_PM_OPS() sounds a bit heavy handed ;) Gives the impression that the macro itelf is not used anywhere in code. Something like _DISCARD_PM_OPS() makes more sense. Raag
Le mercredi 20 septembre 2023 à 12:33 +0300, Raag Jadav a écrit : > On Wed, Sep 20, 2023 at 10:53:23AM +0200, Paul Cercueil wrote: > > Le mercredi 20 septembre 2023 à 14:11 +0530, Raag Jadav a écrit : > > > Currently EXPORT_*_SIMPLE_DEV_PM_OPS() use EXPORT_*_DEV_PM_OPS() > > > set > > > of macros to export dev_pm_ops symbol, which export the symbol in > > > case > > > CONFIG_PM=y but don't take CONFIG_PM_SLEEP into consideration. > > > > > > Since _SIMPLE_ variants of _PM_OPS() do not include runtime PM > > > handles > > > and are only used in case CONFIG_PM_SLEEP=y, we should not be > > > exporting > > > their dev_pm_ops symbol in case CONFIG_PM_SLEEP=n. > > > > > > This can be fixed by having two distinct set of export macros for > > > both > > > _RUNTIME_ and _SIMPLE_ variants of _PM_OPS(), such that the > > > export of > > > dev_pm_ops symbol used in each variant depends on CONFIG_PM and > > > CONFIG_PM_SLEEP respectively. > > > > > > Introduce _DEV_SLEEP_PM_OPS() set of export macros for _SIMPLE_ > > > variants > > > of _PM_OPS(), which export dev_pm_ops symbol only in case > > > CONFIG_PM_SLEEP=y > > > and discard it otherwise. > > > > > > Fixes: 34e1ed189fab ("PM: Improve EXPORT_*_DEV_PM_OPS macros") > > > Signed-off-by: Raag Jadav <raag.jadav@intel.com> > > > --- > > > PS: This is a standalone fix and works without updating any > > > drivers. > > > > I had to double-check that, to make sure that none of the drivers > > using > > these macros also use pm_ptr() instead of pm_sleep_ptr() to access > > the > > exported dev_pm_ops. > > > > I did not check extensively but everything seems to use > > pm_sleep_ptr(), > > so it looks safe enough. > > I have tested it against -rc2 without any problems. You'd need to test an "allyesconfig" with CONFIG_PM=y, and CONFIG_PM_SLEEP disabled. Is that what you tested? > > > > Changes since v2: > > > - Drop redundant patches > > > > > > Changes since v1: > > > - Update drivers to new set of macros > > > > > > include/linux/pm.h | 43 +++++++++++++++++++++++++++++----------- > > > --- > > > 1 file changed, 29 insertions(+), 14 deletions(-) > > > > > > diff --git a/include/linux/pm.h b/include/linux/pm.h > > > index 1400c37b29c7..99a8146fa479 100644 > > > --- a/include/linux/pm.h > > > +++ b/include/linux/pm.h > > > @@ -374,24 +374,39 @@ const struct dev_pm_ops name = { \ > > > RUNTIME_PM_OPS(runtime_suspend_fn, runtime_resume_fn, > > > idle_fn) \ > > > } > > > > > > -#ifdef CONFIG_PM > > > -#define _EXPORT_DEV_PM_OPS(name, license, > > > ns) \ > > > +#define _EXPORT_PM_OPS(name, license, > > > ns) \ > > > const struct dev_pm_ops > > > name; \ > > > __EXPORT_SYMBOL(name, license, > > > ns); \ > > > const struct dev_pm_ops name > > > -#define EXPORT_PM_FN_GPL(name) EXPORT_SYMBOL_GPL(name) > > > -#define EXPORT_PM_FN_NS_GPL(name, > > > ns) EXPORT_SYMBOL_NS_GPL(name, > > > ns) > > > -#else > > > -#define _EXPORT_DEV_PM_OPS(name, license, > > > ns) \ > > > + > > > +#define _PM_OPS(name, license, > > > > This macro creates a dev_pm_ops that's meant to be garbage- > > collected by > > the compiler; so maybe name it _USELESS_PM_OPS() or something like > > that. > > _USELESS_PM_OPS() sounds a bit heavy handed ;) > Gives the impression that the macro itelf is not used anywhere in > code. > > Something like _DISCARD_PM_OPS() makes more sense. Works for me. > > Raag Cheers, -Paul
On Wed, Sep 20, 2023 at 12:00:05PM +0200, Paul Cercueil wrote: > Le mercredi 20 septembre 2023 à 12:33 +0300, Raag Jadav a écrit : > > On Wed, Sep 20, 2023 at 10:53:23AM +0200, Paul Cercueil wrote: > > > Le mercredi 20 septembre 2023 à 14:11 +0530, Raag Jadav a écrit : > > > > Currently EXPORT_*_SIMPLE_DEV_PM_OPS() use EXPORT_*_DEV_PM_OPS() > > > > set > > > > of macros to export dev_pm_ops symbol, which export the symbol in > > > > case > > > > CONFIG_PM=y but don't take CONFIG_PM_SLEEP into consideration. > > > > > > > > Since _SIMPLE_ variants of _PM_OPS() do not include runtime PM > > > > handles > > > > and are only used in case CONFIG_PM_SLEEP=y, we should not be > > > > exporting > > > > their dev_pm_ops symbol in case CONFIG_PM_SLEEP=n. > > > > > > > > This can be fixed by having two distinct set of export macros for > > > > both > > > > _RUNTIME_ and _SIMPLE_ variants of _PM_OPS(), such that the > > > > export of > > > > dev_pm_ops symbol used in each variant depends on CONFIG_PM and > > > > CONFIG_PM_SLEEP respectively. > > > > > > > > Introduce _DEV_SLEEP_PM_OPS() set of export macros for _SIMPLE_ > > > > variants > > > > of _PM_OPS(), which export dev_pm_ops symbol only in case > > > > CONFIG_PM_SLEEP=y > > > > and discard it otherwise. > > > > > > > > Fixes: 34e1ed189fab ("PM: Improve EXPORT_*_DEV_PM_OPS macros") > > > > Signed-off-by: Raag Jadav <raag.jadav@intel.com> > > > > --- > > > > PS: This is a standalone fix and works without updating any > > > > drivers. > > > > > > I had to double-check that, to make sure that none of the drivers > > > using > > > these macros also use pm_ptr() instead of pm_sleep_ptr() to access > > > the > > > exported dev_pm_ops. > > > > > > I did not check extensively but everything seems to use > > > pm_sleep_ptr(), > > > so it looks safe enough. > > > > I have tested it against -rc2 without any problems. > > You'd need to test an "allyesconfig" with CONFIG_PM=y, and > CONFIG_PM_SLEEP disabled. Is that what you tested? Yes, and also validated *.symvers if dev_pm_ops symbol is really discarded. Seems to work as expected. Raag
diff --git a/include/linux/pm.h b/include/linux/pm.h index 1400c37b29c7..99a8146fa479 100644 --- a/include/linux/pm.h +++ b/include/linux/pm.h @@ -374,24 +374,39 @@ const struct dev_pm_ops name = { \ RUNTIME_PM_OPS(runtime_suspend_fn, runtime_resume_fn, idle_fn) \ } -#ifdef CONFIG_PM -#define _EXPORT_DEV_PM_OPS(name, license, ns) \ +#define _EXPORT_PM_OPS(name, license, ns) \ const struct dev_pm_ops name; \ __EXPORT_SYMBOL(name, license, ns); \ const struct dev_pm_ops name -#define EXPORT_PM_FN_GPL(name) EXPORT_SYMBOL_GPL(name) -#define EXPORT_PM_FN_NS_GPL(name, ns) EXPORT_SYMBOL_NS_GPL(name, ns) -#else -#define _EXPORT_DEV_PM_OPS(name, license, ns) \ + +#define _PM_OPS(name, license, ns) \ static __maybe_unused const struct dev_pm_ops __static_##name + +#ifdef CONFIG_PM +#define _EXPORT_DEV_PM_OPS(name, license, ns) _EXPORT_PM_OPS(name, license, ns) +#define EXPORT_PM_FN_GPL(name) EXPORT_SYMBOL_GPL(name) +#define EXPORT_PM_FN_NS_GPL(name, ns) EXPORT_SYMBOL_NS_GPL(name, ns) +#else +#define _EXPORT_DEV_PM_OPS(name, license, ns) _PM_OPS(name, license, ns) #define EXPORT_PM_FN_GPL(name) #define EXPORT_PM_FN_NS_GPL(name, ns) #endif -#define EXPORT_DEV_PM_OPS(name) _EXPORT_DEV_PM_OPS(name, "", "") -#define EXPORT_GPL_DEV_PM_OPS(name) _EXPORT_DEV_PM_OPS(name, "GPL", "") -#define EXPORT_NS_DEV_PM_OPS(name, ns) _EXPORT_DEV_PM_OPS(name, "", #ns) -#define EXPORT_NS_GPL_DEV_PM_OPS(name, ns) _EXPORT_DEV_PM_OPS(name, "GPL", #ns) +#ifdef CONFIG_PM_SLEEP +#define _EXPORT_DEV_SLEEP_PM_OPS(name, license, ns) _EXPORT_PM_OPS(name, license, ns) +#else +#define _EXPORT_DEV_SLEEP_PM_OPS(name, license, ns) _PM_OPS(name, license, ns) +#endif + +#define EXPORT_DEV_PM_OPS(name) _EXPORT_DEV_PM_OPS(name, "", "") +#define EXPORT_GPL_DEV_PM_OPS(name) _EXPORT_DEV_PM_OPS(name, "GPL", "") +#define EXPORT_NS_DEV_PM_OPS(name, ns) _EXPORT_DEV_PM_OPS(name, "", #ns) +#define EXPORT_NS_GPL_DEV_PM_OPS(name, ns) _EXPORT_DEV_PM_OPS(name, "GPL", #ns) + +#define EXPORT_DEV_SLEEP_PM_OPS(name) _EXPORT_DEV_SLEEP_PM_OPS(name, "", "") +#define EXPORT_GPL_DEV_SLEEP_PM_OPS(name) _EXPORT_DEV_SLEEP_PM_OPS(name, "GPL", "") +#define EXPORT_NS_DEV_SLEEP_PM_OPS(name, ns) _EXPORT_DEV_SLEEP_PM_OPS(name, "", #ns) +#define EXPORT_NS_GPL_DEV_SLEEP_PM_OPS(name, ns) _EXPORT_DEV_SLEEP_PM_OPS(name, "GPL", #ns) /* * Use this if you want to use the same suspend and resume callbacks for suspend @@ -404,19 +419,19 @@ const struct dev_pm_ops name = { \ _DEFINE_DEV_PM_OPS(name, suspend_fn, resume_fn, NULL, NULL, NULL) #define EXPORT_SIMPLE_DEV_PM_OPS(name, suspend_fn, resume_fn) \ - EXPORT_DEV_PM_OPS(name) = { \ + EXPORT_DEV_SLEEP_PM_OPS(name) = { \ SYSTEM_SLEEP_PM_OPS(suspend_fn, resume_fn) \ } #define EXPORT_GPL_SIMPLE_DEV_PM_OPS(name, suspend_fn, resume_fn) \ - EXPORT_GPL_DEV_PM_OPS(name) = { \ + EXPORT_GPL_DEV_SLEEP_PM_OPS(name) = { \ SYSTEM_SLEEP_PM_OPS(suspend_fn, resume_fn) \ } #define EXPORT_NS_SIMPLE_DEV_PM_OPS(name, suspend_fn, resume_fn, ns) \ - EXPORT_NS_DEV_PM_OPS(name, ns) = { \ + EXPORT_NS_DEV_SLEEP_PM_OPS(name, ns) = { \ SYSTEM_SLEEP_PM_OPS(suspend_fn, resume_fn) \ } #define EXPORT_NS_GPL_SIMPLE_DEV_PM_OPS(name, suspend_fn, resume_fn, ns) \ - EXPORT_NS_GPL_DEV_PM_OPS(name, ns) = { \ + EXPORT_NS_GPL_DEV_SLEEP_PM_OPS(name, ns) = { \ SYSTEM_SLEEP_PM_OPS(suspend_fn, resume_fn) \ }
Currently EXPORT_*_SIMPLE_DEV_PM_OPS() use EXPORT_*_DEV_PM_OPS() set of macros to export dev_pm_ops symbol, which export the symbol in case CONFIG_PM=y but don't take CONFIG_PM_SLEEP into consideration. Since _SIMPLE_ variants of _PM_OPS() do not include runtime PM handles and are only used in case CONFIG_PM_SLEEP=y, we should not be exporting their dev_pm_ops symbol in case CONFIG_PM_SLEEP=n. This can be fixed by having two distinct set of export macros for both _RUNTIME_ and _SIMPLE_ variants of _PM_OPS(), such that the export of dev_pm_ops symbol used in each variant depends on CONFIG_PM and CONFIG_PM_SLEEP respectively. Introduce _DEV_SLEEP_PM_OPS() set of export macros for _SIMPLE_ variants of _PM_OPS(), which export dev_pm_ops symbol only in case CONFIG_PM_SLEEP=y and discard it otherwise. Fixes: 34e1ed189fab ("PM: Improve EXPORT_*_DEV_PM_OPS macros") Signed-off-by: Raag Jadav <raag.jadav@intel.com> --- PS: This is a standalone fix and works without updating any drivers. Changes since v2: - Drop redundant patches Changes since v1: - Update drivers to new set of macros include/linux/pm.h | 43 +++++++++++++++++++++++++++++-------------- 1 file changed, 29 insertions(+), 14 deletions(-)