Message-ID: <CAD=FV=XxCMMLAyfnf=MBu--Fkcjxza6603deCA-dOCCXKeX7uQ@mail.gmail.com>
Date:   Tue, 1 Mar 2022 08:13:46 -0800
From:   Doug Anderson <dianders@...omium.org>
To:     Ulf Hansson <ulf.hansson@...aro.org>
Cc:     "Rafael J . Wysocki" <rafael@...nel.org>,
        Dmitry Baryshkov <dmitry.baryshkov@...aro.org>,
        Linus Walleij <linus.walleij@...aro.org>,
        Stephen Boyd <sboyd@...nel.org>,
        Laurent Pinchart <laurent.pinchart@...asonboard.com>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Len Brown <len.brown@...el.com>, Pavel Machek <pavel@....cz>,
        LKML <linux-kernel@...r.kernel.org>,
        Linux PM <linux-pm@...r.kernel.org>
Subject: Re: [PATCH] PM: runtime: Have devm_pm_runtime_enable() handle pm_runtime_dont_use_autosuspend()

Hi,

On Tue, Mar 1, 2022 at 2:27 AM Ulf Hansson <ulf.hansson@...aro.org> wrote:
>
> On Wed, 23 Feb 2022 at 17:35, Douglas Anderson <dianders@...omium.org> wrote:
> >
> > The PM Runtime docs say:
> >   Drivers in ->remove() callback should undo the runtime PM changes done
> >   in ->probe(). Usually this means calling pm_runtime_disable(),
> >   pm_runtime_dont_use_autosuspend() etc.
> >
> > From grepping code, it's clear that many people aren't aware of the
> > need to call pm_runtime_dont_use_autosuspend().
>
> Well, I admit it's good practice that they should take care of this.
>
> However, it doesn't really matter to keep the autosuspend turned on
> when runtime PM becomes disabled, I think. When the driver gets probed
> again, it will most likely call pm_runtime_use_autosuspend() again,
> which should work fine, right?
>
> >
> > When brainstorming solutions, one idea that came up was to leverage
> > the new-ish devm_pm_runtime_enable() function. The idea here is that:
> > * When the devm action is called we know that the driver is being
> >   removed. It's the perfect time to undo the use_autosuspend.
> > * The code of pm_runtime_dont_use_autosuspend() already handles the
> >   case of being called when autosuspend wasn't enabled.
>
> Hmm, I am hesitating to extend devm_pm_runtime_enable(), as it
> currently makes it look too simple to turn off things at ->remove()
> for runtime PM. While in fact it's more complicated.
>
> A bigger problem, for example, is that a driver calls
> pm_runtime_put_sync() during ->remove(), relying on that it actually
> ends up calling its ->runtime_suspend() callback to turn off various
> specific resources for the device. And in fact there are no guarantees
> that will happen - and when it doesn't, the next time the driver's
> ->probe() runs, things are likely to be really screwed up.
>
> To cover this case, one could use the below code in the ->remove() callback:
>
> ...
> pm_runtime_get_sync();
>
> "turn off resources for the devices - like calling
> clk_disable_unprepare(), for example"
>
> pm_runtime_disable();
> pm_runtime_put_noidle();
> ...
>
> In this example, it would be too late to call pm_runtime_disable()
> through the pm_runtime_disable_action().

In the example you're listing above, though, you can't use
devm_pm_runtime_enable() anyway, right? If you have a reason not to
use the devm approach then that's fine and this patch won't affect
you. However, if you're using devm_pm_runtime_enable() already then
there should be no downside, right?

Though I'm by no means a Runtime PM expert, I think the most common
use case might be when a driver is kept simple by _rely_ing on
CONFIG_PM. Certainly folks have debated about this in the past, but at
least for a set of drivers where nobody has a need for running them
without CONFIG_PM they can be kept simpler by relying on it. When you
do this, you don't need quite as many machinations with regards to
playing with power management when runtime PM is off. You can
basically just enable runtime power management and assume that the
runtime resume / runtime suspend will do the work of powering you,
right?


-Doug

Powered by blists - more mailing lists