| Message ID | 20090813003544.GB2532@srcf.ucam.org (mailing list archive) |
|---|---|
| State | RFC, archived |
| Headers | show |
Am Donnerstag, 13. August 2009 02:35:44 schrieb Matthew Garrett: > The power savings from this are measurable but not huge - it still seems How large? > like a decent optimisation. The main problem is that BIOS bugs on some > Dell laptops will kill USB if this is used, so we either default to off > or add some quirks to handle that case (I have some ideas in that > respect). Your earlier failures don't look promising regarding BIOSes. What do you have in mind? > @@ -1968,6 +1972,9 @@ struct usb_hcd *usb_create_hcd (const struct > hc_driver *driver, INIT_WORK(&hcd->wakeup_work, hcd_resume_work); > #endif > > + pm_runtime_enable(dev); So you don't get a reference from that? > + pm_runtime_get(dev); What happens if you get a runtime suspend request in between? Is this a flaw of the API? Regards Oliver -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Aug 13, 2009 at 02:16:41PM +0200, Oliver Neukum wrote: > Am Donnerstag, 13. August 2009 02:35:44 schrieb Matthew Garrett: > > > The power savings from this are measurable but not huge - it still seems > > How large? About 0.2W on an ich9 system. > > like a decent optimisation. The main problem is that BIOS bugs on some > > Dell laptops will kill USB if this is used, so we either default to off > > or add some quirks to handle that case (I have some ideas in that > > respect). > > Your earlier failures don't look promising regarding BIOSes. > What do you have in mind? They range from pragmatic to ugly. We could blacklist all Dells, though I'm trying to find out if there's a BIOS date that guarantees the system is fixed. Alternatively, it's a single-line bug in the DSDT - we could implement some kind of fixup in the ACPI parsing code. I find the latter interesting but possibly too hideous to live :) > > @@ -1968,6 +1972,9 @@ struct usb_hcd *usb_create_hcd (const struct > > hc_driver *driver, INIT_WORK(&hcd->wakeup_work, hcd_resume_work); > > #endif > > > > + pm_runtime_enable(dev); > > So you don't get a reference from that? No, but... > > + pm_runtime_get(dev); > > What happens if you get a runtime suspend request in between? Is this a flaw > of the API? I suspect that just swapping the order of those two lines would be fine.
Am Donnerstag, 13. August 2009 14:30:34 schrieb Matthew Garrett: > > Your earlier failures don't look promising regarding BIOSes. > > What do you have in mind? > > They range from pragmatic to ugly. We could blacklist all Dells, though > I'm trying to find out if there's a BIOS date that guarantees the system > is fixed. Alternatively, it's a single-line bug in the DSDT - we could > implement some kind of fixup in the ACPI parsing code. I find the latter > interesting but possibly too hideous to live :) Is there any indication only those BIOSes are affected? Regards Oliver -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, 13 Aug 2009, Matthew Garrett wrote: > --- a/drivers/usb/core/hcd-pci.c > +++ b/drivers/usb/core/hcd-pci.c > @@ -363,6 +363,18 @@ static int hcd_pci_restore(struct device *dev) > return resume_common(dev, true); > } > > +static int hcd_pci_runtime_suspend(struct device *dev) > +{ > + struct pci_dev *pci_dev = to_pci_dev(dev); > + struct usb_hcd *hcd = pci_get_drvdata(pci_dev); > + struct usb_device *rhdev = hcd->self.root_hub; > + > + if (!(hcd->driver->flags & HCD_RUNTIME_PM)) > + return -EINVAL; > + > + return 0; > +} You have to call the HCD's pci_suspend method! Not to mention calling synchronize_irq and all the other stuff in hcd_pci_suspend and hcd_pci_suspend_noirq. Ditto for resuming. Alan Stern -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Aug 13, 2009 at 04:26:33PM +0200, Oliver Neukum wrote: > Am Donnerstag, 13. August 2009 14:30:34 schrieb Matthew Garrett: > > > Your earlier failures don't look promising regarding BIOSes. > > > What do you have in mind? > > > > They range from pragmatic to ugly. We could blacklist all Dells, though > > I'm trying to find out if there's a BIOS date that guarantees the system > > is fixed. Alternatively, it's a single-line bug in the DSDT - we could > > implement some kind of fixup in the ACPI parsing code. I find the latter > > interesting but possibly too hideous to live :) > > Is there any indication only those BIOSes are affected? Based on what I've looked at, other BIOSes either indicate that they don't support this or should work properly. Real life may disagree.
On Thu, Aug 13, 2009 at 11:22:44AM -0400, Alan Stern wrote: > You have to call the HCD's pci_suspend method! Not to mention calling > synchronize_irq and all the other stuff in hcd_pci_suspend and > hcd_pci_suspend_noirq. The bus level code does this, assuming that the driver-level code doesn't return an error.
diff --git a/drivers/usb/core/hcd-pci.c b/drivers/usb/core/hcd-pci.c index 91f2885..e86324b 100644 --- a/drivers/usb/core/hcd-pci.c +++ b/drivers/usb/core/hcd-pci.c @@ -363,6 +363,18 @@ static int hcd_pci_restore(struct device *dev) return resume_common(dev, true); } +static int hcd_pci_runtime_suspend(struct device *dev) +{ + struct pci_dev *pci_dev = to_pci_dev(dev); + struct usb_hcd *hcd = pci_get_drvdata(pci_dev); + struct usb_device *rhdev = hcd->self.root_hub; + + if (!(hcd->driver->flags & HCD_RUNTIME_PM)) + return -EINVAL; + + return 0; +} + struct dev_pm_ops usb_hcd_pci_pm_ops = { .suspend = hcd_pci_suspend, .suspend_noirq = hcd_pci_suspend_noirq, @@ -376,6 +388,7 @@ struct dev_pm_ops usb_hcd_pci_pm_ops = { .poweroff_noirq = hcd_pci_suspend_noirq, .restore_noirq = hcd_pci_resume_noirq, .restore = hcd_pci_restore, + .runtime_suspend = hcd_pci_runtime_suspend, }; EXPORT_SYMBOL_GPL(usb_hcd_pci_pm_ops); diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c index 95ccfa0..a8f8784 100644 --- a/drivers/usb/core/hcd.c +++ b/drivers/usb/core/hcd.c @@ -38,6 +38,7 @@ #include <asm/unaligned.h> #include <linux/platform_device.h> #include <linux/workqueue.h> +#include <linux/pm_runtime.h> #include <linux/usb.h> @@ -1747,6 +1748,7 @@ int hcd_bus_suspend(struct usb_device *rhdev, pm_message_t msg) if (status == 0) { usb_set_device_state(rhdev, USB_STATE_SUSPENDED); hcd->state = HC_STATE_SUSPENDED; + pm_runtime_put(hcd->self.controller); } else { hcd->state = old_state; dev_dbg(&rhdev->dev, "bus %s fail, err %d\n", @@ -1768,6 +1770,7 @@ int hcd_bus_resume(struct usb_device *rhdev, pm_message_t msg) if (hcd->state == HC_STATE_RUNNING) return 0; + pm_runtime_get_sync(hcd->self.controller); hcd->state = HC_STATE_RESUMING; status = hcd->driver->bus_resume(hcd); if (status == 0) { @@ -1781,6 +1784,7 @@ int hcd_bus_resume(struct usb_device *rhdev, pm_message_t msg) hcd->state = old_state; dev_dbg(&rhdev->dev, "bus %s fail, err %d\n", "resume", status); + pm_runtime_put(hcd->self.controller); if (status != -ESHUTDOWN) usb_hc_died(hcd); } @@ -1968,6 +1972,9 @@ struct usb_hcd *usb_create_hcd (const struct hc_driver *driver, INIT_WORK(&hcd->wakeup_work, hcd_resume_work); #endif + pm_runtime_enable(dev); + pm_runtime_get(dev); + hcd->driver = driver; hcd->product_desc = (driver->product_desc) ? driver->product_desc : "USB Host Controller"; @@ -1979,6 +1986,8 @@ static void hcd_release (struct kref *kref) { struct usb_hcd *hcd = container_of (kref, struct usb_hcd, kref); + pm_runtime_put_sync(hcd->self.controller); + kfree(hcd); } diff --git a/drivers/usb/core/hcd.h b/drivers/usb/core/hcd.h index ec5c67e..4dc12a8 100644 --- a/drivers/usb/core/hcd.h +++ b/drivers/usb/core/hcd.h @@ -171,6 +171,7 @@ struct hc_driver { int flags; #define HCD_MEMORY 0x0001 /* HC regs use memory (else I/O) */ #define HCD_LOCAL_MEM 0x0002 /* HC needs local memory */ +#define HCD_RUNTIME_PM 0x0004 /* HC supports handling runtime PM */ #define HCD_USB11 0x0010 /* USB 1.1 */ #define HCD_USB2 0x0020 /* USB 2.0 */ #define HCD_USB3 0x0040 /* USB 3.0 */ diff --git a/drivers/usb/host/ehci-pci.c b/drivers/usb/host/ehci-pci.c index c2f1b7d..9583621 100644 --- a/drivers/usb/host/ehci-pci.c +++ b/drivers/usb/host/ehci-pci.c @@ -368,7 +368,7 @@ static const struct hc_driver ehci_pci_hc_driver = { * generic hardware linkage */ .irq = ehci_irq, - .flags = HCD_MEMORY | HCD_USB2, + .flags = HCD_MEMORY | HCD_USB2 | HCD_RUNTIME_PM, /* * basic lifecycle operations diff --git a/drivers/usb/host/uhci-hcd.c b/drivers/usb/host/uhci-hcd.c index 274751b..36a3a4a 100644 --- a/drivers/usb/host/uhci-hcd.c +++ b/drivers/usb/host/uhci-hcd.c @@ -900,7 +900,7 @@ static const struct hc_driver uhci_driver = { /* Generic hardware linkage */ .irq = uhci_irq, - .flags = HCD_USB11, + .flags = HCD_USB11 | HCD_RUNTIME_PM, /* Basic lifecycle operations */ .reset = uhci_init,