@@ -5224,13 +5224,13 @@ static int rtl_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)/* Disable ASPM L1 as that cause random device stop working*problemsaswellasfullsystemhangsforsomePCIedevicesusers.-*ChipsfromRTL8168hpartiallyhaveissueswithL1.2,butseem-*toworkfinewithL1andL1.1.+*ChipsfromRTL8168hpartiallyhaveissueswithL1.1andL1.2,but+*seemtoworkfinewithL1.*/if(rtl_aspm_is_safe(tp))rc=0;elseif(tp->mac_version>=RTL_GIGA_MAC_VER_46)-rc=pci_disable_link_state(pdev,PCIE_LINK_STATE_L1_2);+rc=pci_disable_link_state(pdev,PCIE_LINK_STATE_L1_1|PCIE_LINK_STATE_L1_2);elserc=pci_disable_link_state(pdev,PCIE_LINK_STATE_L1);tp->aspm_manageable=!rc;
This reverts commit ba13d4575da5e656a3cbc18583e0da5c5d865417.
This will be used by module once again.
Signed-off-by: Kai-Heng Feng <redacted>
---
v8:
- New patch.
drivers/pci/pcie/aspm.c | 1 +
1 file changed, 1 insertion(+)
Introduce a new helper, pcie_aspm_capable(), to report ASPM capability.
The user will be introduced by next patch.
Acked-by: Bjorn Helgaas <bhelgaas@google.com>
Suggested-by: Bjorn Helgaas <bhelgaas@google.com>
Signed-off-by: Kai-Heng Feng <redacted>
---
v8:
- No change.
v7:
- Change subject.
v6:
- No change.
v5:
- No change.
v4:
- Report aspm_capable instead.
v3:
- This is a new patch
drivers/pci/pcie/aspm.c | 11 +++++++++++
include/linux/pci.h | 2 ++
2 files changed, 13 insertions(+)
@@ -1692,6 +1692,7 @@ int pci_disable_link_state_locked(struct pci_dev *pdev, int state);voidpcie_no_aspm(void);boolpcie_aspm_support_enabled(void);boolpcie_aspm_enabled(structpci_dev*pdev);+boolpcie_aspm_capable(structpci_dev*pdev);#elsestaticinlineintpci_disable_link_state(structpci_dev*pdev,intstate){return0;}
@@ -1700,6 +1701,7 @@ static inline int pci_disable_link_state_locked(struct pci_dev *pdev, int state)staticinlinevoidpcie_no_aspm(void){}staticinlineboolpcie_aspm_support_enabled(void){returnfalse;}staticinlineboolpcie_aspm_enabled(structpci_dev*pdev){returnfalse;}+staticinlineboolpcie_aspm_capable(structpci_dev*pdev){returnfalse;}#endif#ifdef CONFIG_PCIEAER
To really enable ASPM on r8169 NICs, both standard PCIe ASPM and
chip-specific ASPM have to be enabled at the same time.
Before enabling ASPM at chip side, make sure the following conditions
are met:
1) Use pcie_aspm_support_enabled() to check if ASPM is disabled by
kernel parameter.
2) Use pcie_aspm_capable() to see if the device is capable to perform
PCIe ASPM.
3) Check the return value of pci_disable_link_state(). If it's -EPERM,
it means BIOS doesn't grant ASPM control to OS, and device should use
the ASPM setting as is
Consider ASPM is manageable when those conditions are met.
While at it, disable ASPM at chip-side for TX timeout reset, since
pci_disable_link_state() doesn't have any effect when OS isn't granted
with ASPM control.
Signed-off-by: Kai-Heng Feng <redacted>
---
v8:
- Enable chip-side ASPM only when PCIe ASPM is already available.
- Wording.
v7:
- No change.
v6:
- Unconditionally enable chip-specific ASPM.
v5:
- New patch.
drivers/net/ethernet/realtek/r8169_main.c | 22 ++++++++++++++++++----
1 file changed, 18 insertions(+), 4 deletions(-)
@@ -2675,8 +2675,11 @@ static void rtl_disable_exit_l1(struct rtl8169_private *tp)staticvoidrtl_hw_aspm_clkreq_enable(structrtl8169_private*tp,boolenable){-/* Don't enable ASPM in the chip if OS can't control ASPM */-if(enable&&tp->aspm_manageable){+/* Skip if PCIe ASPM isn't possible */+if(!tp->aspm_manageable)+return;++if(enable){RTL_W8(tp,Config5,RTL_R8(tp,Config5)|ASPM_en);RTL_W8(tp,Config2,RTL_R8(tp,Config2)|ClkReqEn);
@@ -4545,8 +4548,13 @@ static void rtl_task(struct work_struct *work)/* ASPM compatibility issues are a typical reason for tx timeouts */ret=pci_disable_link_state(tp->pci_dev,PCIE_LINK_STATE_L1|PCIE_LINK_STATE_L0S);++/* OS may not be granted to control PCIe ASPM, prevent the driver from using it */+tp->aspm_manageable=0;+if(!ret)netdev_warn_once(tp->dev,"ASPM disabled on Tx timeout\n");+gotoreset;}
@@ -5227,13 +5235,19 @@ static int rtl_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)*ChipsfromRTL8168hpartiallyhaveissueswithL1.1andL1.2,but*seemtoworkfinewithL1.*/-if(rtl_aspm_is_safe(tp))+if(!pcie_aspm_support_enabled()||!pcie_aspm_capable(pdev))+rc=-EINVAL;+elseif(rtl_aspm_is_safe(tp))rc=0;elseif(tp->mac_version>=RTL_GIGA_MAC_VER_46)rc=pci_disable_link_state(pdev,PCIE_LINK_STATE_L1_1|PCIE_LINK_STATE_L1_2);elserc=pci_disable_link_state(pdev,PCIE_LINK_STATE_L1);-tp->aspm_manageable=!rc;++/* -EPERM means BIOS doesn't grant OS ASPM control, ASPM should be use+*asis.Honorit.+*/+tp->aspm_manageable=(rc==-EPERM)?1:!rc;tp->dash_type=rtl_check_dash(tp);
Right now r8169 doesn't have parallel access to its config register, but
the next patch makes the driver access config register at anytime.
So add a mutex to protect the config register from any potential race.
Signed-off-by: Kai-Heng Feng <redacted>
---
v8:
- Swap the place when using the mutex. Protect when config register is
unlocked.
v7:
- This is a new patch.
drivers/net/ethernet/realtek/r8169_main.c | 6 ++++++
1 file changed, 6 insertions(+)
NAPI poll of Realtek NICs don't seem to perform well ASPM is enabled.
The vendor driver uses a mechanism called "dynamic ASPM" to toggle ASPM
based on the packet number in given time period.
Instead of implementing "dynamic ASPM", use a more straightforward way
by disabling ASPM during NAPI poll, as a similar approach was
implemented to solve slow performance on Realtek wireless NIC, see
commit 24f5e38a13b5 ("rtw88: Disable PCIe ASPM while doing NAPI poll on
8821CE").
Since NAPI poll should be handled as fast as possible, also remove the
delay in rtl_hw_aspm_clkreq_enable() which was added by commit
94235460f9ea ("r8169: Align ASPM/CLKREQ setting function with vendor
driver").
Signed-off-by: Kai-Heng Feng <redacted>
---
v8:
- New patch.
drivers/net/ethernet/realtek/r8169_main.c | 14 ++++++++++++--
1 file changed, 12 insertions(+), 2 deletions(-)
Note that net-next is closed during merge window.
Formal aspect: Your patches miss the net/net-next annotation.
The title of the series may be an old one. Actually most ASPM
states are enabled, you add to disable ASPM temporarily.
These references are about problems with L1.2 (which is disabled
per default in mainline). They don't allow any statement about whether
L1.1 is problematic too (and under which circumstances).
At least on my system with RTL8168h there's no problem with L1.1
when running iperf.
@@ -5224,13 +5224,13 @@ static int rtl_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)/* Disable ASPM L1 as that cause random device stop working*problemsaswellasfullsystemhangsforsomePCIedevicesusers.-*ChipsfromRTL8168hpartiallyhaveissueswithL1.2,butseem-*toworkfinewithL1andL1.1.+*ChipsfromRTL8168hpartiallyhaveissueswithL1.1andL1.2,but+*seemtoworkfinewithL1.*/if(rtl_aspm_is_safe(tp))rc=0;elseif(tp->mac_version>=RTL_GIGA_MAC_VER_46)-rc=pci_disable_link_state(pdev,PCIE_LINK_STATE_L1_2);+rc=pci_disable_link_state(pdev,PCIE_LINK_STATE_L1_1|PCIE_LINK_STATE_L1_2);elserc=pci_disable_link_state(pdev,PCIE_LINK_STATE_L1);tp->aspm_manageable=!rc;
NAPI poll of Realtek NICs don't seem to perform well ASPM is enabled.
The vendor driver uses a mechanism called "dynamic ASPM" to toggle ASPM
based on the packet number in given time period.
Instead of implementing "dynamic ASPM", use a more straightforward way
by disabling ASPM during NAPI poll, as a similar approach was
implemented to solve slow performance on Realtek wireless NIC, see
commit 24f5e38a13b5 ("rtw88: Disable PCIe ASPM while doing NAPI poll on
8821CE").
Since NAPI poll should be handled as fast as possible, also remove the
delay in rtl_hw_aspm_clkreq_enable() which was added by commit
94235460f9ea ("r8169: Align ASPM/CLKREQ setting function with vendor
driver").
Signed-off-by: Kai-Heng Feng <redacted>
---
v8:
- New patch.
drivers/net/ethernet/realtek/r8169_main.c | 14 ++++++++++++--
1 file changed, 12 insertions(+), 2 deletions(-)
These references are about problems with L1.2 (which is disabled
per default in mainline). They don't allow any statement about whether
L1.1 is problematic too (and under which circumstances).
At least on my system with RTL8168h there's no problem with L1.1
when running iperf.
There are some systems have performance issue with L1.1 too.
But since the series will disable chip-side ASPM during NAPI poll,
maybe we can keep both L1.1 and L1.2 enabled?
Kai-Heng
@@ -5224,13 +5224,13 @@ static int rtl_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)/* Disable ASPM L1 as that cause random device stop working*problemsaswellasfullsystemhangsforsomePCIedevicesusers.-*ChipsfromRTL8168hpartiallyhaveissueswithL1.2,butseem-*toworkfinewithL1andL1.1.+*ChipsfromRTL8168hpartiallyhaveissueswithL1.1andL1.2,but+*seemtoworkfinewithL1.*/if(rtl_aspm_is_safe(tp))rc=0;elseif(tp->mac_version>=RTL_GIGA_MAC_VER_46)-rc=pci_disable_link_state(pdev,PCIE_LINK_STATE_L1_2);+rc=pci_disable_link_state(pdev,PCIE_LINK_STATE_L1_1|PCIE_LINK_STATE_L1_2);elserc=pci_disable_link_state(pdev,PCIE_LINK_STATE_L1);tp->aspm_manageable=!rc;
On Tue, Feb 21, 2023 at 7:09 PM Heiner Kallweit [off-list ref] wrote:
On 21.02.2023 03:38, Kai-Heng Feng wrote:
quoted
NAPI poll of Realtek NICs don't seem to perform well ASPM is enabled.
The vendor driver uses a mechanism called "dynamic ASPM" to toggle ASPM
based on the packet number in given time period.
Instead of implementing "dynamic ASPM", use a more straightforward way
by disabling ASPM during NAPI poll, as a similar approach was
implemented to solve slow performance on Realtek wireless NIC, see
commit 24f5e38a13b5 ("rtw88: Disable PCIe ASPM while doing NAPI poll on
8821CE").
Since NAPI poll should be handled as fast as possible, also remove the
delay in rtl_hw_aspm_clkreq_enable() which was added by commit
94235460f9ea ("r8169: Align ASPM/CLKREQ setting function with vendor
driver").
Signed-off-by: Kai-Heng Feng <redacted>
---
v8:
- New patch.
drivers/net/ethernet/realtek/r8169_main.c | 14 ++++++++++++--
1 file changed, 12 insertions(+), 2 deletions(-)
On Wed, Feb 22, 2023 at 9:03 PM Kai-Heng Feng
[off-list ref] wrote:
On Tue, Feb 21, 2023 at 7:09 PM Heiner Kallweit [off-list ref] wrote:
quoted
On 21.02.2023 03:38, Kai-Heng Feng wrote:
quoted
NAPI poll of Realtek NICs don't seem to perform well ASPM is enabled.
The vendor driver uses a mechanism called "dynamic ASPM" to toggle ASPM
based on the packet number in given time period.
Instead of implementing "dynamic ASPM", use a more straightforward way
by disabling ASPM during NAPI poll, as a similar approach was
implemented to solve slow performance on Realtek wireless NIC, see
commit 24f5e38a13b5 ("rtw88: Disable PCIe ASPM while doing NAPI poll on
8821CE").
Since NAPI poll should be handled as fast as possible, also remove the
delay in rtl_hw_aspm_clkreq_enable() which was added by commit
94235460f9ea ("r8169: Align ASPM/CLKREQ setting function with vendor
driver").
Signed-off-by: Kai-Heng Feng <redacted>
---
v8:
- New patch.
drivers/net/ethernet/realtek/r8169_main.c | 14 ++++++++++++--
1 file changed, 12 insertions(+), 2 deletions(-)
@@ -4584,6 +4588,12 @@ static int rtl8169_poll(struct napi_struct *napi, int budget) if (work_done < budget && napi_complete_done(napi, work_done)) rtl_irq_enable(tp);+ if (tp->aspm_manageable) {+ rtl_unlock_config_regs(tp);+ rtl_hw_aspm_clkreq_enable(tp, true);+ rtl_lock_config_regs(tp);
Why not moving lock/unlock into rtl_hw_aspm_clkreq_enable()?
Because where it gets called at other places don't need the lock.
But yes this will make it easier to read, will do in next revision.
We can't do that because it creates deadlock:
rtl_hw_start()
rtl_unlock_config_regs()
rtl_hw_start_8168()
rtl_hw_config()
rtl_hw_start_8168h_1()
rtl_hw_aspm_clkreq_enable()
Kai-Heng