* [PATCH] r8169: enable ASPM on Dell platforms @ 2025-09-12 7:29 Chia-Lin Kao (AceLan) 2025-09-12 15:30 ` Heiner Kallweit 0 siblings, 1 reply; 6+ messages in thread From: Chia-Lin Kao (AceLan) @ 2025-09-12 7:29 UTC (permalink / raw) To: Heiner Kallweit, nic_swsd, Andrew Lunn, David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni, netdev, linux-kernel, Wang, Crag, Chen, Alan, Alex Shen@Dell Enable PCIe ASPM for RTL8169 NICs on Dell platforms that have been verified to work reliably with this power management feature. The r8169 driver traditionally disables ASPM to prevent random link failures and system hangs on problematic hardware. Dell has validated these product families to work correctly with RTL NIC ASPM and commits to addressing any ASPM-related issues with RTL hardware in collaboration with Realtek. This change enables ASPM for the following Dell product families: - Alienware - Dell Laptops/Pro Laptops/Pro Max Laptops - Dell Desktops/Pro Desktops/Pro Max Desktops - Dell Pro Rugged Laptops Signed-off-by: Chia-Lin Kao (AceLan) <acelan.kao@canonical.com> --- drivers/net/ethernet/realtek/r8169_main.c | 29 +++++++++++++++++++++++ 1 file changed, 29 insertions(+) diff --git a/drivers/net/ethernet/realtek/r8169_main.c b/drivers/net/ethernet/realtek/r8169_main.c index 9c601f271c02..63e83cf071de 100644 --- a/drivers/net/ethernet/realtek/r8169_main.c +++ b/drivers/net/ethernet/realtek/r8169_main.c @@ -5366,6 +5366,32 @@ static void rtl_init_mac_address(struct rtl8169_private *tp) rtl_rar_set(tp, mac_addr); } +bool rtl_aspm_new_dell_platforms(void) +{ + const char *family = dmi_get_system_info(DMI_PRODUCT_FAMILY); + static const char * const dell_product_families[] = { + "Alienware", + "Dell Laptops", + "Dell Pro Laptops", + "Dell Pro Max Laptops", + "Dell Desktops", + "Dell Pro Desktops", + "Dell Pro Max Desktops", + "Dell Pro Rugged Laptops" + }; + int i; + + if (!family) + return false; + + for (i = 0; i < ARRAY_SIZE(dell_product_families); i++) { + if (str_has_prefix(family, dell_product_families[i])) + 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) { @@ -5373,6 +5399,9 @@ static bool rtl_aspm_is_safe(struct rtl8169_private *tp) r8168_mac_ocp_read(tp, 0xc0b2) & 0xf) return true; + if (rtl_aspm_new_dell_platforms()) + return true; + return false; } -- 2.43.0 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] r8169: enable ASPM on Dell platforms 2025-09-12 7:29 [PATCH] r8169: enable ASPM on Dell platforms Chia-Lin Kao (AceLan) @ 2025-09-12 15:30 ` Heiner Kallweit 2025-09-15 1:37 ` Chia-Lin Kao (AceLan) 2025-09-22 16:41 ` Bjorn Helgaas 0 siblings, 2 replies; 6+ messages in thread From: Heiner Kallweit @ 2025-09-12 15:30 UTC (permalink / raw) To: Chia-Lin Kao (AceLan), nic_swsd, Andrew Lunn, David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni, netdev, linux-kernel, Wang, Crag, Chen, Alan, Alex Shen@Dell On 9/12/2025 9:29 AM, Chia-Lin Kao (AceLan) wrote: > Enable PCIe ASPM for RTL8169 NICs on Dell platforms that have been > verified to work reliably with this power management feature. The > r8169 driver traditionally disables ASPM to prevent random link > failures and system hangs on problematic hardware. > > Dell has validated these product families to work correctly with > RTL NIC ASPM and commits to addressing any ASPM-related issues > with RTL hardware in collaboration with Realtek. > > This change enables ASPM for the following Dell product families: > - Alienware > - Dell Laptops/Pro Laptops/Pro Max Laptops > - Dell Desktops/Pro Desktops/Pro Max Desktops > - Dell Pro Rugged Laptops > I'd like to avoid DMI-based whitelists in kernel code. If more system vendors do it the same way, then this becomes hard to maintain. There is already a mechanism for vendors to flag that they successfully tested ASPM. See c217ab7a3961 ("r8169: enable ASPM L1.2 if system vendor flags it as safe"). Last but not least ASPM can be (re-)enabled from userspace, using sysfs. > Signed-off-by: Chia-Lin Kao (AceLan) <acelan.kao@canonical.com> > --- > drivers/net/ethernet/realtek/r8169_main.c | 29 +++++++++++++++++++++++ > 1 file changed, 29 insertions(+) > > diff --git a/drivers/net/ethernet/realtek/r8169_main.c b/drivers/net/ethernet/realtek/r8169_main.c > index 9c601f271c02..63e83cf071de 100644 > --- a/drivers/net/ethernet/realtek/r8169_main.c > +++ b/drivers/net/ethernet/realtek/r8169_main.c > @@ -5366,6 +5366,32 @@ static void rtl_init_mac_address(struct rtl8169_private *tp) > rtl_rar_set(tp, mac_addr); > } > > +bool rtl_aspm_new_dell_platforms(void) > +{ > + const char *family = dmi_get_system_info(DMI_PRODUCT_FAMILY); > + static const char * const dell_product_families[] = { > + "Alienware", > + "Dell Laptops", > + "Dell Pro Laptops", > + "Dell Pro Max Laptops", > + "Dell Desktops", > + "Dell Pro Desktops", > + "Dell Pro Max Desktops", > + "Dell Pro Rugged Laptops" > + }; > + int i; > + > + if (!family) > + return false; > + > + for (i = 0; i < ARRAY_SIZE(dell_product_families); i++) { > + if (str_has_prefix(family, dell_product_families[i])) > + 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) > { > @@ -5373,6 +5399,9 @@ static bool rtl_aspm_is_safe(struct rtl8169_private *tp) > r8168_mac_ocp_read(tp, 0xc0b2) & 0xf) > return true; > > + if (rtl_aspm_new_dell_platforms()) > + return true; > + > return false; > } > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] r8169: enable ASPM on Dell platforms 2025-09-12 15:30 ` Heiner Kallweit @ 2025-09-15 1:37 ` Chia-Lin Kao (AceLan) 2025-09-15 19:25 ` Heiner Kallweit 2025-09-22 16:41 ` Bjorn Helgaas 1 sibling, 1 reply; 6+ messages in thread From: Chia-Lin Kao (AceLan) @ 2025-09-15 1:37 UTC (permalink / raw) To: Heiner Kallweit Cc: nic_swsd, Andrew Lunn, David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni, netdev, linux-kernel, Wang, Crag, Chen, Alan, Alex Shen@Dell On Fri, Sep 12, 2025 at 05:30:52PM +0200, Heiner Kallweit wrote: > On 9/12/2025 9:29 AM, Chia-Lin Kao (AceLan) wrote: > > Enable PCIe ASPM for RTL8169 NICs on Dell platforms that have been > > verified to work reliably with this power management feature. The > > r8169 driver traditionally disables ASPM to prevent random link > > failures and system hangs on problematic hardware. > > > > Dell has validated these product families to work correctly with > > RTL NIC ASPM and commits to addressing any ASPM-related issues > > with RTL hardware in collaboration with Realtek. > > > > This change enables ASPM for the following Dell product families: > > - Alienware > > - Dell Laptops/Pro Laptops/Pro Max Laptops > > - Dell Desktops/Pro Desktops/Pro Max Desktops > > - Dell Pro Rugged Laptops > > > I'd like to avoid DMI-based whitelists in kernel code. If more system > vendors do it the same way, then this becomes hard to maintain. I totally understand your point; I also don’t like constantly adding DMI info to the list. But this list isn’t for a single product name, it’s a product family that covers a series of products, and it probably won’t change anytime soon. > There is already a mechanism for vendors to flag that they successfully > tested ASPM. See c217ab7a3961 ("r8169: enable ASPM L1.2 if system vendor > flags it as safe"). Right, but writing the flag is not applicable for Dell manufacturing processes. > Last but not least ASPM can be (re-)enabled from userspace, using sysfs. That doesn't sound like a good solution to push the list to userspace. Dell has already been working with Canonical for more than a decade to ship their products with r8169 ASPM enabled. Dell has also had lengthy discussions with Realtek to have this feature enabled by default in the r8169 driver. I think this is a good opportunity for Dell to work with Realtek to spot bugs and refine the r8169 driver. BTW, I found the dmi.h header file is missing in the patch, so I submitted a v2 patch. https://lore.kernel.org/lkml/20250915013555.365230-1-acelan.kao@canonical.com/T/#u > > > Signed-off-by: Chia-Lin Kao (AceLan) <acelan.kao@canonical.com> > > --- > > drivers/net/ethernet/realtek/r8169_main.c | 29 +++++++++++++++++++++++ > > 1 file changed, 29 insertions(+) > > > > diff --git a/drivers/net/ethernet/realtek/r8169_main.c b/drivers/net/ethernet/realtek/r8169_main.c > > index 9c601f271c02..63e83cf071de 100644 > > --- a/drivers/net/ethernet/realtek/r8169_main.c > > +++ b/drivers/net/ethernet/realtek/r8169_main.c > > @@ -5366,6 +5366,32 @@ static void rtl_init_mac_address(struct rtl8169_private *tp) > > rtl_rar_set(tp, mac_addr); > > } > > > > +bool rtl_aspm_new_dell_platforms(void) > > +{ > > + const char *family = dmi_get_system_info(DMI_PRODUCT_FAMILY); > > + static const char * const dell_product_families[] = { > > + "Alienware", > > + "Dell Laptops", > > + "Dell Pro Laptops", > > + "Dell Pro Max Laptops", > > + "Dell Desktops", > > + "Dell Pro Desktops", > > + "Dell Pro Max Desktops", > > + "Dell Pro Rugged Laptops" > > + }; > > + int i; > > + > > + if (!family) > > + return false; > > + > > + for (i = 0; i < ARRAY_SIZE(dell_product_families); i++) { > > + if (str_has_prefix(family, dell_product_families[i])) > > + 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) > > { > > @@ -5373,6 +5399,9 @@ static bool rtl_aspm_is_safe(struct rtl8169_private *tp) > > r8168_mac_ocp_read(tp, 0xc0b2) & 0xf) > > return true; > > > > + if (rtl_aspm_new_dell_platforms()) > > + return true; > > + > > return false; > > } > > > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] r8169: enable ASPM on Dell platforms 2025-09-15 1:37 ` Chia-Lin Kao (AceLan) @ 2025-09-15 19:25 ` Heiner Kallweit 2025-09-16 20:35 ` Bjorn Helgaas 0 siblings, 1 reply; 6+ messages in thread From: Heiner Kallweit @ 2025-09-15 19:25 UTC (permalink / raw) To: Chia-Lin Kao (AceLan), nic_swsd, Andrew Lunn, David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni, netdev, linux-kernel, Wang, Crag, Chen, Alan, Alex Shen@Dell On 9/15/2025 3:37 AM, Chia-Lin Kao (AceLan) wrote: > On Fri, Sep 12, 2025 at 05:30:52PM +0200, Heiner Kallweit wrote: >> On 9/12/2025 9:29 AM, Chia-Lin Kao (AceLan) wrote: >>> Enable PCIe ASPM for RTL8169 NICs on Dell platforms that have been >>> verified to work reliably with this power management feature. The >>> r8169 driver traditionally disables ASPM to prevent random link >>> failures and system hangs on problematic hardware. >>> >>> Dell has validated these product families to work correctly with >>> RTL NIC ASPM and commits to addressing any ASPM-related issues >>> with RTL hardware in collaboration with Realtek. >>> >>> This change enables ASPM for the following Dell product families: >>> - Alienware >>> - Dell Laptops/Pro Laptops/Pro Max Laptops >>> - Dell Desktops/Pro Desktops/Pro Max Desktops >>> - Dell Pro Rugged Laptops >>> >> I'd like to avoid DMI-based whitelists in kernel code. If more system >> vendors do it the same way, then this becomes hard to maintain. > I totally understand your point; I also don’t like constantly adding DMI > info to the list. But this list isn’t for a single product name, it’s a > product family that covers a series of products, and it probably won’t > change anytime soon. > >> There is already a mechanism for vendors to flag that they successfully >> tested ASPM. See c217ab7a3961 ("r8169: enable ASPM L1.2 if system vendor >> flags it as safe"). > Right, but writing the flag is not applicable for Dell manufacturing > processes. > Can you elaborate on why this doesn't work for Dell? >> Last but not least ASPM can be (re-)enabled from userspace, using sysfs. > That doesn't sound like a good solution to push the list to userspace. > > Dell has already been working with Canonical for more than a decade to > ship their products with r8169 ASPM enabled. Dell has also had lengthy > discussions with Realtek to have this feature enabled by default in the > r8169 driver. I think this is a good opportunity for Dell to work with > Realtek to spot bugs and refine the r8169 driver. > One more option to avoid having to change kernel code with each new and successfully ASPM-tested system family from any system vendor: We could define a device property which states that ASPM has been successfully tested on a system. This device property could be set via device tree or via ACPI. Then a simple device_property_present() in the driver would be sufficient. A device property would also have the advantage that vendors can control behavior per device, not only per device family. An ACPI device property could be rolled out via normal BIOS update for existing systems. See also: https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/tree/Documentation/firmware-guide/acpi/DSD-properties-rules.rst?h=v6.16.7 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] r8169: enable ASPM on Dell platforms 2025-09-15 19:25 ` Heiner Kallweit @ 2025-09-16 20:35 ` Bjorn Helgaas 0 siblings, 0 replies; 6+ messages in thread From: Bjorn Helgaas @ 2025-09-16 20:35 UTC (permalink / raw) To: Heiner Kallweit Cc: Chia-Lin Kao (AceLan), nic_swsd, Andrew Lunn, David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni, netdev, linux-kernel, Wang, Crag, Chen, Alan, Alex Shen@Dell, linux-pci [+cc linux-pci] On Mon, Sep 15, 2025 at 09:25:39PM +0200, Heiner Kallweit wrote: > On 9/15/2025 3:37 AM, Chia-Lin Kao (AceLan) wrote: > > On Fri, Sep 12, 2025 at 05:30:52PM +0200, Heiner Kallweit wrote: > >> On 9/12/2025 9:29 AM, Chia-Lin Kao (AceLan) wrote: > >>> Enable PCIe ASPM for RTL8169 NICs on Dell platforms that have been > >>> verified to work reliably with this power management feature. The > >>> r8169 driver traditionally disables ASPM to prevent random link > >>> failures and system hangs on problematic hardware. > >>> > >>> Dell has validated these product families to work correctly with > >>> RTL NIC ASPM and commits to addressing any ASPM-related issues > >>> with RTL hardware in collaboration with Realtek. > >>> > >>> This change enables ASPM for the following Dell product families: > >>> - Alienware > >>> - Dell Laptops/Pro Laptops/Pro Max Laptops > >>> - Dell Desktops/Pro Desktops/Pro Max Desktops > >>> - Dell Pro Rugged Laptops > >>> > >> I'd like to avoid DMI-based whitelists in kernel code. If more system > >> vendors do it the same way, then this becomes hard to maintain. > > > > I totally understand your point; I also don’t like constantly adding DMI > > info to the list. But this list isn’t for a single product name, it’s a > > product family that covers a series of products, and it probably won’t > > change anytime soon. > > > >> There is already a mechanism for vendors to flag that they successfully > >> tested ASPM. See c217ab7a3961 ("r8169: enable ASPM L1.2 if system vendor > >> flags it as safe"). > > > > Right, but writing the flag is not applicable for Dell manufacturing > > processes. > > > Can you elaborate on why this doesn't work for Dell? > > >> Last but not least ASPM can be (re-)enabled from userspace, using sysfs. > > > > That doesn't sound like a good solution to push the list to userspace. > > > > Dell has already been working with Canonical for more than a decade to > > ship their products with r8169 ASPM enabled. Dell has also had lengthy > > discussions with Realtek to have this feature enabled by default in the > > r8169 driver. I think this is a good opportunity for Dell to work with > > Realtek to spot bugs and refine the r8169 driver. > > One more option to avoid having to change kernel code with each new > and successfully ASPM-tested system family from any system vendor: > > We could define a device property which states that ASPM has been > successfully tested on a system. This device property could be set > via device tree or via ACPI. Then a simple device_property_present() > in the driver would be sufficient. > A device property would also have the advantage that vendors can > control behavior per device, not only per device family. > An ACPI device property could be rolled out via normal BIOS update > for existing systems. > > See also: > https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/tree/Documentation/firmware-guide/acpi/DSD-properties-rules.rst?h=v6.16.7 Related conversation: https://lore.kernel.org/r/5b00870c-be1a-42d6-8857-48b89716d5e2@gmail.com ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] r8169: enable ASPM on Dell platforms 2025-09-12 15:30 ` Heiner Kallweit 2025-09-15 1:37 ` Chia-Lin Kao (AceLan) @ 2025-09-22 16:41 ` Bjorn Helgaas 1 sibling, 0 replies; 6+ messages in thread From: Bjorn Helgaas @ 2025-09-22 16:41 UTC (permalink / raw) To: Heiner Kallweit Cc: Chia-Lin Kao (AceLan), nic_swsd, Andrew Lunn, David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni, netdev, linux-kernel, Wang, Crag, Chen, Alan, Alex Shen@Dell, ChunHao Lin, Hayes Wang [+cc ChunHao, Hayes, nic_swsd] On Fri, Sep 12, 2025 at 05:30:52PM +0200, Heiner Kallweit wrote: > On 9/12/2025 9:29 AM, Chia-Lin Kao (AceLan) wrote: > > Enable PCIe ASPM for RTL8169 NICs on Dell platforms that have been > > verified to work reliably with this power management feature. The > > r8169 driver traditionally disables ASPM to prevent random link > > failures and system hangs on problematic hardware. > > > > Dell has validated these product families to work correctly with > > RTL NIC ASPM and commits to addressing any ASPM-related issues > > with RTL hardware in collaboration with Realtek. > > > > This change enables ASPM for the following Dell product families: > > - Alienware > > - Dell Laptops/Pro Laptops/Pro Max Laptops > > - Dell Desktops/Pro Desktops/Pro Max Desktops > > - Dell Pro Rugged Laptops Kudos to Dell for validating their products. > I'd like to avoid DMI-based whitelists in kernel code. If more system > vendors do it the same way, then this becomes hard to maintain. > There is already a mechanism for vendors to flag that they successfully > tested ASPM. See c217ab7a3961 ("r8169: enable ASPM L1.2 if system vendor > flags it as safe"). > Last but not least ASPM can be (re-)enabled from userspace, using sysfs. I don't maintain r8169, but I agree that a DMI-based list is a maintenance headache. Such a list also screws up the incentives: Realtek and OEMs benefit by selling these products, so they should bear the burden when they don't work correctly. Adding a DMI list unfairly shifts that burden to the maintainer. "Random link failures and system hangs" are probably not actually random, just not understood completely. If they're due to broken RTL8169 hardware or firmware, we should have quirks to disable ASPM when necessary. Same if there are broken Root Ports, although that seems unlikely since it would affect many devices. If the problems are due to misconfiguration, we should debug that. Do we have any concrete bug reports? Are there cases where RTL8169 works correctly with Windows but not Linux, where we could compare the ASPM configuration? > > Signed-off-by: Chia-Lin Kao (AceLan) <acelan.kao@canonical.com> > > --- > > drivers/net/ethernet/realtek/r8169_main.c | 29 +++++++++++++++++++++++ > > 1 file changed, 29 insertions(+) > > > > diff --git a/drivers/net/ethernet/realtek/r8169_main.c b/drivers/net/ethernet/realtek/r8169_main.c > > index 9c601f271c02..63e83cf071de 100644 > > --- a/drivers/net/ethernet/realtek/r8169_main.c > > +++ b/drivers/net/ethernet/realtek/r8169_main.c > > @@ -5366,6 +5366,32 @@ static void rtl_init_mac_address(struct rtl8169_private *tp) > > rtl_rar_set(tp, mac_addr); > > } > > > > +bool rtl_aspm_new_dell_platforms(void) > > +{ > > + const char *family = dmi_get_system_info(DMI_PRODUCT_FAMILY); > > + static const char * const dell_product_families[] = { > > + "Alienware", > > + "Dell Laptops", > > + "Dell Pro Laptops", > > + "Dell Pro Max Laptops", > > + "Dell Desktops", > > + "Dell Pro Desktops", > > + "Dell Pro Max Desktops", > > + "Dell Pro Rugged Laptops" > > + }; > > + int i; > > + > > + if (!family) > > + return false; > > + > > + for (i = 0; i < ARRAY_SIZE(dell_product_families); i++) { > > + if (str_has_prefix(family, dell_product_families[i])) > > + 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) > > { > > @@ -5373,6 +5399,9 @@ static bool rtl_aspm_is_safe(struct rtl8169_private *tp) > > r8168_mac_ocp_read(tp, 0xc0b2) & 0xf) > > return true; > > > > + if (rtl_aspm_new_dell_platforms()) > > + return true; > > + > > return false; > > } > > > ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2025-09-22 16:41 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2025-09-12 7:29 [PATCH] r8169: enable ASPM on Dell platforms Chia-Lin Kao (AceLan) 2025-09-12 15:30 ` Heiner Kallweit 2025-09-15 1:37 ` Chia-Lin Kao (AceLan) 2025-09-15 19:25 ` Heiner Kallweit 2025-09-16 20:35 ` Bjorn Helgaas 2025-09-22 16:41 ` Bjorn Helgaas
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).