| Message ID | 2d61ba5a-9a2c-28c3-4a1b-a81a3f34af3d@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | r8169: disable ASPM during NAPI poll | expand |
On Sat, Feb 25, 2023 at 10:47:32PM +0100, Heiner Kallweit wrote: > Now that ASPM is disabled during NAPI poll, we can remove all ASPM > restrictions. This allows for higher power savings if the network > isn't fully loaded. > > Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> > --- > drivers/net/ethernet/realtek/r8169_main.c | 27 +---------------------- > 1 file changed, 1 insertion(+), 26 deletions(-) > > diff --git a/drivers/net/ethernet/realtek/r8169_main.c b/drivers/net/ethernet/realtek/r8169_main.c > index 2897b9bf2..6563e4c6a 100644 > --- a/drivers/net/ethernet/realtek/r8169_main.c > +++ b/drivers/net/ethernet/realtek/r8169_main.c > @@ -620,7 +620,6 @@ struct rtl8169_private { > int cfg9346_usage_count; > > unsigned supports_gmii:1; > - unsigned aspm_manageable:1; > dma_addr_t counters_phys_addr; > struct rtl8169_counters *counters; > struct rtl8169_tc_offsets tc_offset; > @@ -2744,8 +2743,7 @@ static void rtl_hw_aspm_clkreq_enable(struct rtl8169_private *tp, bool enable) > if (tp->mac_version < RTL_GIGA_MAC_VER_32) > return; > > - /* Don't enable ASPM in the chip if OS can't control ASPM */ > - if (enable && tp->aspm_manageable) { > + if (enable) { > rtl_mod_config5(tp, 0, ASPM_en); > rtl_mod_config2(tp, 0, ClkReqEn); > > @@ -5221,16 +5219,6 @@ static void rtl_init_mac_address(struct rtl8169_private *tp) > rtl_rar_set(tp, mac_addr); > } > > -/* 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) > - return true; > - > - return false; > -} > - > static int rtl_init_one(struct pci_dev *pdev, const struct pci_device_id *ent) > { > struct rtl8169_private *tp; > @@ -5302,19 +5290,6 @@ static int rtl_init_one(struct pci_dev *pdev, const struct pci_device_id *ent) > > tp->mac_version = chipset; > > - /* Disable ASPM L1 as that cause random device stop working > - * problems as well as full system hangs for some PCIe devices users. > - * Chips from RTL8168h partially have issues with L1.2, but seem > - * to work fine with L1 and L1.1. > - */ > - if (rtl_aspm_is_safe(tp)) > - rc = 0; > - else if (tp->mac_version >= RTL_GIGA_MAC_VER_46) > - rc = pci_disable_link_state(pdev, PCIE_LINK_STATE_L1_2); > - else > - rc = pci_disable_link_state(pdev, PCIE_LINK_STATE_L1); > - tp->aspm_manageable = !rc; This is beautiful. But since this series still enables/disables ASPM using the chip-specific knobs, I have to ask whether this is all safe with respect to simultaneous arbitrary ASPM enable/disable via the sysfs knobs. For example, it should be safe to run these loops indefinitely while the NIC is operating and doing NAPI polls: DEV=/sys/bus/pci/devices/... while /bin/true; do echo 1 > $DEV/link/l1_2_aspm echo 0 > $DEV/link/l1_2_aspm done while /bin/true; do echo 1 > $DEV/link/l1_1_aspm echo 0 > $DEV/link/l1_1_aspm done while /bin/true; do echo 1 > $DEV/link/l1_aspm echo 0 > $DEV/link/l1_aspm done Bjorn
On 10.03.2023 22:19, Bjorn Helgaas wrote: > On Sat, Feb 25, 2023 at 10:47:32PM +0100, Heiner Kallweit wrote: >> Now that ASPM is disabled during NAPI poll, we can remove all ASPM >> restrictions. This allows for higher power savings if the network >> isn't fully loaded. >> >> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> >> --- >> drivers/net/ethernet/realtek/r8169_main.c | 27 +---------------------- >> 1 file changed, 1 insertion(+), 26 deletions(-) >> >> diff --git a/drivers/net/ethernet/realtek/r8169_main.c b/drivers/net/ethernet/realtek/r8169_main.c >> index 2897b9bf2..6563e4c6a 100644 >> --- a/drivers/net/ethernet/realtek/r8169_main.c >> +++ b/drivers/net/ethernet/realtek/r8169_main.c >> @@ -620,7 +620,6 @@ struct rtl8169_private { >> int cfg9346_usage_count; >> >> unsigned supports_gmii:1; >> - unsigned aspm_manageable:1; >> dma_addr_t counters_phys_addr; >> struct rtl8169_counters *counters; >> struct rtl8169_tc_offsets tc_offset; >> @@ -2744,8 +2743,7 @@ static void rtl_hw_aspm_clkreq_enable(struct rtl8169_private *tp, bool enable) >> if (tp->mac_version < RTL_GIGA_MAC_VER_32) >> return; >> >> - /* Don't enable ASPM in the chip if OS can't control ASPM */ >> - if (enable && tp->aspm_manageable) { >> + if (enable) { >> rtl_mod_config5(tp, 0, ASPM_en); >> rtl_mod_config2(tp, 0, ClkReqEn); >> >> @@ -5221,16 +5219,6 @@ static void rtl_init_mac_address(struct rtl8169_private *tp) >> rtl_rar_set(tp, mac_addr); >> } >> >> -/* 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) >> - return true; >> - >> - return false; >> -} >> - >> static int rtl_init_one(struct pci_dev *pdev, const struct pci_device_id *ent) >> { >> struct rtl8169_private *tp; >> @@ -5302,19 +5290,6 @@ static int rtl_init_one(struct pci_dev *pdev, const struct pci_device_id *ent) >> >> tp->mac_version = chipset; >> >> - /* Disable ASPM L1 as that cause random device stop working >> - * problems as well as full system hangs for some PCIe devices users. >> - * Chips from RTL8168h partially have issues with L1.2, but seem >> - * to work fine with L1 and L1.1. >> - */ >> - if (rtl_aspm_is_safe(tp)) >> - rc = 0; >> - else if (tp->mac_version >= RTL_GIGA_MAC_VER_46) >> - rc = pci_disable_link_state(pdev, PCIE_LINK_STATE_L1_2); >> - else >> - rc = pci_disable_link_state(pdev, PCIE_LINK_STATE_L1); >> - tp->aspm_manageable = !rc; > > This is beautiful. But since this series still enables/disables ASPM > using the chip-specific knobs, I have to ask whether this is all safe > with respect to simultaneous arbitrary ASPM enable/disable via the > sysfs knobs. For example, it should be safe to run these loops > indefinitely while the NIC is operating and doing NAPI polls: > Good question, but: Realtek doesn't provide any chip data sheets, therefore I can't say what the chip-specific knobs actually do. > DEV=/sys/bus/pci/devices/... > while /bin/true; do > echo 1 > $DEV/link/l1_2_aspm > echo 0 > $DEV/link/l1_2_aspm > done > while /bin/true; do > echo 1 > $DEV/link/l1_1_aspm > echo 0 > $DEV/link/l1_1_aspm > done > while /bin/true; do > echo 1 > $DEV/link/l1_aspm > echo 0 > $DEV/link/l1_aspm > done > > Bjorn
diff --git a/drivers/net/ethernet/realtek/r8169_main.c b/drivers/net/ethernet/realtek/r8169_main.c index 2897b9bf2..6563e4c6a 100644 --- a/drivers/net/ethernet/realtek/r8169_main.c +++ b/drivers/net/ethernet/realtek/r8169_main.c @@ -620,7 +620,6 @@ struct rtl8169_private { int cfg9346_usage_count; unsigned supports_gmii:1; - unsigned aspm_manageable:1; dma_addr_t counters_phys_addr; struct rtl8169_counters *counters; struct rtl8169_tc_offsets tc_offset; @@ -2744,8 +2743,7 @@ static void rtl_hw_aspm_clkreq_enable(struct rtl8169_private *tp, bool enable) if (tp->mac_version < RTL_GIGA_MAC_VER_32) return; - /* Don't enable ASPM in the chip if OS can't control ASPM */ - if (enable && tp->aspm_manageable) { + if (enable) { rtl_mod_config5(tp, 0, ASPM_en); rtl_mod_config2(tp, 0, ClkReqEn); @@ -5221,16 +5219,6 @@ static void rtl_init_mac_address(struct rtl8169_private *tp) rtl_rar_set(tp, mac_addr); } -/* 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) - return true; - - return false; -} - static int rtl_init_one(struct pci_dev *pdev, const struct pci_device_id *ent) { struct rtl8169_private *tp; @@ -5302,19 +5290,6 @@ static int rtl_init_one(struct pci_dev *pdev, const struct pci_device_id *ent) tp->mac_version = chipset; - /* Disable ASPM L1 as that cause random device stop working - * problems as well as full system hangs for some PCIe devices users. - * Chips from RTL8168h partially have issues with L1.2, but seem - * to work fine with L1 and L1.1. - */ - if (rtl_aspm_is_safe(tp)) - rc = 0; - else if (tp->mac_version >= RTL_GIGA_MAC_VER_46) - rc = pci_disable_link_state(pdev, PCIE_LINK_STATE_L1_2); - else - rc = pci_disable_link_state(pdev, PCIE_LINK_STATE_L1); - tp->aspm_manageable = !rc; - tp->dash_type = rtl_check_dash(tp); tp->cp_cmd = RTL_R16(tp, CPlusCmd) & CPCMD_MASK;
Now that ASPM is disabled during NAPI poll, we can remove all ASPM restrictions. This allows for higher power savings if the network isn't fully loaded. Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> --- drivers/net/ethernet/realtek/r8169_main.c | 27 +---------------------- 1 file changed, 1 insertion(+), 26 deletions(-)