Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S966360Ab3E2OvO (ORCPT ); Wed, 29 May 2013 10:51:14 -0400 Received: from iolanthe.rowland.org ([192.131.102.54]:38958 "HELO iolanthe.rowland.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with SMTP id S966311Ab3E2OvM (ORCPT ); Wed, 29 May 2013 10:51:12 -0400 Date: Wed, 29 May 2013 10:51:11 -0400 (EDT) From: Alan Stern X-X-Sender: stern@iolanthe.rowland.org To: "Rafael J. Wysocki" cc: Linux PM list , ACPI Devel Maling List , LKML , Greg Kroah-Hartman , Bjorn Helgaas , Linux PCI , Kevin Hilman , Mika Westerberg Subject: Re: [PATCH RFC] PM / Runtime: Rework the "runtime idle" helper routine In-Reply-To: <1463685.qFyr66vqF2@vostro.rjw.lan> Message-ID: MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 3973 Lines: 103 On Wed, 29 May 2013, Rafael J. Wysocki wrote: > From: Rafael J. Wysocki > > 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-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/