Message-ID: <CAJZ5v0gsq5XDW8CW2My60Fn94F8h4XBFFUhKb19LcCwXLWvEfg@mail.gmail.com>
Date: Thu, 13 Feb 2025 21:42:39 +0100
From: "Rafael J. Wysocki" <rafael@...nel.org>
To: Ulf Hansson <ulf.hansson@...aro.org>
Cc: "Rafael J. Wysocki" <rafael@...nel.org>, "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 Thu, Feb 13, 2025 at 2:38 PM Ulf Hansson <ulf.hansson@...aro.org> wrote:
>
> On Wed, 12 Feb 2025 at 20:04, Rafael J. Wysocki <rafael@...nel.org> wrote:
> >
> > On Wed, Feb 12, 2025 at 6:05 PM 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.

BTW, I'm wondering how this driver handles the case in which someone
runs pm_runtime_get*() on its device and doesn't release the reference
throughout a system suspend-resume cycle, which is a valid thing to do
albeit unusual.

> > > > >
> > > > > 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.
> >
> > We may be making a logical mistake here by assuming that any of these
> > devices will end up in dependency chains starting at the devices that
> > I'm concerned about.
> >
> > > 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.
> >
> > Well, maybe not.
> >
> > So the "smart suspend" thing can be done only if all of the devices
> > above the given one in the dependency graph (including the parent,
> > suppliers etc) either opt in for "smart suspend" or are devices
> > without PM callbacks and the dependency graphs can be cut at devices
> > that don't support runtime PM.  This would require another flag in
> > struct dev_pm_info for the tracking of dependencies, but it should be
> > entirely doable because all of them are known at the device_prepare()
> > time.
> >
> > That should avoid involving anyone who can be surprised.
>
> This would be an improvement, while it is questionable for how
> valuable this would be in the end. As soon as there is a
> parent/supplier that has PM callbacks, we would need to prevent the
> propagation.

One without "smart suspend" that is.  Yes.

For the current users of DPM_FLAG_SMART_SUSPEND this shouldn't
actually matter because it is the case already AFAICS, so I'm not
worried about this.

> I am also a bit worried about adding more corner cases to
> the PM core, as it's not entirely easy to understand them, I think.

That is always a valid concern, but sometimes there is no other way to
make progress.

> That said, maybe it's the best we can do.

Powered by blists - more mailing lists