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).
---
drivers/base/power/main.c | 20 ++++++++++++++++----
drivers/base/power/runtime.c | 41 ++++++++++++++++++++++++++++++++++++-----
include/linux/pm_runtime.h | 5 +++++
3 files changed, 57 insertions(+), 9 deletions(-)
--- a/drivers/base/power/main.c
+++ b/drivers/base/power/main.c
@@ -655,14 +655,25 @@
* this device later, it needs to appear as "suspended" to PM-runtime,
* so change its status accordingly.
*
- * Otherwise, the device is going to be resumed, so set its PM-runtime
- * status to "active" unless its power.set_active flag is clear, in
- * which case it is not necessary to update its PM-runtime status.
+ * Otherwise, the device is going to be resumed, so try to update its
+ * PM-runtime status unless its power.set_active flag is clear, in which
+ * case it is not necessary to do that.
*/
if (skip_resume) {
pm_runtime_set_suspended(dev);
} else if (dev->power.set_active) {
- pm_runtime_set_active(dev);
+ /*
+ * If PM-runtime is not going to be actually enabled for the
+ * device by a subsequent pm_runtime_enabled() call, it may
+ * have never been enabled or pm_runtime_force_suspend() may
+ * have been used. Don't update the PM-runtime statue in
+ * that case and set power.needs_force_resume in case
+ * pm_runtime_force_resume() will be called for the device
+ * subsequently.
+ */
+ if (pm_runtime_cond_set_active(dev) > 0)
+ dev->power.needs_force_resume = true;
+
dev->power.set_active = false;
}
@@ -988,6 +999,7 @@
End:
error = dpm_run_callback(callback, dev, state, info);
dev->power.is_suspended = false;
+ dev->power.needs_force_resume = false;
Unlock:
device_unlock(dev);
--- a/drivers/base/power/runtime.c
+++ b/drivers/base/power/runtime.c
@@ -1253,9 +1253,10 @@
EXPORT_SYMBOL_GPL(pm_runtime_get_if_in_use);
/**
- * __pm_runtime_set_status - Set runtime PM status of a device.
+ * pm_runtime_set_status_internal - Set runtime PM status of a device.
* @dev: Device to handle.
* @status: New runtime PM status of the device.
+ * @cond: Change the status if runtime PM will be enabled by the next attempt.
*
* If runtime PM of the device is disabled or its power.runtime_error field is
* different from zero, the status may be changed either to RPM_ACTIVE, or to
@@ -1275,8 +1276,12 @@
* of the @status value) and the suppliers will be deacticated on exit. The
* error returned by the failing supplier activation will be returned in that
* case.
+ *
+ * If @cond is set, only change the status if power.disable_depth is equal to 1,
+ * or do nothing and return (power.disable_depth - 1) otherwise.
*/
-int __pm_runtime_set_status(struct device *dev, unsigned int status)
+static int pm_runtime_set_status_internal(struct device *dev,
+ unsigned int status, bool cond)
{
struct device *parent = dev->parent;
bool notify_parent = false;
@@ -1292,10 +1297,26 @@
* Prevent PM-runtime from being enabled for the device or return an
* error if it is enabled already and working.
*/
- if (dev->power.runtime_error || dev->power.disable_depth)
- dev->power.disable_depth++;
- else
+ if (dev->power.runtime_error) {
+ if (cond)
+ error = -EINVAL;
+ else
+ dev->power.disable_depth++;
+ } else if (dev->power.disable_depth) {
+ /*
+ * If cond is set, only attempt to change the status if the
+ * next invocation of pm_runtime_enable() for the device is
+ * going to actually enable runtime PM for it.
+ *
+ * This is used in a corner case during system-wide resume.
+ */
+ if (cond && dev->power.disable_depth > 1)
+ error = dev->power.disable_depth - 1;
+ else
+ dev->power.disable_depth++;
+ } else {
error = -EAGAIN;
+ }
spin_unlock_irqrestore(&dev->power.lock, flags);
@@ -1376,6 +1397,16 @@
return error;
}
+
+int pm_runtime_cond_set_active(struct device *dev)
+{
+ return pm_runtime_set_status_internal(dev, RPM_ACTIVE, true);
+}
+
+int __pm_runtime_set_status(struct device *dev, unsigned int status)
+{
+ return pm_runtime_set_status_internal(dev, status, false);
+}
EXPORT_SYMBOL_GPL(__pm_runtime_set_status);
/**
--- a/include/linux/pm_runtime.h
+++ b/include/linux/pm_runtime.h
@@ -75,6 +75,7 @@
extern int pm_runtime_get_if_active(struct device *dev);
extern int pm_runtime_get_if_in_use(struct device *dev);
extern int pm_schedule_suspend(struct device *dev, unsigned int delay);
+extern int pm_runtime_cond_set_active(struct device *dev);
extern int __pm_runtime_set_status(struct device *dev, unsigned int status);
extern int pm_runtime_barrier(struct device *dev);
extern void pm_runtime_enable(struct device *dev);
@@ -268,6 +269,10 @@
{
return -EINVAL;
}
+static inline int pm_runtime_cond_set_active(struct device *dev)
+{
+ return 1;
+}
static inline int __pm_runtime_set_status(struct device *dev,
unsigned int status) { return 0; }
static inline int pm_runtime_barrier(struct device *dev) { return 0; }
