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

[Heiner Kallweit <hkallweit1@gmail.com>]

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