Re: Runtime PM for PCI-based USB host controllers

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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


[Index of Archives]     [DMA Engine]     [Linux Coverity]     [Linux USB]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Greybus]

  Powered by Linux