r8169: only disable ASPM L1.1 support, instead of disabling them all
diff mbox series

Message ID 20200318014548.14547-1-acelan.kao@canonical.com
State New
Headers show
Series
  • r8169: only disable ASPM L1.1 support, instead of disabling them all
Related show

Commit Message

AceLan Kao March 18, 2020, 1:45 a.m. UTC
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.

Signed-off-by: AceLan Kao <acelan.kao@canonical.com>
---
 drivers/net/ethernet/realtek/r8169_main.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

Comments

Heiner Kallweit March 18, 2020, 11:09 a.m. UTC | #1
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) */
>
David Miller March 22, 2020, 3:20 a.m. UTC | #2
From: Heiner Kallweit <hkallweit1@gmail.com>
Date: Wed, 18 Mar 2020 12:09:14 +0100

> Having said that I'd prefer to keep ASPM an opt-in feature.

Agreed, too much stuff broke randomly and in hard to detect ways.

Patch
diff mbox series

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) */