[SRU] [Unstable/OEM-B] [PATCH 0/2] r8169 doesn't get woken up by ethernet cable plugging, no PME generated

classic Classic list List threaded Threaded
7 messages Options
Reply | Threaded
Open this post in threaded view
|

[SRU] [Unstable/OEM-B] [PATCH 0/2] r8169 doesn't get woken up by ethernet cable plugging, no PME generated

Kai-Heng Feng
BugLink: https://bugs.launchpad.net/bugs/1817676

[Impact]
Once r8169 (Realtek ethernet) and its root port enter to D3, plugging
network cable doesn't wake r8169 up.

[Fix]
Revert "PCI/PME: Implement runtime PM callbacks".
Quote the original commit message:
"
The commit tried to prevent root port waking up from D3cold immediately but
looks like disabing root port PME interrupt is not the right way to fix
that issue so revert it now.  The patch following proposes an alternative
solution to that issue.
"

[Test]
After applying the fix, plugging the ethernet cable can generate PME IRQ
correctly, hence waking up r8169.

[Regression Potential]
Medium. The commits are relative new, rely on some PCI detail that I am
not familiar with (PCI spec isn't publicly available).
Fortunately we only need this fix for Unstable/OEM-B, so it won't hit
most users if any regression happens.

Mika Westerberg (2):
  Revert "PCI/PME: Implement runtime PM callbacks"
  PCI: pciehp: Disable Data Link Layer State Changed event on suspend

 drivers/pci/hotplug/pciehp_hpc.c | 17 +++++++++++++++--
 drivers/pci/pcie/pme.c           | 27 ---------------------------
 2 files changed, 15 insertions(+), 29 deletions(-)

--
2.17.1


--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|

[PATCH 1/2] Revert "PCI/PME: Implement runtime PM callbacks"

Kai-Heng Feng
From: Mika Westerberg <[hidden email]>

BugLink: https://bugs.launchpad.net/bugs/1817676

This reverts commit 0e157e52860441cb26051f131dd0b5ae3187a07b.

Heiner reported that the commit in question prevents his network adapter
from triggering PME and waking up when network cable is plugged.

The commit tried to prevent root port waking up from D3cold immediately but
looks like disabing root port PME interrupt is not the right way to fix
that issue so revert it now.  The patch following proposes an alternative
solution to that issue.

Link: https://bugzilla.kernel.org/show_bug.cgi?id=202103
Fixes: 0e157e528604 ("PCI/PME: Implement runtime PM callbacks")
Reported-by: Heiner Kallweit <[hidden email]>
Tested-by: Heiner Kallweit <[hidden email]>
Signed-off-by: Mika Westerberg <[hidden email]>
Signed-off-by: Bjorn Helgaas <[hidden email]>
Acked-by: Rafael J. Wysocki <[hidden email]>
CC: [hidden email] # v4.20+
(cherry picked from commit c528f7bd362b097eeeafa6fbbeccd9750b79c7ba linux-next)
Signed-off-by: Kai-Heng Feng <[hidden email]>
---
 drivers/pci/pcie/pme.c | 27 ---------------------------
 1 file changed, 27 deletions(-)

diff --git a/drivers/pci/pcie/pme.c b/drivers/pci/pcie/pme.c
index c3fd490c8cb7..d5b2916f4ce3 100644
--- a/drivers/pci/pcie/pme.c
+++ b/drivers/pci/pcie/pme.c
@@ -435,31 +435,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,
@@ -467,8 +442,6 @@ 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,
  .resume = pcie_pme_resume,
  .remove = pcie_pme_remove,
 };
--
2.17.1


--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|

[PATCH 2/2] PCI: pciehp: Disable Data Link Layer State Changed event on suspend

Kai-Heng Feng
In reply to this post by Kai-Heng Feng
From: Mika Westerberg <[hidden email]>

BugLink: https://bugs.launchpad.net/bugs/1817676

Commit 0e157e528604 ("PCI/PME: Implement runtime PM callbacks") tried to
solve an issue where the hierarchy immediately wakes up when it is
transitioned into D3cold.  However, it turns out to prevent PME
propagation on some systems that do not support D3cold.

I looked more closely at what might cause the immediate wakeup.  It happens
when the ACPI power resource of the root port is turned off.  The AML code
associated with the _OFF() method of the ACPI power resource starts a PCIe
L2/L3 Ready transition and waits for it to complete.  Right after the L2/L3
Ready transition is started the root port receives a PME from the
downstream port.

The simplest hierarchy where this happens looks like this:

  00:1d.0 PCIe Root Port
    ^
    |
    v
    05:00.0 PCIe switch #1 upstream port
      06:01.0 PCIe switch #1 downstream hotplug port
        ^
        |
        v
        08:00.0 PCIe switch #2 upstream port

It seems that the PCIe link between the two switches, before
PME_Turn_Off/PME_TO_Ack is complete for the whole hierarchy, goes
inactive and triggers PME towards the root port bringing it back to D0.
The L2/L3 Ready sequence is described in PCIe r4.0 spec sections 5.2 and
5.3.3 but unfortunately they do not state what happens if DLLSCE is
enabled during the sequence.

Disabling Data Link Layer State Changed event (DLLSCE) seems to prevent
the issue and still allows the downstream hotplug port to notice when a
device is plugged/unplugged.

Link: https://bugzilla.kernel.org/show_bug.cgi?id=202593
Fixes: 0e157e528604 ("PCI/PME: Implement runtime PM callbacks")
Signed-off-by: Mika Westerberg <[hidden email]>
Signed-off-by: Bjorn Helgaas <[hidden email]>
Reviewed-by: Rafael J. Wysocki <[hidden email]>
CC: [hidden email] # v4.20+
(cherry picked from commit bbe54ea5330d828cc396d451c0e1e5c3f9764c1e linux-next)
Signed-off-by: Kai-Heng Feng <[hidden email]>
---
 drivers/pci/hotplug/pciehp_hpc.c | 17 +++++++++++++++--
 1 file changed, 15 insertions(+), 2 deletions(-)

diff --git a/drivers/pci/hotplug/pciehp_hpc.c b/drivers/pci/hotplug/pciehp_hpc.c
index 2b93764f68f7..6874ab24f926 100644
--- a/drivers/pci/hotplug/pciehp_hpc.c
+++ b/drivers/pci/hotplug/pciehp_hpc.c
@@ -752,12 +752,25 @@ void pcie_clear_hotplug_events(struct controller *ctrl)
 
 void pcie_enable_interrupt(struct controller *ctrl)
 {
- pcie_write_cmd(ctrl, PCI_EXP_SLTCTL_HPIE, PCI_EXP_SLTCTL_HPIE);
+ u16 mask;
+
+ mask = PCI_EXP_SLTCTL_HPIE | PCI_EXP_SLTCTL_DLLSCE;
+ pcie_write_cmd(ctrl, mask, mask);
 }
 
 void pcie_disable_interrupt(struct controller *ctrl)
 {
- pcie_write_cmd(ctrl, 0, PCI_EXP_SLTCTL_HPIE);
+ u16 mask;
+
+ /*
+ * Mask hot-plug interrupt to prevent it triggering immediately
+ * when the link goes inactive (we still get PME when any of the
+ * enabled events is detected). Same goes with Link Layer State
+ * changed event which generates PME immediately when the link goes
+ * inactive so mask it as well.
+ */
+ mask = PCI_EXP_SLTCTL_HPIE | PCI_EXP_SLTCTL_DLLSCE;
+ pcie_write_cmd(ctrl, 0, mask);
 }
 
 /*
--
2.17.1


--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|

ACK: [SRU] [Unstable/OEM-B] [PATCH 0/2] r8169 doesn't get woken up by ethernet cable plugging, no PME generated

You-Sheng Yang
In reply to this post by Kai-Heng Feng
Acked-By: You-Sheng Yang <[hidden email]>

--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team

signature.asc (499 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [SRU] [Unstable/OEM-B] [PATCH 0/2] r8169 doesn't get woken up by ethernet cable plugging, no PME generated

Seth Forshee
In reply to this post by Kai-Heng Feng
On Tue, Feb 26, 2019 at 04:56:55PM +0800, Kai-Heng Feng wrote:

> BugLink: https://bugs.launchpad.net/bugs/1817676
>
> [Impact]
> Once r8169 (Realtek ethernet) and its root port enter to D3, plugging
> network cable doesn't wake r8169 up.
>
> [Fix]
> Revert "PCI/PME: Implement runtime PM callbacks".
> Quote the original commit message:
> "
> The commit tried to prevent root port waking up from D3cold immediately but
> looks like disabing root port PME interrupt is not the right way to fix
> that issue so revert it now.  The patch following proposes an alternative
> solution to that issue.
> "
>
> [Test]
> After applying the fix, plugging the ethernet cable can generate PME IRQ
> correctly, hence waking up r8169.
>
> [Regression Potential]
> Medium. The commits are relative new, rely on some PCI detail that I am
> not familiar with (PCI spec isn't publicly available).
> Fortunately we only need this fix for Unstable/OEM-B, so it won't hit
> most users if any regression happens.

I think it would be safer to apply only the revert if the only problem
we're trying to fix is the wakeup on cable plug, since that commit was
new in 4.20 anyhow. If you think we also need the second commit, please
explain why and let me know what kind of testing you've done with it.

Thanks,
Seth

--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|

Re: [SRU] [Unstable/OEM-B] [PATCH 0/2] r8169 doesn't get woken up by ethernet cable plugging, no PME generated

Kai-Heng Feng


> On Feb 27, 2019, at 21:48, Seth Forshee <[hidden email]> wrote:
>
> On Tue, Feb 26, 2019 at 04:56:55PM +0800, Kai-Heng Feng wrote:
>> BugLink: https://bugs.launchpad.net/bugs/1817676
>>
>> [Impact]
>> Once r8169 (Realtek ethernet) and its root port enter to D3, plugging
>> network cable doesn't wake r8169 up.
>>
>> [Fix]
>> Revert "PCI/PME: Implement runtime PM callbacks".
>> Quote the original commit message:
>> "
>> The commit tried to prevent root port waking up from D3cold immediately but
>> looks like disabing root port PME interrupt is not the right way to fix
>> that issue so revert it now.  The patch following proposes an alternative
>> solution to that issue.
>> "
>>
>> [Test]
>> After applying the fix, plugging the ethernet cable can generate PME IRQ
>> correctly, hence waking up r8169.
>>
>> [Regression Potential]
>> Medium. The commits are relative new, rely on some PCI detail that I am
>> not familiar with (PCI spec isn't publicly available).
>> Fortunately we only need this fix for Unstable/OEM-B, so it won't hit
>> most users if any regression happens.
>
> I think it would be safer to apply only the revert if the only problem
> we're trying to fix is the wakeup on cable plug, since that commit was
> new in 4.20 anyhow. If you think we also need the second commit, please
> explain why and let me know what kind of testing you've done with it.

Yes, the second commit is needed.

The reverted commit "PCI/PME: Implement runtime PM callbacks” is an
attempt to solve a TBT issue on X1 Carbon 6th Gen [1].

Patch [2/2] is a replacement for the reverted fix.
Without it, the native TBT device will be in a D0/D3cold loop [2]:

[  263.418400] pcieport 0000:00:1d.0: power state changed by ACPI to D3cold
[  264.919693] pcieport 0000:00:1d.0: power state changed by ACPI to D0

[  285.114443] pcieport 0000:00:1d.0: power state changed by ACPI to D3cold
[  286.614872] pcieport 0000:00:1d.0: power state changed by ACPI to D0

[  306.682371] pcieport 0000:00:1d.0: power state changed by ACPI to D3cold
[  308.183711] pcieport 0000:00:1d.0: power state changed by ACPI to D0

I think the dmesg itself is a good justification for picking patch [2/2].

Right now I can’t conduct any meaningful test because the one I have
(Dell TB16) has a device that prevents the full hierarchy to enter D3cold,
so the issue can't be reproduced here.

[1] https://bugzilla.kernel.org/show_bug.cgi?id=202593
[2] https://bugzilla.kernel.org/attachment.cgi?id=281163

Kai-Heng

>
> Thanks,
> Seth


--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|

APPLIED/OEM: Re: [SRU] [Unstable/OEM-B] [PATCH 0/2] r8169 doesn't get woken up by ethernet cable plugging, no PME generated

Timo Aaltonen-6
In reply to this post by Kai-Heng Feng
On 26.2.2019 10.56, Kai-Heng Feng wrote:

> BugLink: https://bugs.launchpad.net/bugs/1817676
>
> [Impact]
> Once r8169 (Realtek ethernet) and its root port enter to D3, plugging
> network cable doesn't wake r8169 up.
>
> [Fix]
> Revert "PCI/PME: Implement runtime PM callbacks".
> Quote the original commit message:
> "
> The commit tried to prevent root port waking up from D3cold immediately but
> looks like disabing root port PME interrupt is not the right way to fix
> that issue so revert it now.  The patch following proposes an alternative
> solution to that issue.
> "
>
> [Test]
> After applying the fix, plugging the ethernet cable can generate PME IRQ
> correctly, hence waking up r8169.
>
> [Regression Potential]
> Medium. The commits are relative new, rely on some PCI detail that I am
> not familiar with (PCI spec isn't publicly available).
> Fortunately we only need this fix for Unstable/OEM-B, so it won't hit
> most users if any regression happens.
>
> Mika Westerberg (2):
>   Revert "PCI/PME: Implement runtime PM callbacks"
>   PCI: pciehp: Disable Data Link Layer State Changed event on suspend
>
>  drivers/pci/hotplug/pciehp_hpc.c | 17 +++++++++++++++--
>  drivers/pci/pcie/pme.c           | 27 ---------------------------
>  2 files changed, 15 insertions(+), 29 deletions(-)

applied to oem-next, thanks

--
t

--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team