On Wed, 29 May 2013, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@xxxxxxxxx>
>
> The "runtime idle" helper routine, rpm_idle(), currently ignores
> return values from .runtime_idle() callbacks executed by it.
>
> However, it turns out that many subsystems use the generic idle
> callback routine pm_generic_runtime_idle() which checks the return
> value of the driver's callback and executes pm_runtime_suspend() for
> the device unless that value is different from 0. If that logic is
> moved to rpm_idle() instead, pm_generic_runtime_idle() can be dropped
> and its users will not need any .runtime_idle() callbacks any more.
Since you're making this change, wouldn't it be a good idea to adopt
Mika's original suggestion and turn on the RPM_AUTO bit in rpmflags
when the use_autosuspend flag is set?
> Moreover, the PCI subsystem's .runtime_idle() routine,
> pci_pm_runtime_idle(), works in analogy with the generic one and if
> rpm_idle() calls rpm_suspend() after 0 has been returned by the
> .runtime_idle() callback executed by it, that routine will not be
> necessary any more and may be dropped.
See below.
What about cases where the runtime-idle callback does
rpm_schedule_suspend or rpm_request_suspend? You'd have to make sure
that it returns -EBUSY in such cases. Did you audit for this?
> Index: linux-pm/Documentation/power/runtime_pm.txt
> ===================================================================
> --- linux-pm.orig/Documentation/power/runtime_pm.txt
> +++ linux-pm/Documentation/power/runtime_pm.txt
> @@ -660,11 +660,6 @@ Subsystems may wish to conserve code spa
> management callbacks provided by the PM core, defined in
> driver/base/power/generic_ops.c:
>
> - int pm_generic_runtime_idle(struct device *dev);
> - - invoke the ->runtime_idle() callback provided by the driver of this
> - device, if defined, and call pm_runtime_suspend() for this device if the
> - return value is 0 or the callback is not defined
> -
The documentation for the runtime-idle callback needs to be updated too.
> Index: linux-pm/drivers/pci/pci-driver.c
> ===================================================================
> --- linux-pm.orig/drivers/pci/pci-driver.c
> +++ linux-pm/drivers/pci/pci-driver.c
> @@ -1046,32 +1046,6 @@ static int pci_pm_runtime_resume(struct
> return rc;
> }
>
> -static int pci_pm_runtime_idle(struct device *dev)
> -{
> - struct pci_dev *pci_dev = to_pci_dev(dev);
> - const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
> -
> - /*
> - * If pci_dev->driver is not set (unbound), the device should
> - * always remain in D0 regardless of the runtime PM status
> - */
> - if (!pci_dev->driver)
> - goto out;
> -
> - if (!pm)
> - return -ENOSYS;
> -
> - if (pm->runtime_idle) {
> - int ret = pm->runtime_idle(dev);
> - if (ret)
> - return ret;
> - }
> -
> -out:
> - pm_runtime_suspend(dev);
> - return 0;
> -}
This may not be a safe change, because now the behavior is different
in the case where dev->driver is set but pci_dev->driver isn't. The
difference is that you will now call the driver's runtime-idle
handler, whereas the existing code doesn't.
In fact, this may turn out to be a more widespread problem.
dev->driver gets set before the probe routine is called, and it gets
cleared after the remove routine is called. A runtime PM callback to
the driver during these windows isn't a good idea. Erasing subsystems'
runtime_idle handlers, as this patch does, makes it impossible for the
subsystems to protect against this.
The patch also needs to update
drivers/usb/core/driver.c:usb_runtime_idle(). If you include Mika's
suggestion, the routine can be removed entirely.
Alan Stern
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
