Message-ID: <CAPDyKFr1LmYji1Yh=mrx03eeW8ukFr9rE0hk277iam174TjGig@mail.gmail.com>
Date: Thu, 13 Feb 2025 14:10:53 +0100
From: Ulf Hansson <ulf.hansson@...aro.org>
To: "Rafael J. Wysocki" <rafael@...nel.org>
Cc: "Rafael J. Wysocki" <rjw@...ysocki.net>, Linux PM <linux-pm@...r.kernel.org>, 
	LKML <linux-kernel@...r.kernel.org>, Alan Stern <stern@...land.harvard.edu>, 
	Johan Hovold <johan@...nel.org>, Manivannan Sadhasivam <manivannan.sadhasivam@...aro.org>, 
	Jon Hunter <jonathanh@...dia.com>
Subject: Re: [PATCH v1 00/10] PM: Make the core and pm_runtime_force_suspend/resume()
 agree more

On Wed, 12 Feb 2025 at 18:05, Rafael J. Wysocki <rafael@...nel.org> wrote:
>
> On Wed, Feb 12, 2025 at 4:15 PM Ulf Hansson <ulf.hansson@...aro.org> wrote:
> >
> > On Wed, 12 Feb 2025 at 12:33, Rafael J. Wysocki <rafael@...nel.org> wrote:
> > >
> > > On Wed, Feb 12, 2025 at 11:59 AM Rafael J. Wysocki <rafael@...nel.org> wrote:
> > > >
> > > > On Wed, Feb 12, 2025 at 10:12 AM Ulf Hansson <ulf.hansson@...aro.org> wrote:
> > > > >
> > > > > On Tue, 11 Feb 2025 at 22:25, Rafael J. Wysocki <rjw@...ysocki.net> wrote:
> > > > > >
> > > > > > Hi Everyone,
> > > > > >
> > > > > > This series is a result of the discussion on a recently reported issue
> > > > > > with device runtime PM status propagation during system resume and
> > > > > > the resulting patches:
> > > > > >
> > > > > > https://lore.kernel.org/linux-pm/12619233.O9o76ZdvQC@rjwysocki.net/
> > > > > > https://lore.kernel.org/linux-pm/6137505.lOV4Wx5bFT@rjwysocki.net/
> > > > > >
> > > > > > Overall, due to restrictions related to pm_runtime_force_suspend() and
> > > > > > pm_runtime_force_resume(), it was necessary to limit the RPM_ACTIVE
> > > > > > setting propagation to the parent of the first device in a dependency
> > > > > > chain that turned out to have to be resumed during system resume even
> > > > > > though it was runtime-suspended before system suspend.
> > > > > >
> > > > > > Those restrictions are that (1) pm_runtime_force_suspend() attempts to
> > > > > > suspend devices that have never had runtime PM enabled if their runtime
> > > > > > PM status is currently RPM_ACTIVE and (2) pm_runtime_force_resume()
> > > > > > will skip device whose runtime PM status is currently RPM_ACTIVE.
> > > > > >
> > > > > > The purpose of this series is to eliminate the above restrictions and
> > > > > > get pm_runtime_force_suspend() and pm_runtime_force_resume() to agree
> > > > > > more with what the core does.
> > > > >
> > > > > For my understanding, would you mind elaborating a bit more around the
> > > > > end-goal with this?
> > > >
> > > > The end goal is, of course, full integration of runtime PM with system
> > > > sleep for all devices.  Eventually.
> > > >
> > > > And it is necessary to make the core and
> > > > pm_runtime_force_suspend|resume() work together better for this
> > > > purpose.
> > > >
> > > > > Are you trying to make adaptations for
> > > > > pm_runtime_force_suspend|resume() and the PM core, such that drivers
> > > > > that uses pm_runtime_force_suspend|resume() should be able to cope
> > > > > with other drivers for child-devices that make use of
> > > > > DPM_FLAG_SMART_SUSPEND?
> > > >
> > > > Yes.
> > > >
> > > > This is a more general case, though, when a device that was
> > > > runtime-suspended before system suspend and is left in suspend during
> > > > it, needs to be resumed during the system resume that follows.
> > > >
> > > > Currently, DPM_FLAG_SMART_SUSPEND can lead to this sometimes and it
> > > > cannot happen otherwise, but I think that it is a generally valid
> > > > case.
> > > >
> > > > > If we can make this work, it would enable the propagation of
> > > > > RPM_ACTIVE in the PM core for more devices, but still not for all,
> > > > > right?
> > > >
> > > > It is all until there is a known case in which it isn't.  So either
> > > > you know a specific case in which it doesn't work, or I don't see a
> > > > reason for avoiding it.
> > > >
> > > > ATM the only specific case in which it doesn't work is when
> > > > pm_runtime_force_suspend|resume() are used.
> > > >
> > > > > The point is, the other bigger issue that I pointed out in our earlier
> > > > > discussions; all those devices where their drivers/buses don't cope
> > > > > with the behaviour of the DPM_FLAG_SMART_SUSPEND flag, will prevent
> > > > > the PM core from *unconditionally* propagating the RPM_ACTIVE to
> > > > > parents. I guess this is the best we can do then?
> > > >
> > > > OK, what are they?
> > > >
> > > > You keep saying that they exist without giving any examples.
> > >
> > > To put it more bluntly, I'm not aware of any place other than
> > > pm_runtime_force_suspend|resume() that can be confused by changing the
> > > runtime PM status of a device with runtime PM disabled (either
> > > permanently, or transiently during a system suspend-resume cycle) to
> > > RPM_ACTIVE, so if there are any such places, I would appreciate
> > > letting me know what they are.
> >
> > Well, sorry I thought you were aware. Anyway, I believe you need to do
> > your own investigation as it's simply too time consuming for me to
> > find them all. The problem is that it's not just a simple pattern to
> > search for, so we would need some clever scripting to move forward to
> > find them.
> >
> > To start with, we should look for drivers that enable runtime PM, by
> > calling pm_runtime_enable().
> >
> > Additionally, in their system suspend callback they should typically
> > *not* use pm_runtime_suspended(), pm_runtime_status_suspended() or
> > pm_runtime_active() as that is usually (but no always) indicating that
> > they got it right. Then there are those that don't have system
> > suspend/resume callbacks assigned at all (or uses some other subsystem
> > specific callback for this), but only uses runtime PM.
> >
> > On top of that, drivers that makes use of
> > pm_runtime_force_suspend|resume() should be disregarded, which has
> > reached beyond 300 as this point.
> >
> > Anyway, here is just one example that I found from a quick search.
> > drivers/i2c/busses/i2c-qcom-geni.c
>
> OK, I see.
>
> It sets the status to RPM_SUSPENDED in geni_i2c_suspend_noirq(), if
> not suspended already, and assumes it to stay this way across
> geni_i2c_resume_noirq() until someone resumes it via runtime PM.
>
> Fair enough, but somebody should tell them that they don't need to use
> pm_runtime_disable/enable() in _noirq.
>
> > >
> > > Note that rpm_active() could start producing confusing results if the
> > > runtime PM status of a device with runtime PM disabled is changed from
> > > RPM_ACTIVE to RPM_SUSPENDED because it will then start to return
> > > -EACCES instead of 1, but changing the status to RPM_ACTIVE will not
> > > confuse it the same way.
> >
> > Trust me, it will cause problems.
> >
> > Drivers may call pm_runtime_get_sync() to turn on the resources for
> > the device after the system has resumed, when runtime PM has been
> > re-enabled for the device by the PM core.
> >
> > Those calls to pm_runtime_get_sync() will then not end up invoking any
> > if ->runtime_resume() callbacks for the device since its state is
> > already RPM_ACTIVE. Hence, the device will remain in a low power state
> > even if the driver believes it has been powered on. In many cases,
> > accessing the device (like reading/writing a register) will often just
> > just hang in these cases, but in worst cases we could end up getting
> > even more difficult bugs to debug.
>
> Sure, that would be a problem.
>
> I think I need to find a different way to address the problem I'm
> seeing, that is to resume devices that were runtime-suspended before
> system suspend along with their superiors.
>
> One way to do it would be to just defer their resume to the point when
> the core has enabled runtime PM for them, which means that it has also
> enabled runtime PM for all of their superiors, and then call
> pm_runtime_resume().
>
> This should work unless one of the superiors has runtime PM disabled
> at that point, of course.

Right, so typically users of pm_runtime_force_suspend|resume() from
the regular ->suspend|resume() callbacks would not work, because they
disable/enable runtime PM. Although, we could probably fix this quite
easily by making some adaptations in
pm_runtime_force_suspend|resume().

I am not sure if this approach would have any other issue though, but
it seems like it could make sense to explore this approach. In general
drivers should cope with their devices being runtime resumed if
runtime PM is enabled, right!?

If this works, it seems like a generic and a good solution to me.

Kind regards
Uffe

Powered by blists - more mailing lists