On Fri, Feb 7, 2025 at 7:14 PM Rafael J. Wysocki <rafael@xxxxxxxxxx> wrote:
>
> On Fri, Feb 7, 2025 at 5:26 PM Johan Hovold <johan@xxxxxxxxxx> wrote:
> >
> > On Fri, Feb 07, 2025 at 05:06:32PM +0100, Johan Hovold wrote:
> > > On Fri, Feb 07, 2025 at 04:41:18PM +0100, Rafael J. Wysocki wrote:
> > > > On Fri, Feb 7, 2025 at 3:45 PM Johan Hovold <johan@xxxxxxxxxx> wrote:
> > > > > On Fri, Feb 07, 2025 at 02:50:29PM +0100, Johan Hovold wrote:
> > >
> > > > > Ok, so the driver data is never set and runtime PM is never enabled for
> > > > > this simple bus device, which uses pm_runtime_force_suspend() for system
> > > > > sleep.
> > > >
> > > > This is kind of confusing. Why use pm_runtime_force_suspend() if
> > > > runtime PM is never enabled and cannot really work?
> > >
> > > It's only done for some buses that this driver handles. The driver is
> > > buggy; I'm preparing a fix for it regardless of the correctness of the
> > > commit that now triggered this.
> >
> > Hmm. The driver implementation is highly odd, but actually works as long
> > as the runtime PM status is left at 'suspended' (as
> > pm_runtime_force_resume() won't enable runtime PM unless it was enabled
> > before suspend).
> >
> > So we'd strictly only need something like the below if we are going to
> > keep the set_active propagation.
>
> I think you are right.
>
> > diff --git a/drivers/bus/simple-pm-bus.c b/drivers/bus/simple-pm-bus.c
> > index 5dea31769f9a..d8e029e7e53f 100644
> > --- a/drivers/bus/simple-pm-bus.c
> > +++ b/drivers/bus/simple-pm-bus.c
> > @@ -109,9 +109,29 @@ static int simple_pm_bus_runtime_resume(struct device *dev)
> > return 0;
> > }
> >
> > +static int simple_pm_bus_suspend(struct device *dev)
> > +{
> > + struct simple_pm_bus *bus = dev_get_drvdata(dev);
> > +
> > + if (!bus)
> > + return 0;
> > +
> > + return pm_runtime_force_suspend(dev);
> > +}
> > +
> > +static int simple_pm_bus_resume(struct device *dev)
> > +{
> > + struct simple_pm_bus *bus = dev_get_drvdata(dev);
> > +
> > + if (!bus)
> > + return 0;
> > +
> > + return pm_runtime_force_resume(dev);
> > +}
> > +
> > static const struct dev_pm_ops simple_pm_bus_pm_ops = {
> > RUNTIME_PM_OPS(simple_pm_bus_runtime_suspend, simple_pm_bus_runtime_resume, NULL)
> > - NOIRQ_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend, pm_runtime_force_resume)
> > + NOIRQ_SYSTEM_SLEEP_PM_OPS(simple_pm_bus_suspend, simple_pm_bus_resume)
> > };
> >
> > #define ONLY_BUS ((void *) 1) /* Match if the device is only a bus. */
>
> In the meantime, I've cut the attached (untested) patch that should
> take care of the pm_runtime_force_suspend() issue altogether.
>
> It changes the code to only set the device's runtime PM status to
> "active" if runtime PM is going to be enabled for it by the first
> pm_runtime_enable() call, which never happens for devices where
> runtime PM has never been enabled (because it is disabled for them
> once again in device_suspend_late()) and for devices subject to
> pm_runtime_force_suspend() during system suspend (because it disables
> runtime PM for them once again).
All right, this is not going to work.
I see two problems and both are related to pm_runtime_force_suspend/resume().
The first problem is that pm_runtime_force_suspend() doesn't
distinguish devices for which runtime PM has never been enabled from
devices that have been runtime-suspended. This clearly is a mistake
as demonstrated by the breakage at hand.
The second problem is that pm_runtime_force_resume() expects all
devices to be resumed to have both runtime PM status set to
RPM_SUSPENDED and power.needs_force_resume set. AFAICS, checking
power.needs_force_resume alone should be sufficient there, but even
that is problematic because something needs to set the flag for
devices that are expected to be resumed.
Unfortunately, they both are not straightforward to address. I have
ideas, but nothing clear yet.
For now, the power.set_active propagation can be restricted to the
parent of the device with DPM_FLAG_SMART_SUSPEND set that needs to be
resumed and that will just be sufficient ATM, so attached is a patch
doing this.
In the future, though, all of this needs to be addressed properly
because if one device in a dependency chain needs to be resumed,
whatever the reason, all of the devices depended on by it need to be
resumed as well.
---
drivers/base/power/main.c | 21 +++++++++------------
1 file changed, 9 insertions(+), 12 deletions(-)
--- a/drivers/base/power/main.c
+++ b/drivers/base/power/main.c
@@ -1191,24 +1191,18 @@
return PMSG_ON;
}
-static void dpm_superior_set_must_resume(struct device *dev, bool set_active)
+static void dpm_superior_set_must_resume(struct device *dev)
{
struct device_link *link;
int idx;
- if (dev->parent) {
+ if (dev->parent)
dev->parent->power.must_resume = true;
- if (set_active)
- dev->parent->power.set_active = true;
- }
idx = device_links_read_lock();
- list_for_each_entry_rcu_locked(link, &dev->links.suppliers, c_node) {
+ list_for_each_entry_rcu_locked(link, &dev->links.suppliers, c_node)
link->supplier->power.must_resume = true;
- if (set_active)
- link->supplier->power.set_active = true;
- }
device_links_read_unlock(idx);
}
@@ -1287,9 +1281,12 @@
dev->power.must_resume = true;
if (dev->power.must_resume) {
- dev->power.set_active = dev->power.set_active ||
- dev_pm_test_driver_flags(dev, DPM_FLAG_SMART_SUSPEND);
- dpm_superior_set_must_resume(dev, dev->power.set_active);
+ if (dev_pm_test_driver_flags(dev, DPM_FLAG_SMART_SUSPEND)) {
+ dev->power.set_active = true;
+ if (dev->parent)
+ dev->parent->power.set_active = true;
+ }
+ dpm_superior_set_must_resume(dev);
}
Complete:
