PM: Introduce core framework for run-time PM of I/O devices (rev. 14)

From: Rafael J. Wysocki <[email protected]>
Subject: PM: Introduce core framework for run-time PM of I/O devices (rev. 15)

Introduce a core framework for run-time power management of I/O devices.  Add device run-time PM fields to 'struct dev_pm_info' and device run-time PM callbacks to 'struct dev_pm_ops'.  Introduce a run-time PM workqueue and define some device run-time PM helper functions at the core level.  Document all these things.

Special thanks to Alan Stern for his help with the design and multiple detailed reviews of the pereceding versions of this patch and to Magnus Damm for testing feedback.

Signed-off-by Rafael J. Wysocki <[email protected]>

include/linux/pm.h +0 -0

 struct dev_pm_info {
       pm_message_t            power_state;
-       unsigned                can_wakeup:1;
-       unsigned                should_wakeup:1;
+       unsigned int            can_wakeup:1;
+       unsigned int            should_wakeup:1;
       enum dpm_state          status;         /* Owned by the PM core */

include/linux/pm.h +1 -1

-#ifdef CONFIG_PM_SLEEP
+#ifdef CONFIG_PM_SLEEP

       struct list_head        entry;
 #endif

+#ifdef CONFIG_PM_RUNTIME
+       struct timer_list       suspend_timer;
+       unsigned long           timer_expires;
+       struct work_struct      work;
+       wait_queue_head_t       wait_queue;
+       spinlock_t              lock;
+       atomic_t                usage_count;
+       atomic_t                child_count;

drivers/base/power/runtime.c new file +0 -0

+int __pm_runtime_suspend(struct device *dev, bool from_wq)
+       __releases(&dev->power.lock) __acquires(&dev->power.lock)

+int __pm_runtime_resume(struct device *dev, bool from_wq)
+       __releases(&dev->power.lock) __acquires(&dev->power.lock)

From: Rafael J. Wysocki <[email protected]>
Subject: PM: Introduce core framework for run-time PM of I/O devices (rev. 15)

Introduce a core framework for run-time power management of I/O devices.  Add device run-time PM fields to 'struct dev_pm_info' and device run-time PM callbacks to 'struct dev_pm_ops'.  Introduce a run-time PM workqueue and define some device run-time PM helper functions at the core level.  Document all these things.

Special thanks to Alan Stern for his help with the design and multiple detailed reviews of the pereceding versions of this patch and to Magnus Damm for testing feedback.

Signed-off-by Rafael J. Wysocki <[email protected]>

include/linux/pm.h +0 -0

 struct dev_pm_info {
       pm_message_t            power_state;
-       unsigned                can_wakeup:1;
-       unsigned                should_wakeup:1;
+       unsigned int            can_wakeup:1;
+       unsigned int            should_wakeup:1;
       enum dpm_state          status;         /* Owned by the PM core */

include/linux/pm.h +1 -1

-#ifdef CONFIG_PM_SLEEP
+#ifdef CONFIG_PM_SLEEP

       struct list_head        entry;
 #endif

+#ifdef CONFIG_PM_RUNTIME
+       struct timer_list       suspend_timer;
+       unsigned long           timer_expires;
+       struct work_struct      work;
+       wait_queue_head_t       wait_queue;
+       spinlock_t              lock;
+       atomic_t                usage_count;
+       atomic_t                child_count;

drivers/base/power/runtime.c new file +0 -0

+int __pm_runtime_suspend(struct device *dev, bool from_wq)
+       __releases(&dev->power.lock) __acquires(&dev->power.lock)

+int __pm_runtime_resume(struct device *dev, bool from_wq)
+       __releases(&dev->power.lock) __acquires(&dev->power.lock)

Quoted 2 lines

drivers/base/power/runtime.c new file +0 -0

[...]

Quoted 2 lines

+int __pm_runtime_suspend(struct device *dev, bool from_wq)
+       __releases(&dev->power.lock) __acquires(&dev->power.lock)

[...]

drivers/base/power/runtime.c line 202

Quoted 10 lines

+       if (dev->bus && dev->bus->pm && dev->bus->pm->runtime_suspend) {
+               spin_unlock_irq(&dev->power.lock);

drivers/base/power/runtime.c new file +8 -0

+
+               retval = dev->bus->pm->runtime_suspend(dev);
+
+               spin_lock_irq(&dev->power.lock);
+               dev->power.runtime_error = retval;
+       } else {
+               retval = -ENOSYS;
+       }

Nit: { and } above do not follow the regular coding style.

Quoted 2 lines

drivers/base/power/runtime.c new file +0 -0

[...]

Quoted 2 lines

+int __pm_runtime_suspend(struct device *dev, bool from_wq)
+       __releases(&dev->power.lock) __acquires(&dev->power.lock)

[...]

drivers/base/power/runtime.c line 202

Quoted 10 lines

+       if (dev->bus && dev->bus->pm && dev->bus->pm->runtime_suspend) {
+               spin_unlock_irq(&dev->power.lock);

drivers/base/power/runtime.c new file +8 -0

+
+               retval = dev->bus->pm->runtime_suspend(dev);
+
+               spin_lock_irq(&dev->power.lock);
+               dev->power.runtime_error = retval;
+       } else {
+               retval = -ENOSYS;
+       }

Nit: { and } above do not follow the regular coding style.

Hi Rafael,

Quoted 9 lines

From: Rafael J. Wysocki <[email protected]>
Subject: PM: Introduce core framework for run-time PM of I/O devices (rev. 15)

Introduce a core framework for run-time power management of I/O devices. Add device run-time PM fields to 'struct dev_pm_info' and device run-time PM callbacks to 'struct dev_pm_ops'. Introduce a run-time PM workqueue and define some device run-time PM helper functions at the core level. Document all these things.

Quoted 5 lines

Special thanks to Alan Stern for his help with the design and multiple detailed reviews of the pereceding versions of this patch and to Magnus Damm for testing feedback.

Signed-off-by Rafael J. Wysocki <[email protected]>

Looking good! I have a few nitpicks below, but from a functional perspective it's all good. I've tested v15 with platform device drivers for I2C, UIO and framebuffer. Before adding my "Acked-by" I also want to test the V4L capture driver, but I need to wait a few days until I can get my hands on such a hardware platform.

Thanks for folding in and fixing up the debug patch. I was able to drop most remaining patches thanks to feedback from Alan. So the only needed patch apart from this one (and the ones in your linux-next branch) is the one in this micro-series: "PM: Runtime PM v15 for Platform Devices 20090812".

Quoted 2 lines

include/linux/pm.h +0 -0

[..]

Quoted 13 lines

 struct dev_pm_info {
       pm_message_t            power_state;

include/linux/pm.h +5 -3

-       unsigned                can_wakeup:1;
-       unsigned                should_wakeup:1;
+       unsigned int            can_wakeup:1;
+       unsigned int            should_wakeup:1;
       enum dpm_state          status;         /* Owned by the PM core */
-#ifdef CONFIG_PM_SLEEP
+#ifdef CONFIG_PM_SLEEP
       struct list_head        entry;
 #endif
+#ifdef CONFIG_PM_RUNTIME
+       struct timer_list       suspend_timer;

include/linux/pm.h enum dpm_state { · line 425

Quoted 6 lines

include/linux/pm.h +6 -0

+       unsigned long           timer_expires;
+       struct work_struct      work;
+       wait_queue_head_t       wait_queue;
+       spinlock_t              lock;
+       atomic_t                usage_count;
+       atomic_t                child_count;

I suppose child_count has to be atomic?

Quoted 2 lines

drivers/base/power/runtime.c new file +0 -0

[...]

Quoted 2 lines

drivers/base/power/runtime.c new file +2 -0

+int __pm_runtime_suspend(struct device *dev, bool from_wq)
+       __releases(&dev->power.lock) __acquires(&dev->power.lock)

[...]

drivers/base/power/runtime.c line 202

Quoted 10 lines

drivers/base/power/runtime.c new file +10 -0

+       if (dev->bus && dev->bus->pm && dev->bus->pm->runtime_suspend) {
+               spin_unlock_irq(&dev->power.lock);
+
+               retval = dev->bus->pm->runtime_suspend(dev);
+
+               spin_lock_irq(&dev->power.lock);
+               dev->power.runtime_error = retval;
+       } else {
+               retval = -ENOSYS;
+       }

Nit: { and } above do not follow the regular coding style.

Hi Rafael,

Quoted 9 lines

From: Rafael J. Wysocki <[email protected]>
Subject: PM: Introduce core framework for run-time PM of I/O devices (rev. 15)

Introduce a core framework for run-time power management of I/O devices. Add device run-time PM fields to 'struct dev_pm_info' and device run-time PM callbacks to 'struct dev_pm_ops'. Introduce a run-time PM workqueue and define some device run-time PM helper functions at the core level. Document all these things.

Quoted 5 lines

Special thanks to Alan Stern for his help with the design and multiple detailed reviews of the pereceding versions of this patch and to Magnus Damm for testing feedback.

Signed-off-by Rafael J. Wysocki <[email protected]>

Looking good! I have a few nitpicks below, but from a functional perspective it's all good. I've tested v15 with platform device drivers for I2C, UIO and framebuffer. Before adding my "Acked-by" I also want to test the V4L capture driver, but I need to wait a few days until I can get my hands on such a hardware platform.

Thanks for folding in and fixing up the debug patch. I was able to drop most remaining patches thanks to feedback from Alan. So the only needed patch apart from this one (and the ones in your linux-next branch) is the one in this micro-series: "PM: Runtime PM v15 for Platform Devices 20090812".

Quoted 2 lines

include/linux/pm.h +0 -0

[..]

Quoted 13 lines

 struct dev_pm_info {
       pm_message_t            power_state;

include/linux/pm.h +5 -3

-       unsigned                can_wakeup:1;
-       unsigned                should_wakeup:1;
+       unsigned int            can_wakeup:1;
+       unsigned int            should_wakeup:1;
       enum dpm_state          status;         /* Owned by the PM core */
-#ifdef CONFIG_PM_SLEEP
+#ifdef CONFIG_PM_SLEEP
       struct list_head        entry;
 #endif
+#ifdef CONFIG_PM_RUNTIME
+       struct timer_list       suspend_timer;

include/linux/pm.h enum dpm_state { · line 425

Quoted 6 lines

include/linux/pm.h +6 -0

+       unsigned long           timer_expires;
+       struct work_struct      work;
+       wait_queue_head_t       wait_queue;
+       spinlock_t              lock;
+       atomic_t                usage_count;
+       atomic_t                child_count;

I suppose child_count has to be atomic?

Quoted 2 lines

drivers/base/power/runtime.c new file +0 -0

[...]

Quoted 2 lines

drivers/base/power/runtime.c new file +2 -0

+int __pm_runtime_suspend(struct device *dev, bool from_wq)
+       __releases(&dev->power.lock) __acquires(&dev->power.lock)

[...]

drivers/base/power/runtime.c line 202

Quoted 10 lines

drivers/base/power/runtime.c new file +10 -0

+       if (dev->bus && dev->bus->pm && dev->bus->pm->runtime_suspend) {
+               spin_unlock_irq(&dev->power.lock);
+
+               retval = dev->bus->pm->runtime_suspend(dev);
+
+               spin_lock_irq(&dev->power.lock);
+               dev->power.runtime_error = retval;
+       } else {
+               retval = -ENOSYS;
+       }

Nit: { and } above do not follow the regular coding style.

The power savings from this are measurable but not huge - it still seems

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).

Am Donnerstag, 13. August 2009 02:35:44 schrieb Matthew Garrett:

Quoted 1 line

The power savings from this are measurable but not huge - it still seems

How large?

Quoted 4 lines

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?

drivers/usb/core/hcd.c struct usb_hcd *usb_create_hcd (const struct · line 1975

Quoted 5 lines

drivers/usb/core/hcd.c +1 -0

@@ -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?

drivers/usb/core/hcd.c struct usb_hcd *usb_create_hcd (const struct hc_driver *driver, · line 1976

Quoted 1 line

drivers/usb/core/hcd.c +1 -0

+	pm_runtime_get(dev);

What happens if you get a runtime suspend request in between? Is this a flaw of the API?

Am Donnerstag, 13. August 2009 02:35:44 schrieb Matthew Garrett:

Quoted 1 line

The power savings from this are measurable but not huge - it still seems

How large?

Quoted 4 lines

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?

drivers/usb/core/hcd.c struct usb_hcd *usb_create_hcd (const struct · line 1975

Quoted 5 lines

drivers/usb/core/hcd.c +1 -0

@@ -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?

drivers/usb/core/hcd.c struct usb_hcd *usb_create_hcd (const struct hc_driver *driver, · line 1976

Quoted 1 line

drivers/usb/core/hcd.c +1 -0

+	pm_runtime_get(dev);

What happens if you get a runtime suspend request in between? Is this a flaw of the API?

Quoted 2 lines

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 :)

Am Donnerstag, 13. August 2009 14:30:34 schrieb Matthew Garrett:

Quoted 8 lines

Quoted 2 lines

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?

Am Donnerstag, 13. August 2009 14:30:34 schrieb Matthew Garrett:

Quoted 8 lines

Quoted 2 lines

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?

Quoted 2 lines

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 :)

The power savings from this are measurable but not huge - it still seems

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).

drivers/usb/core/hcd-pci.c +1 -0

@@ -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)

drivers/usb/core/hcd-pci.c +1 -0

@@ -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)

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.

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.

drivers/pci/pci-driver.c +25 -0

+	error = pci_enable_runtime_wake(pci_dev, true);
+
+	if (error)
+		return -EBUSY;
+
+	if (pm && pm->runtime_suspend)
+		error = pm->runtime_suspend(dev);
+
+	if (error)
+		goto out;
+
+	error = pci_pm_suspend(dev);
+
+	if (error)
+		goto resume;
+
+	disable_irq(pci_dev->irq);
+	error = pci_pm_suspend_noirq(dev);
+	enable_irq(pci_dev->irq);
+
+	if (error)
+		goto resume_noirq;
+
+	return 0;
+

drivers/pci/pci-driver.c +21 -0

+static int pci_pm_runtime_resume(struct device *dev)
+{
+	struct pci_dev *pci_dev = to_pci_dev(dev);
+	struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
+	int error = 0;
+
+	disable_irq(pci_dev->irq);
+	error = pci_pm_resume_noirq(dev);
+	enable_irq(pci_dev->irq);
+
+	if (error)
+		return error;
+
+	error = pci_pm_resume(dev);
+
+	if (error)
+		return error;
+
+	if (pm->runtime_resume)
+		error = pm->runtime_resume(dev);
+

drivers/pci/pci-driver.c +25 -0

+	error = pci_enable_runtime_wake(pci_dev, true);
+
+	if (error)
+		return -EBUSY;
+
+	if (pm && pm->runtime_suspend)
+		error = pm->runtime_suspend(dev);
+
+	if (error)
+		goto out;
+
+	error = pci_pm_suspend(dev);
+
+	if (error)
+		goto resume;
+
+	disable_irq(pci_dev->irq);
+	error = pci_pm_suspend_noirq(dev);
+	enable_irq(pci_dev->irq);
+
+	if (error)
+		goto resume_noirq;
+
+	return 0;
+

drivers/pci/pci-driver.c +21 -0

+static int pci_pm_runtime_resume(struct device *dev)
+{
+	struct pci_dev *pci_dev = to_pci_dev(dev);
+	struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
+	int error = 0;
+
+	disable_irq(pci_dev->irq);
+	error = pci_pm_resume_noirq(dev);
+	enable_irq(pci_dev->irq);
+
+	if (error)
+		return error;
+
+	error = pci_pm_resume(dev);
+
+	if (error)
+		return error;
+
+	if (pm->runtime_resume)
+		error = pm->runtime_resume(dev);
+

Quoted 2 lines · Matthew Garrett

drivers/pci/pci-driver.c +0 -0

drivers/pci/pci-driver.c static int pci_pm_restore(struct device *dev) · line 923

Quoted 9 lines

drivers/pci/pci-driver.c +9 -0

+#ifdef CONFIG_PM_RUNTIME
+
+static int pci_pm_runtime_suspend(struct device *dev)
+{
+	struct pci_dev *pci_dev = to_pci_dev(dev);
+	struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
+	int error;
+
+	device_set_wakeup_enable(dev, 1);

This is a userspace policy parameter. Kernel code should not alter it. Instead you should test device_may_wakeup.

drivers/pci/pci-driver.c static int pci_pm_restore(struct device *dev) · line 958

Quoted 7 lines

drivers/pci/pci-driver.c +7 -0

+	enable_irq(pci_dev->irq);
+resume:
+	pci_pm_resume(dev);
+out:
+	pci_enable_runtime_wake(pci_dev, false);
+	return error;
+}

The goto statements and unwinding code don't match up.

drivers/pci/pci-driver.c static int pci_pm_restore(struct device *dev) · line 990

Quoted 5 lines

drivers/pci/pci-driver.c +5 -0

+	if (error)
+		return error;
+
+	return 0;
+}

Log an error message when something goes wrong?

Quoted 6 lines

drivers/pci/pci-driver.c +6 -0

+static void pci_pm_runtime_idle(struct device *dev)
+{
+	struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
+
+	if (pm && pm->runtime_idle)
+		pm->runtime_idle(dev);

drivers/pci/pci-driver.c static int pci_pm_restore(struct device *dev) · line 1000

Quoted 3 lines

drivers/pci/pci-driver.c +3 -0

+
+	pm_schedule_suspend(dev, 0);
+}

This misses the point. The whole idea of runtime_idle is to tell you that the device is idle and might be ready to be suspended. If you're going to call pm_schedule_suspend anyway, there's no reason to invoke pm->runtime_idle.

Quoted 2 lines · Matthew Garrett

drivers/pci/pci-driver.c +0 -0

drivers/pci/pci-driver.c static int pci_pm_restore(struct device *dev) · line 923

Quoted 9 lines

drivers/pci/pci-driver.c +9 -0

+#ifdef CONFIG_PM_RUNTIME
+
+static int pci_pm_runtime_suspend(struct device *dev)
+{
+	struct pci_dev *pci_dev = to_pci_dev(dev);
+	struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
+	int error;
+
+	device_set_wakeup_enable(dev, 1);

This is a userspace policy parameter. Kernel code should not alter it. Instead you should test device_may_wakeup.

drivers/pci/pci-driver.c static int pci_pm_restore(struct device *dev) · line 958

Quoted 7 lines

drivers/pci/pci-driver.c +7 -0

+	enable_irq(pci_dev->irq);
+resume:
+	pci_pm_resume(dev);
+out:
+	pci_enable_runtime_wake(pci_dev, false);
+	return error;
+}

The goto statements and unwinding code don't match up.

drivers/pci/pci-driver.c static int pci_pm_restore(struct device *dev) · line 990

Quoted 5 lines

drivers/pci/pci-driver.c +5 -0

+	if (error)
+		return error;
+
+	return 0;
+}

Log an error message when something goes wrong?

Quoted 6 lines

drivers/pci/pci-driver.c +6 -0

+static void pci_pm_runtime_idle(struct device *dev)
+{
+	struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
+
+	if (pm && pm->runtime_idle)
+		pm->runtime_idle(dev);

drivers/pci/pci-driver.c static int pci_pm_restore(struct device *dev) · line 1000

Quoted 3 lines

drivers/pci/pci-driver.c +3 -0

+
+	pm_schedule_suspend(dev, 0);
+}

This misses the point. The whole idea of runtime_idle is to tell you that the device is idle and might be ready to be suspended. If you're going to call pm_schedule_suspend anyway, there's no reason to invoke pm->runtime_idle.

Ugh. I'd really prefer us to assume that drivers are able to cope unless proven otherwise. Userspace policy makes sense where we don't have any idea whether something will work or not, but I'd really expect that most PCI drivers will either cope (in which case they'll have enabling code) or won't (in which case they won't). Why would we want userspace to influence this?

Quoted 4 lines

This misses the point. The whole idea of runtime_idle is to tell you that the device is idle and might be ready to be suspended. If you're going to call pm_schedule_suspend anyway, there's no reason to invoke pm->runtime_idle.

My understanding of the API was that pm_device_put() invokes runtime_idle if the refcount hits 0. The bus layer has no idea of the refcount, and calling suspend directly from the driver would defeat the point of the system-wide recounting.

From the API docs:

Ugh. I'd really prefer us to assume that drivers are able to cope unless proven otherwise. Userspace policy makes sense where we don't have any idea whether something will work or not, but I'd really expect that most PCI drivers will either cope (in which case they'll have enabling code) or won't (in which case they won't). Why would we want userspace to influence this?

Quoted 4 lines

This misses the point. The whole idea of runtime_idle is to tell you that the device is idle and might be ready to be suspended. If you're going to call pm_schedule_suspend anyway, there's no reason to invoke pm->runtime_idle.

My understanding of the API was that pm_device_put() invokes runtime_idle if the refcount hits 0. The bus layer has no idea of the refcount, and calling suspend directly from the driver would defeat the point of the system-wide recounting.

From the API docs:

Quoted 3 lines · Alan Stern

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.

Quoted 6 lines · Matthew Garrett

Ugh. I'd really prefer us to assume that drivers are able to cope unless proven otherwise. Userspace policy makes sense where we don't have any idea whether something will work or not, but I'd really expect that most PCI drivers will either cope (in which case they'll have enabling code) or won't (in which case they won't). Why would we want userspace to influence this?

Though, thinking about it, you're right that setting this does override user policy. I think we need an additional flag to indicate that the device supports runtime wakeup and test that as well when doing device_may_wakeup().

Quoted 9 lines

Quoted 4 lines

This misses the point. The whole idea of runtime_idle is to tell you that the device is idle and might be ready to be suspended. If you're going to call pm_schedule_suspend anyway, there's no reason to invoke pm->runtime_idle.

My understanding of the API was that pm_device_put() invokes runtime_idle if the refcount hits 0. The bus layer has no idea of the refcount, and calling suspend directly from the driver would defeat the point of the system-wide recounting.

From the API docs:

"The action performed by a bus type's ->runtime_idle() callback is totally dependent on the bus type in question, but the expected and recommended action is to check if the device can be suspended (i.e. if all of the conditions necessary for suspending the device are satisfied) and to queue up a suspend request for the device in that case."

Though perhaps the device level runtime_idle shouldn't be void - that way the bus can ask the driver whether its suspend conditions have been satisfied? Right now there doesn't seem to be any way for the bus to ask that.

Quoted 3 lines · Alan Stern

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.

Quoted 6 lines · Matthew Garrett

Ugh. I'd really prefer us to assume that drivers are able to cope unless proven otherwise. Userspace policy makes sense where we don't have any idea whether something will work or not, but I'd really expect that most PCI drivers will either cope (in which case they'll have enabling code) or won't (in which case they won't). Why would we want userspace to influence this?

Though, thinking about it, you're right that setting this does override user policy. I think we need an additional flag to indicate that the device supports runtime wakeup and test that as well when doing device_may_wakeup().

Quoted 9 lines

Quoted 4 lines

This misses the point. The whole idea of runtime_idle is to tell you that the device is idle and might be ready to be suspended. If you're going to call pm_schedule_suspend anyway, there's no reason to invoke pm->runtime_idle.

My understanding of the API was that pm_device_put() invokes runtime_idle if the refcount hits 0. The bus layer has no idea of the refcount, and calling suspend directly from the driver would defeat the point of the system-wide recounting.

From the API docs:

"The action performed by a bus type's ->runtime_idle() callback is totally dependent on the bus type in question, but the expected and recommended action is to check if the device can be suspended (i.e. if all of the conditions necessary for suspending the device are satisfied) and to queue up a suspend request for the device in that case."

Though perhaps the device level runtime_idle shouldn't be void - that way the bus can ask the driver whether its suspend conditions have been satisfied? Right now there doesn't seem to be any way for the bus to ask that.

Quoted 8 lines · Matthew Garrett

Quoted 3 lines · Alan Stern

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.

So it does; my mistake.

Quoted 9 lines · Matthew Garrett

Quoted 6 lines · Matthew Garrett

Ugh. I'd really prefer us to assume that drivers are able to cope unless proven otherwise. Userspace policy makes sense where we don't have any idea whether something will work or not, but I'd really expect that most PCI drivers will either cope (in which case they'll have enabling code) or won't (in which case they won't). Why would we want userspace to influence this?

Quoted 4 lines

Though, thinking about it, you're right that setting this does override user policy. I think we need an additional flag to indicate that the device supports runtime wakeup and test that as well when doing device_may_wakeup().

You are suggesting separate flag sets for system-wide wakeup and runtime wakeup? I don't disagree, but implementing them will be problematical.

That's because it's not always possible to change a device's wakeup setting while it is suspended. Thus if a device was runtime suspended with wakeup enabled, and then we want to do a system sleep and change the device's wakeup setting to disabled, we would have to wake the device back up in order to do it.

Quoted 18 lines

Quoted 9 lines

Quoted 4 lines

This misses the point. The whole idea of runtime_idle is to tell you that the device is idle and might be ready to be suspended. If you're going to call pm_schedule_suspend anyway, there's no reason to invoke pm->runtime_idle.

My understanding of the API was that pm_device_put() invokes runtime_idle if the refcount hits 0. The bus layer has no idea of the refcount, and calling suspend directly from the driver would defeat the point of the system-wide recounting.

From the API docs:

"The action performed by a bus type's ->runtime_idle() callback is totally dependent on the bus type in question, but the expected and recommended action is to check if the device can be suspended (i.e. if all of the conditions necessary for suspending the device are satisfied) and to queue up a suspend request for the device in that case."

Quoted 4 lines

Though perhaps the device level runtime_idle shouldn't be void - that way the bus can ask the driver whether its suspend conditions have been satisfied? Right now there doesn't seem to be any way for the bus to ask that.

If you want to get the device-level runtime_idle involved, you can make _it_ responsible for scheduling the suspend. Then the bus-level code simply has to check whether everything is okay at the bus level, and if it is, call the device-level routine.

However changing the return type wouldn't hurt anything, and it would allow the pm_schedule_suspend call to be centralized in the bus code. You could ask Rafael about it, or just send him a patch.

Quoted 3 lines · Alan Stern

Quoted 1 line · Matthew Garrett

On Thu, Aug 13, 2009 at 11:22:44AM -0400, Alan Stern wrote:

Quoted 10 lines

Quoted 4 lines

Though perhaps the device level runtime_idle shouldn't be void - that way the bus can ask the driver whether its suspend conditions have been satisfied? Right now there doesn't seem to be any way for the bus to ask that.

If you want to get the device-level runtime_idle involved, you can make _it_ responsible for scheduling the suspend. Then the bus-level code simply has to check whether everything is okay at the bus level, and if it is, call the device-level routine.

Quoted 3 lines

However changing the return type wouldn't hurt anything, and it would allow the pm_schedule_suspend call to be centralized in the bus code. You could ask Rafael about it, or just send him a patch.

Well, I'm not against that, but what should pm_runtime_idle() do with the result returned by it? Just pass it to the caller?

Quoted 4 lines · Rafael J. Wysocki

Quoted 3 lines · Alan Stern

Quoted 1 line · Matthew Garrett

On Thu, Aug 13, 2009 at 11:22:44AM -0400, Alan Stern wrote:

...

Quoted 10 lines

Quoted 10 lines

Quoted 4 lines

Though perhaps the device level runtime_idle shouldn't be void - that way the bus can ask the driver whether its suspend conditions have been satisfied? Right now there doesn't seem to be any way for the bus to ask that.

If you want to get the device-level runtime_idle involved, you can make _it_ responsible for scheduling the suspend. Then the bus-level code simply has to check whether everything is okay at the bus level, and if it is, call the device-level routine.

Quoted 6 lines

Quoted 3 lines

However changing the return type wouldn't hurt anything, and it would allow the pm_schedule_suspend call to be centralized in the bus code. You could ask Rafael about it, or just send him a patch.

Well, I'm not against that, but what should pm_runtime_idle() do with the result returned by it? Just pass it to the caller?

Hm, perhaps its better to ignore it, though.

Quoted 4 lines · Rafael J. Wysocki

Quoted 3 lines · Alan Stern

Quoted 1 line · Matthew Garrett

On Thu, Aug 13, 2009 at 11:22:44AM -0400, Alan Stern wrote:

...

Quoted 10 lines

Quoted 10 lines

Quoted 4 lines

Though perhaps the device level runtime_idle shouldn't be void - that way the bus can ask the driver whether its suspend conditions have been satisfied? Right now there doesn't seem to be any way for the bus to ask that.

If you want to get the device-level runtime_idle involved, you can make _it_ responsible for scheduling the suspend. Then the bus-level code simply has to check whether everything is okay at the bus level, and if it is, call the device-level routine.

Quoted 6 lines

Quoted 3 lines

However changing the return type wouldn't hurt anything, and it would allow the pm_schedule_suspend call to be centralized in the bus code. You could ask Rafael about it, or just send him a patch.

Well, I'm not against that, but what should pm_runtime_idle() do with the result returned by it? Just pass it to the caller?

Hm, perhaps its better to ignore it, though.

Quoted 3 lines · Alan Stern

Quoted 1 line · Matthew Garrett

On Thu, Aug 13, 2009 at 11:22:44AM -0400, Alan Stern wrote:

Quoted 10 lines

Quoted 4 lines

Though perhaps the device level runtime_idle shouldn't be void - that way the bus can ask the driver whether its suspend conditions have been satisfied? Right now there doesn't seem to be any way for the bus to ask that.

If you want to get the device-level runtime_idle involved, you can make _it_ responsible for scheduling the suspend. Then the bus-level code simply has to check whether everything is okay at the bus level, and if it is, call the device-level routine.

Quoted 3 lines

However changing the return type wouldn't hurt anything, and it would allow the pm_schedule_suspend call to be centralized in the bus code. You could ask Rafael about it, or just send him a patch.

Well, I'm not against that, but what should pm_runtime_idle() do with the result returned by it? Just pass it to the caller?

Quoted 8 lines · Matthew Garrett

Quoted 3 lines · Alan Stern

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.

So it does; my mistake.

Quoted 9 lines · Matthew Garrett

Quoted 6 lines · Matthew Garrett

Ugh. I'd really prefer us to assume that drivers are able to cope unless proven otherwise. Userspace policy makes sense where we don't have any idea whether something will work or not, but I'd really expect that most PCI drivers will either cope (in which case they'll have enabling code) or won't (in which case they won't). Why would we want userspace to influence this?

Quoted 4 lines

Though, thinking about it, you're right that setting this does override user policy. I think we need an additional flag to indicate that the device supports runtime wakeup and test that as well when doing device_may_wakeup().

You are suggesting separate flag sets for system-wide wakeup and runtime wakeup? I don't disagree, but implementing them will be problematical.

That's because it's not always possible to change a device's wakeup setting while it is suspended. Thus if a device was runtime suspended with wakeup enabled, and then we want to do a system sleep and change the device's wakeup setting to disabled, we would have to wake the device back up in order to do it.

Quoted 18 lines

Quoted 9 lines

Quoted 4 lines

This misses the point. The whole idea of runtime_idle is to tell you that the device is idle and might be ready to be suspended. If you're going to call pm_schedule_suspend anyway, there's no reason to invoke pm->runtime_idle.

My understanding of the API was that pm_device_put() invokes runtime_idle if the refcount hits 0. The bus layer has no idea of the refcount, and calling suspend directly from the driver would defeat the point of the system-wide recounting.

From the API docs:

"The action performed by a bus type's ->runtime_idle() callback is totally dependent on the bus type in question, but the expected and recommended action is to check if the device can be suspended (i.e. if all of the conditions necessary for suspending the device are satisfied) and to queue up a suspend request for the device in that case."

Quoted 4 lines

Though perhaps the device level runtime_idle shouldn't be void - that way the bus can ask the driver whether its suspend conditions have been satisfied? Right now there doesn't seem to be any way for the bus to ask that.

If you want to get the device-level runtime_idle involved, you can make _it_ responsible for scheduling the suspend. Then the bus-level code simply has to check whether everything is okay at the bus level, and if it is, call the device-level routine.

However changing the return type wouldn't hurt anything, and it would allow the pm_schedule_suspend call to be centralized in the bus code. You could ask Rafael about it, or just send him a patch.

Quoted 8 lines · Matthew Garrett

Quoted 3 lines · Alan Stern

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.

So it does; my mistake.

Quoted 9 lines · Matthew Garrett

Quoted 6 lines · Matthew Garrett

Ugh. I'd really prefer us to assume that drivers are able to cope unless proven otherwise. Userspace policy makes sense where we don't have any idea whether something will work or not, but I'd really expect that most PCI drivers will either cope (in which case they'll have enabling code) or won't (in which case they won't). Why would we want userspace to influence this?

Quoted 4 lines

Though, thinking about it, you're right that setting this does override user policy. I think we need an additional flag to indicate that the device supports runtime wakeup and test that as well when doing device_may_wakeup().

You are suggesting separate flag sets for system-wide wakeup and runtime wakeup? I don't disagree, but implementing them will be problematical.

That's because it's not always possible to change a device's wakeup setting while it is suspended. Thus if a device was runtime suspended with wakeup enabled, and then we want to do a system sleep and change the device's wakeup setting to disabled, we would have to wake the device back up in order to do it.

Well, sometimes the user may want a device to be power managed at run time and not to be able to wake up the system from sleep states. For example, I'd like the USB controller in my box to be suspended at run time whenever it's not used, but surely I wouldn't like it to do system-wide wakeup, because it does that when I move the mouse which is a cordless one. Simply turning the mouse on causes the system to wake up. :-)

Why don't we add a flag indicating whether or not the device is allowed to be power managed at run time, something like runtime_forbidden, that the user space will be able to set through sysfs?

Well, sometimes the user may want a device to be power managed at run time and not to be able to wake up the system from sleep states. For example, I'd like the USB controller in my box to be suspended at run time whenever it's not used, but surely I wouldn't like it to do system-wide wakeup, because it does that when I move the mouse which is a cordless one. Simply turning the mouse on causes the system to wake up. :-)

Why don't we add a flag indicating whether or not the device is allowed to be power managed at run time, something like runtime_forbidden, that the user space will be able to set through sysfs?

Quoted 6 lines · Rafael J. Wysocki

Well, sometimes the user may want a device to be power managed at run time and not to be able to wake up the system from sleep states. For example, I'd like the USB controller in my box to be suspended at run time whenever it's not used, but surely I wouldn't like it to do system-wide wakeup, because it does that when I move the mouse which is a cordless one. Simply turning the mouse on causes the system to wake up. :-)

Right, so clearly my code is broken right now.

Quoted 3 lines

Why don't we add a flag indicating whether or not the device is allowed to be power managed at run time, something like runtime_forbidden, that the user space will be able to set through sysfs?

I think even having a runtime_wakeup flag (which defaults to on) would be sufficient.

If the worst case is scenario is that we have to resume devices in order to apply the correct policy when going into a systemwide suspend state, I think that's acceptable.

Quoted 4 lines

Quoted 3 lines

Why don't we add a flag indicating whether or not the device is allowed to be power managed at run time, something like runtime_forbidden, that the user space will be able to set through sysfs?

Quoted 2 lines

I think even having a runtime_wakeup flag (which defaults to on) would be sufficient.

Perhaps it would, but then unsetting runtime_wakeup would effectively disable runtime PM for devices that need it to be power managed at run time (probably all input devices). Also there may be situations in which user space may really want to disable runtime PM for some devices (think of broken hardware for one example).

Quoted 4 lines

Quoted 3 lines

Why don't we add a flag indicating whether or not the device is allowed to be power managed at run time, something like runtime_forbidden, that the user space will be able to set through sysfs?

Quoted 2 lines

I think even having a runtime_wakeup flag (which defaults to on) would be sufficient.

Perhaps it would, but then unsetting runtime_wakeup would effectively disable runtime PM for devices that need it to be power managed at run time (probably all input devices). Also there may be situations in which user space may really want to disable runtime PM for some devices (think of broken hardware for one example).

Quoted 4 lines · Rafael J. Wysocki

Quoted 4 lines

Quoted 3 lines

Why don't we add a flag indicating whether or not the device is allowed to be power managed at run time, something like runtime_forbidden, that the user space will be able to set through sysfs?

Quoted 8 lines

Quoted 2 lines

I think even having a runtime_wakeup flag (which defaults to on) would be sufficient.

Perhaps it would, but then unsetting runtime_wakeup would effectively disable runtime PM for devices that need it to be power managed at run time (probably all input devices). Also there may be situations in which user space may really want to disable runtime PM for some devices (think of broken hardware for one example).

It sounds like there are really three choices here, and the decision should largely be left up to the user:

1. don't use runtime PM,

2. allow runtime PM but disable remote wakeup,

3. allow runtime PM with remote wakeup enabled.

Now, a driver may say "I can't do my job without remote wakeup". Such a driver would refuse to do runtime_suspend in case 2. But otherwise we should follow the preference of the user.

The only remaining question is how to expose this in sysfs in a way that won't be confusing and that won't be confused with the "wakeup" attribute. One possibility is to use the "level" attribute introduced in USB; possible levels are "on" (no runtime PM) and "auto" (runtime PM allowed). Then a new "runtime_wakeup" attribute could contain nothing (if wakeup is not available), "enabled", or "disabled".

runtime_wakeup - if set, the device is allowed to do remote wakeup at run time
That could be represented as 'runtime_wakeup' under 'power' with the following values:
* no value (empty file) is 'runtime' is 'disabled'
* 'enabled'
* 'disabled'
To set/unset the user space writes 'enabled'/'disabled' to it, respectively. The default is set.

runtime_wakeup - if set, the device is allowed to do remote wakeup at run time
That could be represented as 'runtime_wakeup' under 'power' with the following values:
* no value (empty file) is 'runtime' is 'disabled'
* 'enabled'
* 'disabled'
To set/unset the user space writes 'enabled'/'disabled' to it, respectively. The default is set.

Quoted 8 lines · Rafael J. Wysocki

runtime_wakeup - if set, the device is allowed to do remote wakeup at run time
That could be represented as 'runtime_wakeup' under 'power' with the following values:
* no value (empty file) is 'runtime' is 'disabled'
* 'enabled'
* 'disabled'
To set/unset the user space writes 'enabled'/'disabled' to it, respectively. The default is set.

Why would you ever want runtime_wakeup to be false unless runtime_forbidden is true? Surely the point of runtime power management is to be transparent to the user, in which case remote wakeup is required?

I can even imagine a scenario where this setting might be useful, like when we don't want a network adapter to be woken up from the outside.

I can even imagine a scenario where this setting might be useful, like when we don't want a network adapter to be woken up from the outside.

Quoted 2 lines · Rafael J. Wysocki

I can even imagine a scenario where this setting might be useful, like when we don't want a network adapter to be woken up from the outside.

I think in that case we'd probably just want the interface to be downed? Some of this is going to require device-specific policy, I think - for the network case we probably want something in between IF_RUNNING and IF_DOWN (IF_CARRIER, perhaps) that indicates that we want the PHY to be powered. Pushing this out to sysfs would mean we'd have a consistent interface but varying semantics, and I'm not convinced that's an especially helpful interface.

Quoted 5 lines · Matthew Garrett

Quoted 2 lines · Rafael J. Wysocki

I can even imagine a scenario where this setting might be useful, like when we don't want a network adapter to be woken up from the outside.

Quoted 7 lines

I think in that case we'd probably just want the interface to be downed? Some of this is going to require device-specific policy, I think - for the network case we probably want something in between IF_RUNNING and IF_DOWN (IF_CARRIER, perhaps) that indicates that we want the PHY to be powered. Pushing this out to sysfs would mean we'd have a consistent interface but varying semantics, and I'm not convinced that's an especially helpful interface.

I'm not disagreeing with that.

At this point I'd like to know the Alan's opinion. I would gladly use the 'runtime_forbidden' flag only, but if we overlook something now, it's going to be difficult to fix later.

Quoted 5 lines · Matthew Garrett

Quoted 2 lines · Rafael J. Wysocki

I can even imagine a scenario where this setting might be useful, like when we don't want a network adapter to be woken up from the outside.

Quoted 7 lines

I think in that case we'd probably just want the interface to be downed? Some of this is going to require device-specific policy, I think - for the network case we probably want something in between IF_RUNNING and IF_DOWN (IF_CARRIER, perhaps) that indicates that we want the PHY to be powered. Pushing this out to sysfs would mean we'd have a consistent interface but varying semantics, and I'm not convinced that's an especially helpful interface.

I'm not disagreeing with that.

At this point I'd like to know the Alan's opinion. I would gladly use the 'runtime_forbidden' flag only, but if we overlook something now, it's going to be difficult to fix later.

Quoted 2 lines · Rafael J. Wysocki

I can even imagine a scenario where this setting might be useful, like when we don't want a network adapter to be woken up from the outside.

I think in that case we'd probably just want the interface to be downed? Some of this is going to require device-specific policy, I think - for the network case we probably want something in between IF_RUNNING and IF_DOWN (IF_CARRIER, perhaps) that indicates that we want the PHY to be powered. Pushing this out to sysfs would mean we'd have a consistent interface but varying semantics, and I'm not convinced that's an especially helpful interface.

Quoted 4 lines

Why would you ever want runtime_wakeup to be false unless runtime_forbidden is true? Surely the point of runtime power management is to be transparent to the user, in which case remote wakeup is required?

Well, this was exactly my point previously. :-)

Still, although for the majority of devices 'runtime_wakeup' disabled would mean no runtime PM at all IMO, there may be devices that actually work without remote wakeup, although they support it in general.

I can even imagine a scenario where this setting might be useful, like when we don't want a network adapter to be woken up from the outside.

Quoted 4 lines

Why would you ever want runtime_wakeup to be false unless runtime_forbidden is true? Surely the point of runtime power management is to be transparent to the user, in which case remote wakeup is required?

Well, this was exactly my point previously. :-)

Still, although for the majority of devices 'runtime_wakeup' disabled would mean no runtime PM at all IMO, there may be devices that actually work without remote wakeup, although they support it in general.

I can even imagine a scenario where this setting might be useful, like when we don't want a network adapter to be woken up from the outside.

Quoted 4 lines · Rafael J. Wysocki

Quoted 4 lines

Why would you ever want runtime_wakeup to be false unless runtime_forbidden is true? Surely the point of runtime power management is to be transparent to the user, in which case remote wakeup is required?

Matthew, what makes you think remote wakeup is required? Lots of power-manageable devices don't support it at all (consider disk drives or display screens).

Quoted 4 lines · Rafael J. Wysocki

Quoted 4 lines

Why would you ever want runtime_wakeup to be false unless runtime_forbidden is true? Surely the point of runtime power management is to be transparent to the user, in which case remote wakeup is required?

Matthew, what makes you think remote wakeup is required? Lots of power-manageable devices don't support it at all (consider disk drives or display screens).

Quoted 6 lines

The only remaining question is how to expose this in sysfs in a way that won't be confusing and that won't be confused with the "wakeup" attribute. One possibility is to use the "level" attribute introduced in USB; possible levels are "on" (no runtime PM) and "auto" (runtime PM allowed). Then a new "runtime_wakeup" attribute could contain nothing (if wakeup is not available), "enabled", or "disabled".

That seems to require two flags.

runtime_forbidden - if unset, the driver decides whether or not to use runtime
PM; that could be exposed through sysfs as 'runtime' under the 'power' subdirectory with the following values:
* 'disabled' - runtime_forbidden is set by the user space
* 'on' - runtime_forbidden is unset, runtime PM is used (disable_depth == 0)
* 'off' - runtime_forbidden is unset, runtime PM is not used
To set/unset the user space writes 'enabled'/'disabled' to it, respectively. The default is unset.

runtime_wakeup - if set, the device is allowed to do remote wakeup at run time
That could be represented as 'runtime_wakeup' under 'power' with the following values:
* no value (empty file) is 'runtime' is 'disabled'
* 'enabled'
* 'disabled'
To set/unset the user space writes 'enabled'/'disabled' to it, respectively. The default is set.

Quoted 6 lines

The only remaining question is how to expose this in sysfs in a way that won't be confusing and that won't be confused with the "wakeup" attribute. One possibility is to use the "level" attribute introduced in USB; possible levels are "on" (no runtime PM) and "auto" (runtime PM allowed). Then a new "runtime_wakeup" attribute could contain nothing (if wakeup is not available), "enabled", or "disabled".

That seems to require two flags.

runtime_forbidden - if unset, the driver decides whether or not to use runtime
PM; that could be exposed through sysfs as 'runtime' under the 'power' subdirectory with the following values:
* 'disabled' - runtime_forbidden is set by the user space
* 'on' - runtime_forbidden is unset, runtime PM is used (disable_depth == 0)
* 'off' - runtime_forbidden is unset, runtime PM is not used
To set/unset the user space writes 'enabled'/'disabled' to it, respectively. The default is unset.

runtime_wakeup - if set, the device is allowed to do remote wakeup at run time
That could be represented as 'runtime_wakeup' under 'power' with the following values:
* no value (empty file) is 'runtime' is 'disabled'
* 'enabled'
* 'disabled'
To set/unset the user space writes 'enabled'/'disabled' to it, respectively. The default is set.

Quoted 4 lines · Rafael J. Wysocki

Quoted 4 lines

Quoted 3 lines

Why don't we add a flag indicating whether or not the device is allowed to be power managed at run time, something like runtime_forbidden, that the user space will be able to set through sysfs?

Quoted 8 lines

Quoted 2 lines

I think even having a runtime_wakeup flag (which defaults to on) would be sufficient.

Perhaps it would, but then unsetting runtime_wakeup would effectively disable runtime PM for devices that need it to be power managed at run time (probably all input devices). Also there may be situations in which user space may really want to disable runtime PM for some devices (think of broken hardware for one example).

It sounds like there are really three choices here, and the decision should largely be left up to the user:

1. don't use runtime PM,

2. allow runtime PM but disable remote wakeup,

3. allow runtime PM with remote wakeup enabled.

Now, a driver may say "I can't do my job without remote wakeup". Such a driver would refuse to do runtime_suspend in case 2. But otherwise we should follow the preference of the user.

The only remaining question is how to expose this in sysfs in a way that won't be confusing and that won't be confused with the "wakeup" attribute. One possibility is to use the "level" attribute introduced in USB; possible levels are "on" (no runtime PM) and "auto" (runtime PM allowed). Then a new "runtime_wakeup" attribute could contain nothing (if wakeup is not available), "enabled", or "disabled".

Quoted 6 lines · Rafael J. Wysocki

Well, sometimes the user may want a device to be power managed at run time and not to be able to wake up the system from sleep states. For example, I'd like the USB controller in my box to be suspended at run time whenever it's not used, but surely I wouldn't like it to do system-wide wakeup, because it does that when I move the mouse which is a cordless one. Simply turning the mouse on causes the system to wake up. :-)

Right, so clearly my code is broken right now.

Quoted 3 lines

Why don't we add a flag indicating whether or not the device is allowed to be power managed at run time, something like runtime_forbidden, that the user space will be able to set through sysfs?

I think even having a runtime_wakeup flag (which defaults to on) would be sufficient.

If the worst case is scenario is that we have to resume devices in order to apply the correct policy when going into a systemwide suspend state, I think that's acceptable.

Quoted 8 lines · Matthew Garrett

Quoted 3 lines · Alan Stern

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.

So it does; my mistake.

Quoted 9 lines · Matthew Garrett

Quoted 6 lines · Matthew Garrett

Ugh. I'd really prefer us to assume that drivers are able to cope unless proven otherwise. Userspace policy makes sense where we don't have any idea whether something will work or not, but I'd really expect that most PCI drivers will either cope (in which case they'll have enabling code) or won't (in which case they won't). Why would we want userspace to influence this?

Quoted 4 lines

Though, thinking about it, you're right that setting this does override user policy. I think we need an additional flag to indicate that the device supports runtime wakeup and test that as well when doing device_may_wakeup().

You are suggesting separate flag sets for system-wide wakeup and runtime wakeup? I don't disagree, but implementing them will be problematical.

That's because it's not always possible to change a device's wakeup setting while it is suspended. Thus if a device was runtime suspended with wakeup enabled, and then we want to do a system sleep and change the device's wakeup setting to disabled, we would have to wake the device back up in order to do it.

I got a fixed BIOS from Dell and have been able to get this working now. It seems entirely happy with USB, but I'd like some sanity checks on whether I'm doing this correctly. There's certainly a couple of quirks related to setting the ACPI GPE type that would need a little bit of work in the ACPI layer, and it breaks ACPI-mediated PCI hotplug though that's easy enough to fix by just calling into the hotplug code from the core notifier.

This patch builds on top of Rafael's work on systemwide runtime power management. It supports suspending and resuming PCI devices at runtime, enabling platform wakeup events that allow the devices to automatically resume when appropriate. It currently requires platform support, but PCIe setups could be supported natively once native PCIe PME code has been added to the kernel.

I got a fixed BIOS from Dell and have been able to get this working now. It seems entirely happy with USB, but I'd like some sanity checks on whether I'm doing this correctly. There's certainly a couple of quirks related to setting the ACPI GPE type that would need a little bit of work in the ACPI layer, and it breaks ACPI-mediated PCI hotplug though that's easy enough to fix by just calling into the hotplug code from the core notifier.

This patch builds on top of Rafael's work on systemwide runtime power management. It supports suspending and resuming PCI devices at runtime, enabling platform wakeup events that allow the devices to automatically resume when appropriate. It currently requires platform support, but PCIe setups could be supported natively once native PCIe PME code has been added to the kernel.

Quoted 8 lines · On Thu, 13 Aug 2009 01:29:25 +0100 Matthew Garrett <[email protected]> wrote:

I got a fixed BIOS from Dell and have been able to get this working now. It seems entirely happy with USB, but I'd like some sanity checks on whether I'm doing this correctly. There's certainly a couple of quirks related to setting the ACPI GPE type that would need a little bit of work in the ACPI layer, and it breaks ACPI-mediated PCI hotplug though that's easy enough to fix by just calling into the hotplug code from the core notifier.

Quoted 6 lines

This patch builds on top of Rafael's work on systemwide runtime power management. It supports suspending and resuming PCI devices at runtime, enabling platform wakeup events that allow the devices to automatically resume when appropriate. It currently requires platform support, but PCIe setups could be supported natively once native PCIe PME code has been added to the kernel.

PCI bits look pretty good to me, though Rafael should take a look too.

Card readers and firewire could benefit from similar treatment,

maybe that would get us to .5W territory on some machines.

Quoted 8 lines · On Thu, 13 Aug 2009 01:29:25 +0100 Matthew Garrett <[email protected]> wrote:

I got a fixed BIOS from Dell and have been able to get this working now. It seems entirely happy with USB, but I'd like some sanity checks on whether I'm doing this correctly. There's certainly a couple of quirks related to setting the ACPI GPE type that would need a little bit of work in the ACPI layer, and it breaks ACPI-mediated PCI hotplug though that's easy enough to fix by just calling into the hotplug code from the core notifier.

Quoted 6 lines

This patch builds on top of Rafael's work on systemwide runtime power management. It supports suspending and resuming PCI devices at runtime, enabling platform wakeup events that allow the devices to automatically resume when appropriate. It currently requires platform support, but PCIe setups could be supported natively once native PCIe PME code has been added to the kernel.

PCI bits look pretty good to me, though Rafael should take a look too.

Card readers and firewire could benefit from similar treatment,

maybe that would get us to .5W territory on some machines.

I got a fixed BIOS from Dell and have been able to get this working now. It seems entirely happy with USB, but I'd like some sanity checks on whether I'm doing this correctly. There's certainly a couple of quirks related to setting the ACPI GPE type that would need a little bit of work in the ACPI layer, and it breaks ACPI-mediated PCI hotplug though that's easy enough to fix by just calling into the hotplug code from the core notifier.

This patch builds on top of Rafael's work on systemwide runtime power management. It supports suspending and resuming PCI devices at runtime, enabling platform wakeup events that allow the devices to automatically resume when appropriate. It currently requires platform support, but PCIe setups could be supported natively once native PCIe PME code has been added to the kernel.

---
 drivers/pci/pci-acpi.c   |   55 +++++++++++++++++++++++++
 drivers/pci/pci-driver.c |  100 ++++++++++++++++++++++++++++++++++++++++++++++
 drivers/pci/pci.c        |   87 ++++++++++++++++++++++++++++++++++++++++
 drivers/pci/pci.h        |    3 +
 include/linux/pci.h      |    3 +
 5 files changed, 248 insertions(+), 0 deletions(-)

drivers/pci/pci-acpi.c +1 -0

index ea15b05..a98a777 100644
@@ -12,6 +12,7 @@
 #include <linux/pci.h>
 #include <linux/module.h>
 #include <linux/pci-aspm.h>
+#include <linux/pm_runtime.h>
 #include <acpi/acpi.h>
 #include <acpi/acpi_bus.h>
 

drivers/pci/pci-acpi.c +12 -0

+	if (!handle)
+		return -ENODEV;
+
+	status = acpi_bus_get_device(handle, &acpi_dev);
+	if (ACPI_FAILURE(status))
+		return -ENODEV;
+
+	if (enable) {
+		acpi_set_gpe_type(acpi_dev->wakeup.gpe_device,
+				  acpi_dev->wakeup.gpe_number,
+				  ACPI_GPE_TYPE_WAKE_RUN);
+		acpi_enable_gpe(acpi_dev->wakeup.gpe_device,

drivers/pci/pci-acpi.c +26 -0

+
+
 static struct pci_platform_pm_ops acpi_pci_platform_pm = {
 	.is_manageable = acpi_pci_power_manageable,
 	.set_state = acpi_pci_set_power_state,
 	.choose_state = acpi_pci_choose_state,
 	.can_wakeup = acpi_pci_can_wakeup,
 	.sleep_wake = acpi_pci_sleep_wake,
+	.runtime_wake = acpi_pci_runtime_wake,
 };
 
+static void pci_device_notify(acpi_handle handle, u32 event, void *data)
+{
+	struct device *dev = data;
+
+	if (event == ACPI_NOTIFY_DEVICE_WAKE)
+		pm_runtime_resume(dev);
+}
+
+static void pci_root_bridge_notify(acpi_handle handle, u32 event, void *data)
+{
+	struct device *dev = data;
+	struct pci_dev *pci_dev = to_pci_dev(dev);
+
+	if (event == ACPI_NOTIFY_DEVICE_WAKE)
+		pci_bus_pme_event(pci_dev);
+}
+
 /* ACPI bus type */
 static int acpi_pci_find_device(struct device *dev, acpi_handle *handle)
 {

@@ -140,6 +189,9 @@ static int acpi_pci_find_device(struct device *dev, acpi_handle *handle)
 	*handle = acpi_get_child(DEVICE_ACPI_HANDLE(dev->parent), addr);
 	if (!*handle)
 		return -ENODEV;
+
+	acpi_install_notify_handler(*handle, ACPI_SYSTEM_NOTIFY,
+				    pci_device_notify, dev);
 	return 0;
 }
 

@@ -158,6 +210,9 @@ static int acpi_pci_find_root_bridge(struct device *dev, acpi_handle *handle)
 	*handle = acpi_get_pci_rootbridge_handle(seg, bus);
 	if (!*handle)
 		return -ENODEV;
+
+	acpi_install_notify_handler(*handle, ACPI_SYSTEM_NOTIFY,
+				    pci_root_bridge_notify, dev);
 	return 0;
 }
 

drivers/pci/pci-driver.c +13 -0

index d76c4c8..1f605d8 100644
@@ -11,12 +11,14 @@
 #include <linux/pci.h>
 #include <linux/module.h>
 #include <linux/init.h>
+#include <linux/interrupt.h>
 #include <linux/device.h>
 #include <linux/mempolicy.h>
 #include <linux/string.h>
 #include <linux/slab.h>
 #include <linux/sched.h>
 #include <linux/cpu.h>
+#include <linux/pm_runtime.h>
 #include "pci.h"
 
 /*

@@ -910,6 +912,101 @@ static int pci_pm_restore(struct device *dev)
 
 #endif /* !CONFIG_HIBERNATION */
 
+#ifdef CONFIG_PM_RUNTIME
+
+static int pci_pm_runtime_suspend(struct device *dev)
+{
+	struct pci_dev *pci_dev = to_pci_dev(dev);
+	struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
+	int error;
+
+	device_set_wakeup_enable(dev, 1);
+	error = pci_enable_runtime_wake(pci_dev, true);
+

drivers/pci/pci-driver.c +50 -0

+	error = pci_pm_suspend_noirq(dev);
+	enable_irq(pci_dev->irq);
+
+	if (error)
+		goto resume_noirq;
+
+	return 0;
+
+resume_noirq:
+	disable_irq(pci_dev->irq);
+	pci_pm_resume_noirq(dev);
+	enable_irq(pci_dev->irq);
+resume:
+	pci_pm_resume(dev);
+out:
+	pci_enable_runtime_wake(pci_dev, false);
+	return error;
+}
+
+static int pci_pm_runtime_resume(struct device *dev)
+{
+	struct pci_dev *pci_dev = to_pci_dev(dev);
+	struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
+	int error = 0;
+
+	disable_irq(pci_dev->irq);
+	error = pci_pm_resume_noirq(dev);
+	enable_irq(pci_dev->irq);
+
+	if (error)
+		return error;
+
+	error = pci_pm_resume(dev);
+
+	if (error)
+		return error;
+
+	if (pm->runtime_resume)
+		error = pm->runtime_resume(dev);
+
+	if (error)
+		return error;
+
+	error = pci_enable_runtime_wake(pci_dev, false);
+
+	if (error)
+		return error;
+
+	return 0;
+}

drivers/pci/pci-driver.c +12 -0

+
+#else /* !CONFIG_PM_RUNTIME */
+
+#define pci_pm_runtime_suspend	NULL
+#define pci_pm_runtime_resume	NULL
+#define pci_pm_runtime_idle	NULL
+
+#endif
+
 struct dev_pm_ops pci_dev_pm_ops = {
 	.prepare = pci_pm_prepare,
 	.complete = pci_pm_complete,

@@ -925,6 +1022,9 @@ struct dev_pm_ops pci_dev_pm_ops = {
 	.thaw_noirq = pci_pm_thaw_noirq,
 	.poweroff_noirq = pci_pm_poweroff_noirq,
 	.restore_noirq = pci_pm_restore_noirq,
+	.runtime_suspend = pci_pm_runtime_suspend,
+	.runtime_resume = pci_pm_runtime_resume,
+	.runtime_idle = pci_pm_runtime_idle,
 };
 
 #define PCI_PM_OPS_PTR	(&pci_dev_pm_ops)

drivers/pci/pci.c +21 -0

index dbd0f94..ab3a116 100644
@@ -18,6 +18,7 @@
 #include <linux/log2.h>
 #include <linux/pci-aspm.h>
 #include <linux/pm_wakeup.h>
+#include <linux/pm_runtime.h>
 #include <linux/interrupt.h>
 #include <asm/dma.h>	/* isa_dma_bridge_buggy */
 #include <linux/device.h>

@@ -428,6 +429,12 @@ static inline int platform_pci_sleep_wake(struct pci_dev *dev, bool enable)
 			pci_platform_pm->sleep_wake(dev, enable) : -ENODEV;
 }
 
+static inline int platform_pci_runtime_wake(struct pci_dev *dev, bool enable)
+{
+	return pci_platform_pm ?
+			pci_platform_pm->runtime_wake(dev, enable) : -ENODEV;
+}
+
 /**
  * pci_raw_set_power_state - Use PCI PM registers to set the power state of
  *                           given PCI device

@@ -1239,6 +1246,38 @@ int pci_enable_wake(struct pci_dev *dev, pci_power_t state, bool enable)
 }
 
 /**
+ * pci_enable_runtime_wake - enable PCI device as runtime wakeup event source
+ * @dev: PCI device affected
+ * @enable: True to enable event generation; false to disable
+ *
+ * This enables the device as a runtime wakeup event source, or disables it.
+ * This typically requires platform support.
+ *
+ * RETURN VALUE:
+ * 0 is returned on success
+ * -EINVAL is returned if device is not supposed to wake up the system
+ * -ENODEV is returned if platform cannot support runtime PM on the device
+ */
+int pci_enable_runtime_wake(struct pci_dev *dev, bool enable)
+{

drivers/pci/pci.c +49 -0

+/**
  * pci_wake_from_d3 - enable/disable device to wake up from D3_hot or D3_cold
  * @dev: PCI device to prepare
  * @enable: True to enable wake-up event generation; false to disable

@@ -1346,6 +1385,54 @@ int pci_back_from_sleep(struct pci_dev *dev)
 }
 
 /**
+ * pci_dev_pme_event - check if a device has a pending pme
+ *
+ * @dev: Device to handle.
+ */
+
+int pci_dev_pme_event(struct pci_dev *dev)
+{
+	u16 pmcsr;
+
+	if (!dev->pm_cap)
+		return -ENODEV;
+
+	pci_read_config_word(dev, dev->pm_cap + PCI_PM_CTRL, &pmcsr);
+
+	if (pmcsr & PCI_PM_CTRL_PME_STATUS) {
+		pci_write_config_word(dev, dev->pm_cap + PCI_PM_CTRL, pmcsr);
+		pm_runtime_get(&dev->dev);
+		return 0;
+	}
+
+	return -ENODEV;
+}
+
+/**
+ * pci_bus_pme_event - search for subordinate devices with a pending
+ *		   pme and handle them
+ *
+ * @dev: Parent device to handle
+ */
+int pci_bus_pme_event(struct pci_dev *dev)
+{
+	struct pci_bus *bus;
+	struct pci_dev *pdev;
+
+	if (pci_is_root_bus(dev->bus))
+		bus = dev->bus;
+	else if (dev->subordinate)
+		bus = dev->subordinate;
+	else
+		return -ENODEV;
+
+	list_for_each_entry(pdev, &bus->devices, bus_list)
+		pci_dev_pme_event(pdev);
+
+	return 0;
+}
+
+/**
  * pci_pm_init - Initialize PM functions of given PCI device
  * @dev: PCI device to handle.
  */

drivers/pci/pci.h +3 -0

index f73bcbe..a81aff2 100644
@@ -34,6 +34,8 @@ extern int pci_mmap_fits(struct pci_dev *pdev, int resno,
  *
  * @sleep_wake: enables/disables the system wake up capability of given device
  *
+ * @runtime_wake: enables/disables the runtime wakeup capability of given device
+ *
  * If given platform is generally capable of power managing PCI devices, all of
  * these callbacks are mandatory.
  */

@@ -43,6 +45,7 @@ struct pci_platform_pm_ops {
 	pci_power_t (*choose_state)(struct pci_dev *dev);
 	bool (*can_wakeup)(struct pci_dev *dev);
 	int (*sleep_wake)(struct pci_dev *dev, bool enable);
+	int (*runtime_wake)(struct pci_dev *dev, bool enable);
 };
 
 extern int pci_set_platform_pm(struct pci_platform_pm_ops *ops);

include/linux/pci.h +0 -0

index 115fb7b..8a3fea0 100644
@@ -734,10 +734,13 @@ pci_power_t pci_choose_state(struct pci_dev *dev, pm_message_t state);
 bool pci_pme_capable(struct pci_dev *dev, pci_power_t state);
 void pci_pme_active(struct pci_dev *dev, bool enable);
 int pci_enable_wake(struct pci_dev *dev, pci_power_t state, bool enable);