Messages in this thread | | | Subject | Re: [PATCH] r8169: only disable ASPM L1.1 support, instead of disabling them all | From | Heiner Kallweit <> | Date | Wed, 18 Mar 2020 12:09:14 +0100 |
| |
On 18.03.2020 02:45, AceLan Kao wrote: > The issues which have been seen by enabling ASPM support are from the > BIOS that enables the ASPM L1.1 support on the device. It leads to some > strange behaviors when the device enter L1.1 state. > So, we don't have to disable ASPM support entriely, just disable L1.1 > state, that fixes the issues and also has good power management. >
Meanwhile you can use sysfs to re-enable selected ASPM states, see entries in "link" directory under the PCI device (provided that BIOS allows OS to control ASPM). This allows users with mobile devices w/o the ASPM issue to benefit from the ASPM power savings.
There are ~ 50 RTL8168 chip versions, used on different platforms and dozens of consumer mainboards (with more or less buggy BIOS versions). This leaves a good chance that some users may face issues with L0s/L1 enabled. And unfortunately the symptoms of ASPM issues haven't always been obvious, sometimes just the performance was reduced. Having said that I'd prefer to keep ASPM an opt-in feature.
Heiner
> Signed-off-by: AceLan Kao <acelan.kao@canonical.com> > --- > drivers/net/ethernet/realtek/r8169_main.c | 7 +++---- > 1 file changed, 3 insertions(+), 4 deletions(-) > > diff --git a/drivers/net/ethernet/realtek/r8169_main.c b/drivers/net/ethernet/realtek/r8169_main.c > index a2168a14794c..b52680e7323b 100644 > --- a/drivers/net/ethernet/realtek/r8169_main.c > +++ b/drivers/net/ethernet/realtek/r8169_main.c > @@ -5473,11 +5473,10 @@ static int rtl_init_one(struct pci_dev *pdev, const struct pci_device_id *ent) > if (rc) > return rc; > > - /* Disable ASPM completely as that cause random device stop working > - * problems as well as full system hangs for some PCIe devices users. > + /* r8169 suppots ASPM L0 and L1 well, and doesn't support L1.1, > + * so disable ASPM L1.1 only. > */ > - rc = pci_disable_link_state(pdev, PCIE_LINK_STATE_L0S | > - PCIE_LINK_STATE_L1); > + rc = pci_disable_link_state(pdev, PCIE_LINK_STATE_L1_1); > tp->aspm_manageable = !rc; > > /* enable device (incl. PCI PM wakeup and hotplug setup) */ >
| |