On Tuesday 01 June 2010, Alan Stern wrote:
> On Tue, 1 Jun 2010, Matthew Garrett wrote:
>
> > On Tue, Jun 01, 2010 at 10:59:17AM -0400, Alan Stern wrote:
> >
> > > Can we rely on the forbid/allow mechanism? If userspace never allows
> > > runtime suspend for devices like the video controller then there's no
> > > problem. Conversely, if a PCI device really doesn't have a driver and
> > > isn't used for anything, then by default it should go into a lower
> > > power state if userspace allows this.
> >
> > I think that's safe, yes. Other fringe concerns include things like
> > smbus controllers - we typically won't allow a driver to bind to them
> > because they're used by ACPI, so putting the in D3 is also an error. So
> > I think Rafael's right in that we should default to forbid unless a
> > driver knows that it's safe, and let userspace override us.
>
> Therefore the default state for unbound PCI devices should be
> runtime-PM-enabled.
Why do you think it should?
> Devices will be put in a low-power state if userspace allows. Also, the PCI
> core should make sure that devices are active when drivers' probe and
> release methods are called.
>
> If a driver doesn't want to use runtime PM then not doing anything at
> all should leave the device with a positive usage count. If it does
> want to use runtime PM then a pm_runtime_put_sync() during probe and
> pm_runtime_get_sync() during release will take care of everything.
>
> Something like the patch below, awkward though it is. Does this look
> reasonable?
I'm not sure if r8169 and e1000e will work with it. Probably not, but I'm
too tired to check right now. :-)
> Index: usb-2.6/drivers/pci/bus.c
> ===================================================================
> --- usb-2.6.orig/drivers/pci/bus.c
> +++ usb-2.6/drivers/pci/bus.c
> @@ -15,6 +15,7 @@
> #include <linux/proc_fs.h>
> #include <linux/init.h>
> #include <linux/slab.h>
> +#include <linux/pm_runtime.h>
>
> #include "pci.h"
>
> @@ -134,6 +135,9 @@ pci_bus_alloc_resource(struct pci_bus *b
> int pci_bus_add_device(struct pci_dev *dev)
> {
> int retval;
> +
> + pm_runtime_set_active(&dev->dev);
> + pm_runtime_enable(&dev->dev);
> retval = device_add(&dev->dev);
> if (retval)
> return retval;
> Index: usb-2.6/drivers/pci/pci-driver.c
> ===================================================================
> --- usb-2.6.orig/drivers/pci/pci-driver.c
> +++ usb-2.6/drivers/pci/pci-driver.c
> @@ -289,8 +289,23 @@ struct drv_dev_and_id {
> static long local_pci_probe(void *_ddi)
> {
> struct drv_dev_and_id *ddi = _ddi;
> -
> - return ddi->drv->probe(ddi->dev, ddi->id);
> + struct device *dev = &ddi->dev->dev;
> + struct device_driver *driver = dev->driver;
> + int rc;
> +
> + dev->driver = NULL;
> + rc = pm_runtime_get_sync(dev);
> + dev->driver = driver;
> + if (rc < 0)
> + return rc;
> +
> + rc = ddi->drv->probe(ddi->dev, ddi->id);
> + if (rc) {
> + dev->driver = NULL;
> + pm_runtime_put_sync(dev);
> + dev->driver = driver;
> + }
> + return rc;
> }
>
> static int pci_call_probe(struct pci_driver *drv, struct pci_dev *dev,
> @@ -365,15 +380,24 @@ static int pci_device_probe(struct devic
>
> static int pci_device_remove(struct device * dev)
> {
> + struct device_driver *driver = dev->driver;
> struct pci_dev * pci_dev = to_pci_dev(dev);
> struct pci_driver * drv = pci_dev->driver;
>
> if (drv) {
> - if (drv->remove)
> + if (drv->remove) {
> + pm_runtime_get_sync(dev);
> drv->remove(pci_dev);
> + pm_runtime_put_noidle(dev);
> + }
> pci_dev->driver = NULL;
> }
Is "else" missing here or am I missing something?
> + /* Undo the pm_runtime_get_sync() in local_pci_probe() */
> + dev->driver = NULL;
> + pm_runtime_put_sync(dev);
> + dev->driver = driver;
> +
> /*
> * If the device is still on, set the power state as "unknown",
> * since it might change by the next time we load the driver.
Do PCI bus type callbacks need to be reworked to work correctly with unbound
devices?
Rafael
--
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
