Hi Jonathan,
On Mon, 27 Jan 2025 at 19:24, Jonathan Cameron
<Jonathan.Cameron@xxxxxxxxxx> wrote:
> On Mon, 27 Jan 2025 16:02:32 +0100
> Ulf Hansson <ulf.hansson@xxxxxxxxxx> wrote:
> > On Mon, 27 Jan 2025 at 13:32, Jonathan Cameron
> > <Jonathan.Cameron@xxxxxxxxxx> wrote:
> > > On Mon, 27 Jan 2025 11:47:44 +0100
> > > Ulf Hansson <ulf.hansson@xxxxxxxxxx> wrote:
> > > > > > > > > Do consider OK to change the order in pm_runtime_disable_action() to get
> > > > > > > > > rid of these issues, e.g.:
> > > > > > > > >
> > > > > > > > > diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c
> > > > > > > > > index 2ee45841486b..f27d311d2619 100644
> > > > > > > > > --- a/drivers/base/power/runtime.c
> > > > > > > > > +++ b/drivers/base/power/runtime.c
> > > > > > > > > @@ -1547,8 +1547,8 @@ EXPORT_SYMBOL_GPL(pm_runtime_enable);
> > > > > > > > >
> > > > > > > > > static void pm_runtime_disable_action(void *data)
> > > > > > > > > {
> > > > > > > > > - pm_runtime_dont_use_autosuspend(data);
> > > > > > > > > pm_runtime_disable(data);
> > > > > > > > > + pm_runtime_dont_use_autosuspend(data);
> > > > > > > > > }
> > > > > > > > >
> > > > > > > > > though I see a rpm_resume() call is still possible though pm_runtime_disable().
> > > > > > > >
> > > > > > > > I am still worried about keeping the device runtime enabled during a
> > > > > > > > window when we have turned off all resources for the device. Typically
> > > > > > > > we want to leave the device in a low power state after unbind.
> > > > > > > >
> > > > > > > > That said, I would rather just drop the devm_pm_runtime_enable() API
> > > > > > > > altogether and convert all users of it into
> > > > > > > > pm_runtime_enable|disable(), similar to what your patch does.
> > > > > > >
> > > > > > > That is making a mess of a lot of automated cleanup for a strange
> > > > > > > runtime pm related path. This is pain a driver should not have
> > > > > > > to deal with, though I'm not clear what the right solution is!
> > > > > > >
> > > > > > > Key is that drivers should not mix devm managed cleanup and not, so
> > > > > > > that means that anything that happens after runtime pm is enabled
> > > > > > > has to be torn down manually. One solution to this might be to
> > > > > > > always enable it late assuming that is safe to do so there is
> > > > > > > never anything else done after it in the probe path of a driver.
> > > > > >
> > > > > > The problem is that runtime PM isn't really comparable to other
> > > > > > resources that we are managing through devm* functions.
> > > > > >
> > > > > > Enabling runtime PM for a device changes the behaviour for how
> > > > > > power-mgmt is handled for the device. Enabling/disabling of runtime PM
> > > > > > really needs to be explicitly controlled by the driver for the device.
> > > > >
> > > > > I'm sorry to say I'm not yet convinced.
> > > >
> > > > Okay, let me try one more time. :-)
> > >
> > > +CC Greg as the disagreement here is really a philosophy of what
> > > devm cleanup is relative to remove. Perhaps Greg or Rafael can
> > > given some guidance on the intent there.
> > >
> > > Mind you I think I found another subsystem working around this
> > > and in a somewhat more elegant, general way (to my eyes anyway!)
> > >
> > > https://elixir.bootlin.com/linux/v6.12.6/source/drivers/i2c/i2c-core-base.c#L630
> > > https://lore.kernel.org/all/YFf1GFPephFxC0mC@xxxxxxxxxx/
> > >
> > > +CC Dmitry.
> > >
> > > I2C creates an extra devres group and releases it before devm_pm_domain_detach()
> > > As all devm calls from the driver end up in that group, they are released
> > > before dev_pm_domain_detach()
> >
> > How would that address the problem I pointed out with runtime PM
> > below? This problem isn't limited to attaching/detaching PM domains.
>
> It's associated with anything that happens after a driver remove is done.
> We just disagree on when that remove is finished. There is nothing special about
> the remove() callback, that is just part of remove process.
> No magic transition of state that allows new things to happen follows
> the device driver remove finishing. Sure you can get the remove
> handling ordering wrong whether devm is in use or not. The trick is
> almost always to never mix devm and not. Once you need a single bit of
> manual unwinding stop with the devm and do everything beyond that point
> by hand (in probe order, before that point in remove order)
>
> > > > > Devm callbacks are explicitly registered by the driver so that they
> > > > > are unwound in a specific order. Many other parts of driver
> > > > > registration rely on this ordering. This does not seem different
> > > > > for runtime PM than anything else.
> > > >
> > > > If you compare clocks, for example. It's the driver that is in full
> > > > control of the clock gating/ungating. When the ->remove() callback
> > > > runs, the driver typically makes sure that it leaves the clock gated.
> > > > Then it doesn't really matter when the clock resource gets released.
> > > > The point is, the driver is in full control of the resource.
> > >
> > > Not a good example. devm_clk_get_enabled() does not gate the clock until
> >
> > I was not referring to devm_clk_get_enable(), but rather just devm_clk_get().
> >
> > To me devm_clk_get_enable() is another interface that we should avoid.
> > For example, what if the clock is already gated when the ->remove()
> > callback runs? Then we need to ungate the clock just to make the
> > devres path happy so it doesn't gate an already gated clock. And this,
> > just to save one or two lines of code.
>
> If someone is using a clock that is gated by other calls in the driver
> that then indeed the use of devm_clk_get_enabled() is inappropriate
> or they turn they do need to enable the clock and turn it off again
> as you mention which is a mess but often needs doing anyway as we
> commonly need some clocks at least to put a device into a low power
> state.
>
> >
> > Don't get me wrong, I certainly like the devm* functions in general,
> > but it's not a good fit for everything.
>
> For anything that a driver otherwise calls in remove() they are
> a direct equivalent that is just called automatically. (though apparently
> not quite in platform drivers!) If you have a sequence that is
> sufficiently complex they may not be a good fit. I just don't think
> that the sequence in this driver (and many others) is complex.
> The driver code (with the above change from i2c ported to platform code
> to fix the specific problem) is a lot simpler before Claudia's v2 to
> change the handling. 32 lines added to work around this...
>
> >
> > > the devm cleanup. The assumption being that nothing that affects
> > > it runs between the remove() and devm cleanup. So pretty much identical
> > > to the runtime pm case. They being that you have to obey ordering so
> > > that if you need to run something after the clock is disabled then
> > > you register that callback before you call devm_clk_get_enabled()
> > >
> > > > If runtime PM would remain enabled beyond the call to the ->remove()
> > > > callback, it would mean that the driver's runtime PM callbacks could
> > > > be called too. For example, userspace via sysfs may at any point
> > > > decide to runtime resume the device. In other words, we may end up
> > > > calling the runtime PM callbacks in the driver, when they are not
> > > > intended to be called. In the worst case, I guess we could even end up
> > > > trying to control resources (like a clock) from the ->runtime
> > > > _resume() callback, when the references to these resources may already
> > > > have been released.
> > >
> > > This is all about what we consider remove. To me, with devm_ manged cleanup
> > > in place, both remove() and devm_ cleanup count as parts of that remove
> > > process.
> >
> > There is no straightforward process here, if you would keep runtime PM
> > enabled beyond ->remove(). Things can happen in parallel.
>
> How? Nothing magic happens when a driver remove() ends.
> So there is no difference at all in a driver calling runtime pm disable
> in that code or in devres cleanup that happens immediately after that
> in the bus driver remove.
>
> > In that case, drivers would need to extend their runtime PM callbacks
> > to cope with more complicated conditions, as resources that those use
> > may have been released. Moreover, how can we make sure that the device
> > is put into a low power state after the ->remove() has been called?
>
> This is a fair question. Also one commonly handled by drivers using devm
> though perhaps not for the reason you are thinking. Key is that most
> drivers should not rely at all on runtime PM being built let alone
> enabled.
Drivers for components on SoCs that use PM Domains must use
Runtime PM, as that is the only available method to control the PM Domain.
> Solution is normally a devm_add_action_or_reset() call that
> registers a call to put the device into a low power state.
>
> Sequence is normally something like:
>
> 1) enable clks etc.
Can be done explicitly, or using pm_runtime_resume_and_get() in case
of a clock domain.
> 2) Turn the power on.
I assume you mean through a regulator, GPIO, or (deasserted) reset
signal?
In case of a PM Domain, that is done using pm_runtime_resume_and_get()
again.
> 3) device setup
> 4) enable runtime PM after setting the state to active. Let autosuspend
> do it's work.
>
> On remove (all devm_ managed).
> 5) disable runtime PM.
> 6) Turn the power off (may involve a check on whether it is already
> off though in many cases it's idempotent so we don't bother checking).
> 7) disable clocks.
>
> in this particular driver I'm assuming that the low power state is
> handled via the reset lines being put back into reset by the
> the unwind of the two
> devm_reset_control_get_exclusive_deasserted() calls.
No, power control is handled by the PM (Power and Clock!) Domain, which
controls both the power area (R9A08G045_PD_ADC) and the module clocks
(R9A08G045_ADC_ADCLK, R9A08G045_ADC_PCLK).
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@xxxxxxxxxxxxxx
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
