diff mbox series

[RFC] PCI/PME: fix PME runtime PM handling

Message ID 1e0c3baa-c2bb-0a91-19ca-65f4784b1424@gmail.com (mailing list archive)
State New, archived
Delegated to: Bjorn Helgaas
Headers show
Series [RFC] PCI/PME: fix PME runtime PM handling | expand

Commit Message

Heiner Kallweit Dec. 30, 2018, 11:06 a.m. UTC
I face the issue that no PME is generated if the network cable is
re-plugged. To explain that in a little more detail:
r8169 driver enters runtime suspend 10 seconds after cable is detached.
LinkUp detection is armed and device enters D3hot. When the cable is
re-plugged Linkup should generate a PME and device is resumed.
But system receives no PME from the device.

Wake-on-LAN from S3 works perfectly fine and generates a PME.

After checking the pcie pme code and some experiments I found that
the following fixes the issue for me. Now system receives the PME
and properly runtime-resumes the network device.

But I'm no expert in PCIe and PME, therefore I'm not sure whether
the fix is correct. Please advise.

Adding also Mika as author of 0e157e528604 ("PCI/PME: Implement
runtime PM callbacks").

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 drivers/pci/pcie/pme.c | 29 ++---------------------------
 1 file changed, 2 insertions(+), 27 deletions(-)

Comments

Mika Westerberg Dec. 31, 2018, 9:32 a.m. UTC | #1
Hi Heiner,

On Sun, Dec 30, 2018 at 12:06:14PM +0100, Heiner Kallweit wrote:
> I face the issue that no PME is generated if the network cable is
> re-plugged. To explain that in a little more detail:
> r8169 driver enters runtime suspend 10 seconds after cable is detached.
> LinkUp detection is armed and device enters D3hot. When the cable is
> re-plugged Linkup should generate a PME and device is resumed.
> But system receives no PME from the device.
> 
> Wake-on-LAN from S3 works perfectly fine and generates a PME.
> 
> After checking the pcie pme code and some experiments I found that
> the following fixes the issue for me. Now system receives the PME
> and properly runtime-resumes the network device.
> 
> But I'm no expert in PCIe and PME, therefore I'm not sure whether
> the fix is correct. Please advise.
>
> Adding also Mika as author of 0e157e528604 ("PCI/PME: Implement
> runtime PM callbacks").

The root port cannot trigger an interrupt when it is in D3 (hot or cold)
so there needs to be a way to "wakeup" the hierarchy first which is
typically done by using sideband signal called WAKE#. Once the hierarchy
is brought back to communicating state the PME message is propagated to
the root port and the interrupt triggers (now the root port is in D0).
For some reason this does not seem to happen in your case and to
understand why we would need to gather a bit more information.

Can you file a kernel bug about this on bugzilla.kernel.org? Then attach
there acpidump and output of 'sudo lspci -vv'.

Thanks!
Heiner Kallweit Dec. 31, 2018, 9:48 a.m. UTC | #2
On 31.12.2018 10:32, Mika Westerberg wrote:
> Hi Heiner,
> 
> On Sun, Dec 30, 2018 at 12:06:14PM +0100, Heiner Kallweit wrote:
>> I face the issue that no PME is generated if the network cable is
>> re-plugged. To explain that in a little more detail:
>> r8169 driver enters runtime suspend 10 seconds after cable is detached.
>> LinkUp detection is armed and device enters D3hot. When the cable is
>> re-plugged Linkup should generate a PME and device is resumed.
>> But system receives no PME from the device.
>>
>> Wake-on-LAN from S3 works perfectly fine and generates a PME.
>>
>> After checking the pcie pme code and some experiments I found that
>> the following fixes the issue for me. Now system receives the PME
>> and properly runtime-resumes the network device.
>>
>> But I'm no expert in PCIe and PME, therefore I'm not sure whether
>> the fix is correct. Please advise.
>>
>> Adding also Mika as author of 0e157e528604 ("PCI/PME: Implement
>> runtime PM callbacks").
> 
> The root port cannot trigger an interrupt when it is in D3 (hot or cold)
> so there needs to be a way to "wakeup" the hierarchy first which is
> typically done by using sideband signal called WAKE#. Once the hierarchy
> is brought back to communicating state the PME message is propagated to
> the root port and the interrupt triggers (now the root port is in D0).
> For some reason this does not seem to happen in your case and to
> understand why we would need to gather a bit more information.
> 
> Can you file a kernel bug about this on bugzilla.kernel.org? Then attach
> there acpidump and output of 'sudo lspci -vv'.
> 
> Thanks!
> 
Thanks Mika, that's the link to the filed bug report:
https://bugzilla.kernel.org/show_bug.cgi?id=202103

Heiner
Heiner Kallweit Dec. 31, 2018, 10:37 a.m. UTC | #3
On 31.12.2018 10:32, Mika Westerberg wrote:
> Hi Heiner,
> 
> On Sun, Dec 30, 2018 at 12:06:14PM +0100, Heiner Kallweit wrote:
>> I face the issue that no PME is generated if the network cable is
>> re-plugged. To explain that in a little more detail:
>> r8169 driver enters runtime suspend 10 seconds after cable is detached.
>> LinkUp detection is armed and device enters D3hot. When the cable is
>> re-plugged Linkup should generate a PME and device is resumed.
>> But system receives no PME from the device.
>>
>> Wake-on-LAN from S3 works perfectly fine and generates a PME.
>>
>> After checking the pcie pme code and some experiments I found that
>> the following fixes the issue for me. Now system receives the PME
>> and properly runtime-resumes the network device.
>>
>> But I'm no expert in PCIe and PME, therefore I'm not sure whether
>> the fix is correct. Please advise.
>>
>> Adding also Mika as author of 0e157e528604 ("PCI/PME: Implement
>> runtime PM callbacks").
> 
> The root port cannot trigger an interrupt when it is in D3 (hot or cold)
> so there needs to be a way to "wakeup" the hierarchy first which is
> typically done by using sideband signal called WAKE#. Once the hierarchy
> is brought back to communicating state the PME message is propagated to
> the root port and the interrupt triggers (now the root port is in D0).
> For some reason this does not seem to happen in your case and to
> understand why we would need to gather a bit more information.
> 
> Can you file a kernel bug about this on bugzilla.kernel.org? Then attach
> there acpidump and output of 'sudo lspci -vv'.
> 
> Thanks!
> 
After reading again your commit message for adding the PME runtime PM
callbacks: It seems that you focused on root ports only when adding
this functionality. Should there be a check whether port is a root port?

Heiner
Mika Westerberg Dec. 31, 2018, 10:56 a.m. UTC | #4
On Mon, Dec 31, 2018 at 11:37:20AM +0100, Heiner Kallweit wrote:
> After reading again your commit message for adding the PME runtime PM
> callbacks: It seems that you focused on root ports only when adding
> this functionality. Should there be a check whether port is a root port?

This particular PME functionality (native PCIe PME) is present only on
root ports and the pcie_pme driver only binds to those.
Heiner Kallweit Dec. 31, 2018, 1:30 p.m. UTC | #5
On 31.12.2018 10:32, Mika Westerberg wrote:
> Hi Heiner,
> 
> On Sun, Dec 30, 2018 at 12:06:14PM +0100, Heiner Kallweit wrote:
>> I face the issue that no PME is generated if the network cable is
>> re-plugged. To explain that in a little more detail:
>> r8169 driver enters runtime suspend 10 seconds after cable is detached.
>> LinkUp detection is armed and device enters D3hot. When the cable is
>> re-plugged Linkup should generate a PME and device is resumed.
>> But system receives no PME from the device.
>>
>> Wake-on-LAN from S3 works perfectly fine and generates a PME.
>>
>> After checking the pcie pme code and some experiments I found that
>> the following fixes the issue for me. Now system receives the PME
>> and properly runtime-resumes the network device.
>>
>> But I'm no expert in PCIe and PME, therefore I'm not sure whether
>> the fix is correct. Please advise.
>>
>> Adding also Mika as author of 0e157e528604 ("PCI/PME: Implement
>> runtime PM callbacks").
> 
> The root port cannot trigger an interrupt when it is in D3 (hot or cold)
> so there needs to be a way to "wakeup" the hierarchy first which is
> typically done by using sideband signal called WAKE#. Once the hierarchy
> is brought back to communicating state the PME message is propagated to
> the root port and the interrupt triggers (now the root port is in D0).
> For some reason this does not seem to happen in your case and to
> understand why we would need to gather a bit more information.

One more thing I just found, not sure whether it may be relevant.
The WAKE# signal seems to be used only when device is in D3cold.
In D3hot the device sends the PME message directly.

-----------
This is the standard behavior.  Please refer to PCI Express Base
Sepcification Revision 2.0, section 5.3.3.2 Link Wakeup.  In D1, D2
and D3hot state, PCIe device can transit the link from L1 to L0 state,
and send the PME message.  In D3cold, the main link is powered off,
PCIe device will use a STANDARD sideband signal WAKE# to signal wakeup
firstly, then platform (power controller in spec) will power on the
main link for the device, after main link is back to L0, the PME
message is send to root port, pme interrupt is generated.  So in
theory, the wake up process can be divided into platform part (which
power on the main link) and PCIe part (which send PME).
diff mbox series

Patch

diff --git a/drivers/pci/pcie/pme.c b/drivers/pci/pcie/pme.c
index 0dbcf4290..285ea0ed2 100644
--- a/drivers/pci/pcie/pme.c
+++ b/drivers/pci/pcie/pme.c
@@ -432,31 +432,6 @@  static void pcie_pme_remove(struct pcie_device *srv)
 	kfree(get_service_data(srv));
 }
 
-static int pcie_pme_runtime_suspend(struct pcie_device *srv)
-{
-	struct pcie_pme_service_data *data = get_service_data(srv);
-
-	spin_lock_irq(&data->lock);
-	pcie_pme_interrupt_enable(srv->port, false);
-	pcie_clear_root_pme_status(srv->port);
-	data->noirq = true;
-	spin_unlock_irq(&data->lock);
-
-	return 0;
-}
-
-static int pcie_pme_runtime_resume(struct pcie_device *srv)
-{
-	struct pcie_pme_service_data *data = get_service_data(srv);
-
-	spin_lock_irq(&data->lock);
-	pcie_pme_interrupt_enable(srv->port, true);
-	data->noirq = false;
-	spin_unlock_irq(&data->lock);
-
-	return 0;
-}
-
 static struct pcie_port_service_driver pcie_pme_driver = {
 	.name		= "pcie_pme",
 	.port_type	= PCI_EXP_TYPE_ROOT_PORT,
@@ -464,8 +439,8 @@  static struct pcie_port_service_driver pcie_pme_driver = {
 
 	.probe		= pcie_pme_probe,
 	.suspend	= pcie_pme_suspend,
-	.runtime_suspend = pcie_pme_runtime_suspend,
-	.runtime_resume	= pcie_pme_runtime_resume,
+	.runtime_suspend = pcie_pme_suspend,
+	.runtime_resume	= pcie_pme_resume,
 	.resume		= pcie_pme_resume,
 	.remove		= pcie_pme_remove,
 };