PCI / PM: Disable wakeup during shutdown for devices not enabled to wake up
Subsystems (3)
PATCH 1/1 PCI / PM: Disable wakeup during shutdown for devices not enabled to wake up
From: Rafael J. Wysocki <[email protected]>
If a PCI device is enabled to generate wakeup signals (PME) when put into a low-power state by runtime PM, it will be still enabled to generate those signals after the system shutdown, unless its driver's .shutdown() callback takes care of the wakeup signals generation setting. Moreover, there are devices that are not enabled to wake up the system and that are configured by runtime PM to generate wakeup signals so that (runtime) remote wakeup works with them. Those devices should be reconfigured during system shutdown so that they don't generate wakeup signals, but at least some drivers don't do that. However, that very well may be done by the PCI core so that drivers don't have to worry about it. For this reason, modify pci_device_shutdown() to disable the generation of wakeup events for devices not supposed to wake up the system.
References: https://bugzilla.kernel.org/show_bug.cgi?id=37952
--- drivers/pci/pci-driver.c | 10 ++++++++++ 1 file changed, 10 insertions(+)
drivers/pci/pci-driver.c +10 -0
@@ -430,6 +430,16 @@ static void pci_device_shutdown(struct d drv->shutdown (pci_dev);pci_msi_shutdown (pci_dev);pci_msix_shutdown (pci_dev); + +/* +* Devices may be enabled to wake up by runtime PM, but they need not +* be supposed to wake up the system from its "power off" state (e.g. +* ACPI S5). Therefore disable wakeup for all devices that aren't +* supposed to wake up the system at this point. The state argument +* will be ignored by pci_enable_wake(). +*/ +if (!device_may_wakeup (dev)) +pci_enable_wake (pci_dev,PCI_UNKNOWN , false); }#ifdef CONFIG_PM
Quoted 35 lines · On Tue, 7 Feb 2012 00:50:35 +0100 "Rafael J. Wysocki" <[email protected]> wrote:
From: Rafael J. Wysocki <[email protected]>
If a PCI device is enabled to generate wakeup signals (PME) when put into a low-power state by runtime PM, it will be still enabled to generate those signals after the system shutdown, unless its driver's .shutdown() callback takes care of the wakeup signals generation setting. Moreover, there are devices that are not enabled to wake up the system and that are configured by runtime PM to generate wakeup signals so that (runtime) remote wakeup works with them. Those devices should be reconfigured during system shutdown so that they don't generate wakeup signals, but at least some drivers don't do that. However, that very well may be done by the PCI core so that drivers don't have to worry about it. For this reason, modify
pci_device_shutdown()to disable the generation of wakeup events for devices not supposed to wake up the system.References: https://bugzilla.kernel.org/show_bug.cgi?id=37952
Reported-and-tested-by Kamil Iskra <[email protected]>Signed-off-by Rafael J. Wysocki <[email protected]>--- drivers/pci/pci-driver.c | 10 ++++++++++ 1 file changed, 10 insertions(+)Index:
linux/drivers/pci/pci-driver.c
===================================================================drivers/pci/pci-driver.c +3
-0@@ -430,6 +430,16 @@ static void pci_device_shutdown(struct d drv->shutdown (pci_dev);pci_msi_shutdown (pci_dev);pci_msix_shutdown (pci_dev); + +/* +* Devices may be enabled to wake up by runtime PM, but they need not
drivers/pci/pci-driver.c static void pci_device_shutdown(struct d · line 445
Quoted 10 lines
drivers/pci/pci-driver.c +7
-0+* be supposed to wake up the system from its "power off" state (e.g. +* ACPI S5). Therefore disable wakeup for all devices that aren't +* supposed to wake up the system at this point. The state argument +* will be ignored by pci_enable_wake(). +*/ +if (!device_may_wakeup (dev)) +pci_enable_wake (pci_dev,PCI_UNKNOWN , false); }#ifdef CONFIG_PM
So where do we end up setting the right wakeup state for devices we actually *do* want to wake the system from S5? Presumably later in the shutdown sequence?
Signature · 1 line
Jesse Barnes, Intel Open Source Technology Center
Quoted 46 lines · Jesse Barnes
Quoted 43 lines · On Tue, 7 Feb 2012 00:50:35 +0100 "Rafael J. Wysocki" <[email protected]> wrote:
From: Rafael J. Wysocki <[email protected]>
If a PCI device is enabled to generate wakeup signals (PME) when put into a low-power state by runtime PM, it will be still enabled to generate those signals after the system shutdown, unless its driver's .shutdown() callback takes care of the wakeup signals generation setting. Moreover, there are devices that are not enabled to wake up the system and that are configured by runtime PM to generate wakeup signals so that (runtime) remote wakeup works with them. Those devices should be reconfigured during system shutdown so that they don't generate wakeup signals, but at least some drivers don't do that. However, that very well may be done by the PCI core so that drivers don't have to worry about it. For this reason, modify
pci_device_shutdown()to disable the generation of wakeup events for devices not supposed to wake up the system.References: https://bugzilla.kernel.org/show_bug.cgi?id=37952
Reported-and-tested-by Kamil Iskra <[email protected]>Signed-off-by Rafael J. Wysocki <[email protected]>--- drivers/pci/pci-driver.c | 10 ++++++++++ 1 file changed, 10 insertions(+)Index:
linux/drivers/pci/pci-driver.c
===================================================================drivers/pci/pci-driver.c +10
-0@@ -430,6 +430,16 @@ static void pci_device_shutdown(struct d drv->shutdown (pci_dev);pci_msi_shutdown (pci_dev);pci_msix_shutdown (pci_dev); + +/* +* Devices may be enabled to wake up by runtime PM, but they need not +* be supposed to wake up the system from its "power off" state (e.g. +* ACPI S5). Therefore disable wakeup for all devices that aren't +* supposed to wake up the system at this point. The state argument +* will be ignored by pci_enable_wake(). +*/ +if (!device_may_wakeup (dev)) +pci_enable_wake (pci_dev,PCI_UNKNOWN , false); }
Quoted 6 lines
drivers/pci/pci-driver.c static void pci_device_shutdown(struct d · line 445
Quoted 2 lines
drivers/pci/pci-driver.c +0
-0#ifdef CONFIG_PM So where do we end up setting the right wakeup state for devices we actually *do* want to wake the system from S5? Presumably later in the shutdown sequence?
Yes, in general. However, device_may_wakeup(dev) has to return true for those devices, so they aren't affected by this change.
Thanks,
Rafael
Quoted 54 lines · On Fri, 10 Feb 2012 23:40:29 +0100 "Rafael J. Wysocki" <[email protected]> wrote:
Quoted 46 lines · Jesse Barnes
Quoted 43 lines · On Tue, 7 Feb 2012 00:50:35 +0100 "Rafael J. Wysocki" <[email protected]> wrote:
From: Rafael J. Wysocki <[email protected]>
If a PCI device is enabled to generate wakeup signals (PME) when put into a low-power state by runtime PM, it will be still enabled to generate those signals after the system shutdown, unless its driver's .shutdown() callback takes care of the wakeup signals generation setting. Moreover, there are devices that are not enabled to wake up the system and that are configured by runtime PM to generate wakeup signals so that (runtime) remote wakeup works with them. Those devices should be reconfigured during system shutdown so that they don't generate wakeup signals, but at least some drivers don't do that. However, that very well may be done by the PCI core so that drivers don't have to worry about it. For this reason, modify
pci_device_shutdown()to disable the generation of wakeup events for devices not supposed to wake up the system.References: https://bugzilla.kernel.org/show_bug.cgi?id=37952
Reported-and-tested-by Kamil Iskra <[email protected]>Signed-off-by Rafael J. Wysocki <[email protected]>--- drivers/pci/pci-driver.c | 10 ++++++++++ 1 file changed, 10 insertions(+)Index:
linux/drivers/pci/pci-driver.c
===================================================================drivers/pci/pci-driver.c +10
-0@@ -430,6 +430,16 @@ static void pci_device_shutdown(struct d drv->shutdown (pci_dev);pci_msi_shutdown (pci_dev);pci_msix_shutdown (pci_dev); + +/* +* Devices may be enabled to wake up by runtime PM, but they need not +* be supposed to wake up the system from its "power off" state (e.g. +* ACPI S5). Therefore disable wakeup for all devices that aren't +* supposed to wake up the system at this point. The state argument +* will be ignored by pci_enable_wake(). +*/ +if (!device_may_wakeup (dev)) +pci_enable_wake (pci_dev,PCI_UNKNOWN , false); }Quoted 6 lines
drivers/pci/pci-driver.c static void pci_device_shutdown(struct d · line 445
Quoted 2 lines
drivers/pci/pci-driver.c +0
-0#ifdef CONFIG_PM So where do we end up setting the right wakeup state for devices we actually *do* want to wake the system from S5? Presumably later in the shutdown sequence?
Quoted 2 lines
Yes, in general. However,
device_may_wakeup(dev)has to return true for those devices, so they aren't affected by this change.
Ok thanks Rafael, applied. Now to do a little testing...
Thanks,
Signature · 1 line
Jesse Barnes, Intel Open Source Technology Center
Rafael J. Wysocki <[email protected]> :
[...]
Quoted 5 lines
If a PCI device is enabled to generate wakeup signals (PME) when put into a low-power state by runtime PM, it will be still enabled to generate those signals after the system shutdown, unless its driver's .shutdown() callback takes care of the wakeup signals generation setting. Moreover, there are devices that are not enabled to wake
Quoted 8 lines
up the system and that are configured by runtime PM to generate wakeup signals so that (runtime) remote wakeup works with them. Those devices should be reconfigured during system shutdown so that they don't generate wakeup signals, but at least some drivers don't do that. However, that very well may be done by the PCI core so that drivers don't have to worry about it. For this reason, modify
pci_device_shutdown()to disable the generation of wakeup events for devices not supposed to wake up the system.
On a tangent side, if register writes are ignored by the PCI device after its .runtime_suspend() handler has been called, whose responsibility is it to bring back the device in a state from where the .shutdown() callback can operate ?
The r8169 driver exhibits this problem. Its .runtime_xyz() callbacks are called on link activity. Thus, after a link loss - or no link at all - .runtime_suspend() and an ineffective .shutdown(), the device is brought back up as soon as the link recovers.
The device driver can monitor .runtime_{suspend / resume} activity and take appropriate action in .shutdown(). Sameer Nanda has sent such a patch (see below). I am not sure this is the adequate fix:
1. the driver ought essentially to perform the hardware part of a .runtime_resume() on its own behalf. Mildly engaging.
2. by the same line as above, could it make more sense to call pm_runtime_resume() in driver/base/core.c::device_shutdown ?
Thanks.
From: Sameer Nanda <[email protected]>
Date: Tue, 14 Feb 2012 10:31:15 -0800
Subject: [PATCH] r8169: transition to D0 state at shutdown
With runtime PM, if the ethernet cable is disconnected, the device is transitioned to D3 state to conserve energy. If the system is shutdown in this state, any register accesses in rtl_shutdown are dropped on the floor.
This patch transitions the device back to D0 state in rtl_shutdown if the device was runtime PM suspended. In addition, since runtiome PM also mucks around with WOL options, the saved WOL options are also restored in rtl_shutdown.
--- drivers/net/ethernet/realtek/r8169.c | 12 ++++++++++++ 1 files changed, 12 insertions(+), 0 deletions(-)
drivers/net/ethernet/realtek/r8169.c +12 -0
@@ -691,6 +691,7 @@ struct rtl8169_private {u16 intr_event;u16 napi_event;u16 intr_mask; +bool runtime_suspended;struct mdio_ops {void (*write)(void __iomem *,int ,int );
@@ -4315,6 +4316,7 @@ static int rtl8169_open(struct net_device *dev)rtl_hw_start (dev); tp->saved_wolopts =0 ; + tp->runtime_suspended = false;pm_runtime_put_noidle (&pdev->dev);rtl8169_check_link_status (dev, tp, ioaddr);
@@ -6113,6 +6115,7 @@ static int rtl8169_runtime_suspend(struct device *device)spin_lock_irq (&tp->lock); tp->saved_wolopts =__rtl8169_get_wol (tp);__rtl8169_set_wol (tp,WAKE_ANY ); + tp->runtime_suspended = true;spin_unlock_irq (&tp->lock);rtl8169_net_suspend (dev);
@@ -6132,6 +6135,7 @@ static int rtl8169_runtime_resume(struct device *device)spin_lock_irq (&tp->lock);__rtl8169_set_wol (tp, tp->saved_wolopts); tp->saved_wolopts =0 ; + tp->runtime_suspended = false;spin_unlock_irq (&tp->lock);rtl8169_init_phy (dev, tp);
@@ -6195,6 +6199,10 @@ static void rtl_shutdown(struct pci_dev *pdev)struct net_device *dev =pci_get_drvdata (pdev);struct rtl8169_private *tp =netdev_priv (dev); +/* Get the device back to D0 state if it was runtime suspended. */ +if (tp->runtime_suspended) +pci_set_power_state (pdev,PCI_D0 ); +rtl8169_net_suspend (dev);/* Restore original MAC address */
@@ -6202,6 +6210,10 @@ static void rtl_shutdown(struct pci_dev *pdev)spin_lock_irq (&tp->lock); +/* Restore WOL flags if they were messed around with. */ +if (tp->saved_wolopts) +__rtl8169_set_wol (tp, tp->saved_wolopts); +rtl8169_hw_reset (tp);spin_unlock_irq (&tp->lock);
Signature · 1 line
1.7.7.6
Quoted 26 lines · On Fri, 2 Mar 2012 15:44:27 +0100 Francois Romieu <[email protected]> wrote:
Rafael J. Wysocki <[email protected]> :
[...]Quoted 5 lines
If a PCI device is enabled to generate wakeup signals (PME) when put into a low-power state by runtime PM, it will be still enabled to generate those signals after the system shutdown, unless its driver's .shutdown() callback takes care of the wakeup signals generation setting. Moreover, there are devices that are not enabled to wake
Quoted 8 lines
up the system and that are configured by runtime PM to generate wakeup signals so that (runtime) remote wakeup works with them. Those devices should be reconfigured during system shutdown so that they don't generate wakeup signals, but at least some drivers don't do that. However, that very well may be done by the PCI core so that drivers don't have to worry about it. For this reason, modify
pci_device_shutdown()to disable the generation of wakeup events for devices not supposed to wake up the system.On a tangent side, if register writes are ignored by the PCI device after its .runtime_suspend() handler has been called, whose responsibility is it to bring back the device in a state from where the .shutdown() callback can operate ?
The r8169 driver exhibits this problem. Its .runtime_xyz() callbacks are called on link activity. Thus, after a link loss - or no link at all - .runtime_suspend() and an ineffective .shutdown(), the device is brought back up as soon as the link recovers.
Quoted 7 lines
The device driver can monitor .runtime_{suspend / resume} activity and take appropriate action in .shutdown(). Sameer Nanda has sent such a patch (see below). I am not sure this is the adequate fix:
1. the driver ought essentially to perform the hardware part of a .runtime_resume() on its own behalf. Mildly engaging.
2. by the same line as above, could it make more sense to callpm_runtime_resume()in driver/base/core.c::device_shutdown ?
I thought there would be a more elegant way of increasing the device use count on entering .shutdown, either from the wifi core or at the driver level, so you don't have to track things with a separate flag.
Rafael?
Signature · 1 line
Jesse Barnes, Intel Open Source Technology Center
Quoted 37 lines · Jesse Barnes
Quoted 26 lines · On Fri, 2 Mar 2012 15:44:27 +0100 Francois Romieu <[email protected]> wrote:
Rafael J. Wysocki <[email protected]> :
[...]Quoted 5 lines
If a PCI device is enabled to generate wakeup signals (PME) when put into a low-power state by runtime PM, it will be still enabled to generate those signals after the system shutdown, unless its driver's .shutdown() callback takes care of the wakeup signals generation setting. Moreover, there are devices that are not enabled to wake
Quoted 8 lines
up the system and that are configured by runtime PM to generate wakeup signals so that (runtime) remote wakeup works with them. Those devices should be reconfigured during system shutdown so that they don't generate wakeup signals, but at least some drivers don't do that. However, that very well may be done by the PCI core so that drivers don't have to worry about it. For this reason, modify
pci_device_shutdown()to disable the generation of wakeup events for devices not supposed to wake up the system.On a tangent side, if register writes are ignored by the PCI device after its .runtime_suspend() handler has been called, whose responsibility is it to bring back the device in a state from where the .shutdown() callback can operate ?
The r8169 driver exhibits this problem. Its .runtime_xyz() callbacks are called on link activity. Thus, after a link loss - or no link at all - .runtime_suspend() and an ineffective .shutdown(), the device is brought back up as soon as the link recovers.
Quoted 7 lines
The device driver can monitor .runtime_{suspend / resume} activity and take appropriate action in .shutdown(). Sameer Nanda has sent such a patch (see below). I am not sure this is the adequate fix:
1. the driver ought essentially to perform the hardware part of a .runtime_resume() on its own behalf. Mildly engaging.
2. by the same line as above, could it make more sense to callpm_runtime_resume()in driver/base/core.c::device_shutdown ?
Quoted 3 lines
I thought there would be a more elegant way of increasing the device use count on entering .shutdown, either from the wifi core or at the driver level, so you don't have to track things with a separate flag.
That's right. For example, instead of keeping its own flag to know whether or not the device is runtime-suspended, the shutdown routine could just always call pm_runtime_resume_sync(). Or it could ask the PCI core to put the device into D0, regardless of the device's current state.
The driver core doesn't do this because most devices don't need it, and it would slow down the whole shutdown procedure if every suspended device were always resumed.
Alan Stern
Quoted 49 lines · Alan Stern
Quoted 37 lines · Jesse Barnes
Quoted 26 lines · On Fri, 2 Mar 2012 15:44:27 +0100 Francois Romieu <[email protected]> wrote:
Rafael J. Wysocki <[email protected]> :
[...]Quoted 5 lines
If a PCI device is enabled to generate wakeup signals (PME) when put into a low-power state by runtime PM, it will be still enabled to generate those signals after the system shutdown, unless its driver's .shutdown() callback takes care of the wakeup signals generation setting. Moreover, there are devices that are not enabled to wake
Quoted 8 lines
up the system and that are configured by runtime PM to generate wakeup signals so that (runtime) remote wakeup works with them. Those devices should be reconfigured during system shutdown so that they don't generate wakeup signals, but at least some drivers don't do that. However, that very well may be done by the PCI core so that drivers don't have to worry about it. For this reason, modify
pci_device_shutdown()to disable the generation of wakeup events for devices not supposed to wake up the system.On a tangent side, if register writes are ignored by the PCI device after its .runtime_suspend() handler has been called, whose responsibility is it to bring back the device in a state from where the .shutdown() callback can operate ?
The r8169 driver exhibits this problem. Its .runtime_xyz() callbacks are called on link activity. Thus, after a link loss - or no link at all - .runtime_suspend() and an ineffective .shutdown(), the device is brought back up as soon as the link recovers.
Quoted 7 lines
The device driver can monitor .runtime_{suspend / resume} activity and take appropriate action in .shutdown(). Sameer Nanda has sent such a patch (see below). I am not sure this is the adequate fix:
1. the driver ought essentially to perform the hardware part of a .runtime_resume() on its own behalf. Mildly engaging.
2. by the same line as above, could it make more sense to callpm_runtime_resume()in driver/base/core.c::device_shutdown ?Quoted 3 lines
I thought there would be a more elegant way of increasing the device use count on entering .shutdown, either from the wifi core or at the driver level, so you don't have to track things with a separate flag.
That's right. For example, instead of keeping its own flag to know whether or not the device is runtime-suspended, the shutdown routine could just always call
pm_runtime_resume_sync(). Or it could ask the PCI core to put the device into D0, regardless of the device's current state.
Quoted 3 lines
The driver core doesn't do this because most devices don't need it, and it would slow down the whole shutdown procedure if every suspended device were always resumed.
For devices known to have this problem I think calling pm_runtime_resume_sync() unconditionally at the beginning of .suspend() would be a good approach in general.
Thanks,
Rafael
Quoted 50 lines · Rafael J. Wysocki
Quoted 49 lines · Alan Stern
Quoted 37 lines · Jesse Barnes
Quoted 26 lines · On Fri, 2 Mar 2012 15:44:27 +0100 Francois Romieu <[email protected]> wrote:
Rafael J. Wysocki <[email protected]> :
[...]Quoted 5 lines
If a PCI device is enabled to generate wakeup signals (PME) when put into a low-power state by runtime PM, it will be still enabled to generate those signals after the system shutdown, unless its driver's .shutdown() callback takes care of the wakeup signals generation setting. Moreover, there are devices that are not enabled to wake
Quoted 8 lines
up the system and that are configured by runtime PM to generate wakeup signals so that (runtime) remote wakeup works with them. Those devices should be reconfigured during system shutdown so that they don't generate wakeup signals, but at least some drivers don't do that. However, that very well may be done by the PCI core so that drivers don't have to worry about it. For this reason, modify
pci_device_shutdown()to disable the generation of wakeup events for devices not supposed to wake up the system.On a tangent side, if register writes are ignored by the PCI device after its .runtime_suspend() handler has been called, whose responsibility is it to bring back the device in a state from where the .shutdown() callback can operate ?
The r8169 driver exhibits this problem. Its .runtime_xyz() callbacks are called on link activity. Thus, after a link loss - or no link at all - .runtime_suspend() and an ineffective .shutdown(), the device is brought back up as soon as the link recovers.
Quoted 7 lines
The device driver can monitor .runtime_{suspend / resume} activity and take appropriate action in .shutdown(). Sameer Nanda has sent such a patch (see below). I am not sure this is the adequate fix:
1. the driver ought essentially to perform the hardware part of a .runtime_resume() on its own behalf. Mildly engaging.
2. by the same line as above, could it make more sense to callpm_runtime_resume()in driver/base/core.c::device_shutdown ?Quoted 3 lines
I thought there would be a more elegant way of increasing the device use count on entering .shutdown, either from the wifi core or at the driver level, so you don't have to track things with a separate flag.
That's right. For example, instead of keeping its own flag to know whether or not the device is runtime-suspended, the shutdown routine could just always call
pm_runtime_resume_sync(). Or it could ask the PCI core to put the device into D0, regardless of the device's current state.
Quoted 7 lines
Quoted 3 lines
The driver core doesn't do this because most devices don't need it, and it would slow down the whole shutdown procedure if every suspended device were always resumed.
For devices known to have this problem I think calling
pm_runtime_resume_sync()unconditionally at the beginning of .suspend() would be a good approach in general.
Or even call pm_runtime_get_sync() at the beginning and then pm_runtime_put_noidle() after .shutdown() has done its own stuff. Then, you have a guarantee that runtime PM won't interfere with the shutdown operations.
Thanks,
Rafael
Quoted 8 lines · On Fri, 2 Mar 2012 09:37:12 -0800 Jesse Barnes <[email protected]> wrote:
Quoted 7 lines
The device driver can monitor .runtime_{suspend / resume} activity and take appropriate action in .shutdown(). Sameer Nanda has sent such a patch (see below). I am not sure this is the adequate fix:
1. the driver ought essentially to perform the hardware part of a .runtime_resume() on its own behalf. Mildly engaging.
2. by the same line as above, could it make more sense to callpm_runtime_resume()in driver/base/core.c::device_shutdown ?
Quoted 5 lines
I thought there would be a more elegant way of increasing the device use count on entering .shutdown, either from the wifi core or at the driver level, so you don't have to track things with a separate flag.
Rafael?
s/wifi/net of course.
Signature · 1 line
Jesse Barnes, Intel Open Source Technology Center