Re: [PATCH v8 RESEND 6/6] r8169: Disable ASPM while doing NAPI poll

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(-)

diff --git a/drivers/net/ethernet/realtek/r8169_main.c b/drivers/net/ethernet/realtek/r8169_main.c
index 897f90b48bba6..4d4a802346ae3 100644
--- a/drivers/net/ethernet/realtek/r8169_main.c
+++ b/drivers/net/ethernet/realtek/r8169_main.c

@@ -2711,8 +2711,6 @@ static void rtl_hw_aspm_clkreq_enable(struct rtl8169_private *tp, bool enable)
              RTL_W8(tp, Config2, RTL_R8(tp, Config2) & ~ClkReqEn);
              RTL_W8(tp, Config5, RTL_R8(tp, Config5) & ~ASPM_en);
      }
-
-     udelay(10);
 }

 static void rtl_set_fifo_size(struct rtl8169_private *tp, u16 rx_stat,

@@ -4577,6 +4575,12 @@ static int rtl8169_poll(struct napi_struct *napi, int budget)
      struct net_device *dev = tp->dev;
      int work_done;

+     if (tp->aspm_manageable) {
+             rtl_unlock_config_regs(tp);

NAPI poll runs in softirq context (except for threaded NAPI).
Therefore you should use a spinlock instead of a mutex.

You are right. Will change it in next revision.

quoted

quoted

+             rtl_hw_aspm_clkreq_enable(tp, false);
+             rtl_lock_config_regs(tp);
+     }
+
      rtl_tx(dev, tp, budget);

      work_done = rtl_rx(dev, tp, budget);

@@ -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

Kai-Heng

quoted

quoted

+     }
+
      return work_done;
 }