| Message ID | 20190226085657.8268-1-kai.heng.feng@canonical.com |
|---|---|
| Headers | show |
| Series | r8169 doesn't get woken up by ethernet cable plugging, no PME generated | expand |
Acked-By: You-Sheng Yang <vicamo.yang@canonical.com>
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
> On Feb 27, 2019, at 21:48, Seth Forshee <seth.forshee@canonical.com> 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
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
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. Applied patch 2 to disco/master-next. Patch 1 was already applied via upstream stable updates. Since this patch is now upstream I modified the commit message to remove linux-next from the "cherry picked from ..." line. Thanks!