Re: [PATCH v2 04/10] pinctrl: intel: Switch to use DEFINE_NOIRQ_DEV_PM_OPS() helper

[Paul Cercueil]

Le lundi 17 juillet 2023 à 22:33 +0300, Andy Shevchenko a écrit :
> On Mon, Jul 17, 2023 at 10:02 PM Paul Cercueil <paul@xxxxxxxxxxxxxxx>
> wrote:
> > Le lundi 17 juillet 2023 à 20:28 +0300, Andy Shevchenko a écrit :
> 
> ...
> 
> > Unrelated change.
> 
> OK.
> 
> ...
> 
> > So the correct way to update this driver would be to have a
> > conditionally-exported dev_pm_ops structure:
> > 
> > EXPORT_GPL_DEV_PM_OPS(intel_pinctrl_pm_ops) = {
> >     NOIRQ_SYSTEM_SLEEP_PM_OPS(intel_pinctrl_suspend_noirq,
> > intel_pinctrl_resume_noirq),
> > };
> 
> This looks ugly. I didn't know that EXPORT*PM_OPS designed that way,
> but it seems pm.h in such case needs EXPORT for NOIRQ case as well.

It's designed so that when CONFIG_PM is disabled, the dev_pm_ops is
garbage-collected along with all its callbacks.

I know it looks ugly, but we already have 4 variants (regular,
namespace, GPL, namespace + GPL), if we start to add macros for
specific use-cases then it will become bloated really quick.

And the "bloat" I'm trying to avoid here is the extreme expansion of
the API which makes it hard for people not familiar to the code to
understand what should be used and how.

> > Then your two callbacks can be "static" and without #ifdef guards.
> > 
> > The resulting "intel_pinctrl_pm_ops" can be marked as "extern" in
> > the
> > pinctrl-intel.h without any guards, as long as it is only
> > referenced
> > with the pm_ptr() macro.
> 
> I'm not sure I got this. Currently drivers do not have any guards.
> Moreover, the correct one for noirq is pm_sleep_ptr(), isn't it?
> 

The EXPORT_*_DEV_PM_OPS() macros do export the "dev_pm_ops"
conditionally depending on CONFIG_PM. We could add variants that export
it conditionally depending on CONFIG_PM_SLEEP, but we're back at the
problem of adding bloat.

You could use pm_sleep_ptr() indeed, with the existing macros, with the
drawback that in the case where CONFIG_PM && !CONFIG_PM_SLEEP, the
dev_pm_ops + callbacks are compiled in but never referenced.

Cheers,
-Paul