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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <78aa4db2-6dec-e23c-03f4-f76577de756f@gmail.com>
Date:   Wed, 18 Mar 2020 12:09:14 +0100
From:   Heiner Kallweit <hkallweit1@...il.com>
To:     AceLan Kao <acelan.kao@...onical.com>,
        Realtek linux nic maintainers <nic_swsd@...ltek.com>,
        "David S. Miller" <davem@...emloft.net>, netdev@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH] r8169: only disable ASPM L1.1 support, instead of
 disabling them all

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@...onical.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) */
>