Re: [PATCH] r8169: Add quirks to enable ASPM on Dell platforms

[Heiner Kallweit <hkallweit1@gmail.com>]

On 08.12.2024 20:10, Guy Chronister wrote:
> Some non-Dell platforms equipped with r8168h/r8111 have issues with ASPM. It's very hard to fix all known issues in a short time and r8168h/r8111 is not a brand new NIC chip, so introduce the quirk for Dell platforms. It's also easier to track the Dell platform and ask for Realtek's effort.

Which known issues do you refer to? If it's very hard to fix all, then fix some.

Did you verify that ASPM L1.2 doesn't result in missed rx packets even under heavy load?
Check chip hw stats. I have a test system with RTL8168h which works fine up to ASPM L1.1,
but misses rx packets with ASPM L1.2 under heavy load.

> Make the original matching logic more explicit.
> 
> Signed-off-by: Koba Ko <koba.ko@canonical.com>
> Signed-off-by: Timo Aaltonen <timo.aaltonen@canonical.com>
> Signed-off-by: Andrea Righi <andrea.righi@canonical.com>
> Signed-off-by: Guy Chronister <guyc.linux.patches@gmail.com>
> ---
>  drivers/net/ethernet/realtek/r8169_main.c | 83 ++++++++++++++++++++++-
>  1 file changed, 80 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/ethernet/realtek/r8169_main.c b/drivers/net/ethernet/realtek/r8169_main.c
> index 6934bdee2a91..3c1cf704492f 100644
> --- a/drivers/net/ethernet/realtek/r8169_main.c
> +++ b/drivers/net/ethernet/realtek/r8169_main.c
> @@ -15,6 +15,7 @@
>  #include <linux/etherdevice.h>
>  #include <linux/clk.h>
>  #include <linux/delay.h>
> +#include <linux/dmi.h>
>  #include <linux/ethtool.h>
>  #include <linux/hwmon.h>
>  #include <linux/phy.h>
> @@ -5322,13 +5323,89 @@ static void rtl_init_mac_address(struct rtl8169_private *tp)
>  	rtl_rar_set(tp, mac_addr);
>  }
>  
> +static bool rtl_aspm_dell_workaround(struct rtl8169_private *tp)
> +{
> +	static const struct dmi_system_id sysids[] = {
> +		{
> +			.ident = "Dell",
> +			.matches = {
> +				DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
> +				DMI_MATCH(DMI_PRODUCT_NAME, "Vostro 16 5640"),
> +				DMI_MATCH(DMI_PRODUCT_SKU, "0CA0"),
> +			},
> +		},
> +		{
> +			.ident = "Dell",
> +			.matches = {
> +				DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
> +				DMI_MATCH(DMI_PRODUCT_NAME, "Vostro 14 3440"),
> +				DMI_MATCH(DMI_PRODUCT_SKU, "0CA5"),
> +			},
> +		},
> +		{
> +			.ident = "Dell",
> +			.matches = {
> +				DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
> +				DMI_MATCH(DMI_PRODUCT_NAME, "Vostro 14 3440"),
> +				DMI_MATCH(DMI_PRODUCT_SKU, "0CA6"),
> +			},
> +		},
> +		{
> +			.ident = "Dell",
> +			.matches = {
> +				DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
> +				DMI_MATCH(DMI_PRODUCT_NAME, "Latitude 3450"),
> +				DMI_MATCH(DMI_PRODUCT_SKU, "0C99"),
> +			},
> +		},
> +		{
> +			.ident = "Dell",
> +			.matches = {
> +				DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
> +				DMI_MATCH(DMI_PRODUCT_NAME, "Latitude 3450"),
> +				DMI_MATCH(DMI_PRODUCT_SKU, "0C97"),
> +			},
> +		},
> +		{
> +			.ident = "Dell",
> +			.matches = {
> +				DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
> +				DMI_MATCH(DMI_PRODUCT_NAME, "Latitude 3550"),
> +				DMI_MATCH(DMI_PRODUCT_SKU, "0C9A"),
> +			},
> +		},
> +		{
> +			.ident = "Dell",
> +			.matches = {
> +				DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
> +				DMI_MATCH(DMI_PRODUCT_NAME, "Latitude 3550"),
> +				DMI_MATCH(DMI_PRODUCT_SKU, "0C98"),
> +			},
> +		},
> +		{}
> +	};
> +
> +	if (tp->mac_version == RTL_GIGA_MAC_VER_46 && dmi_check_system(sysids))
> +		return true;
> +
> +	return false;
> +}
> +
>  /* register is set if system vendor successfully tested ASPM 1.2 */
>  static bool rtl_aspm_is_safe(struct rtl8169_private *tp)
>  {
> -	if (tp->mac_version >= RTL_GIGA_MAC_VER_61 &&
> -	    r8168_mac_ocp_read(tp, 0xc0b2) & 0xf)
> +	 /* definition of 0xc0b2,
> +	  * 0: L1
> +	  * 1: ASPM L1.0
> +	  * 2: ASPM L0s
> +	  * 3: CLKEREQ
> +	  * 4-7: Reserved
> +	  */
> +	if ((tp->mac_version >= RTL_GIGA_MAC_VER_61 &&
> +		 (r8168_mac_ocp_read(tp, 0xc0b2) & 0x0F)) ||
> +		rtl_aspm_dell_workaround(tp)) {
>  		return true;
> -
> +	}
>  	return false;
>  }
>  

This approach doesn't scale. I don't want to end up with hundreds of such dmi checks.
I understand that the driver works as-is, you just want to enable ASPM per default
on these systems. You have two options to enable ASPM:

1. Check with Realtek whether this register 0xc0b2 (or an equivalent) can be used on
   RTL8168h and these Dell systems too. If yes, may require a BIOS update to enable
   the functionality.
2. (Re-)enable ASPM states from user space: Use attributes under /sys/class/net/<if>/device/link