[PATCH] r8169: only disable ASPM L1.1 support, instead of disabling them all

[PATCH] r8169: only disable ASPM L1.1 support, instead of disabling them all

[AceLan Kao]

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(-)

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

Re: [PATCH] r8169: only disable ASPM L1.1 support, instead of disabling them all

[Heiner Kallweit]

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

Re: [PATCH] r8169: only disable ASPM L1.1 support, instead of disabling them all

[David Miller]

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.