| Message ID | 20230221023849.1906728-7-kai.heng.feng@canonical.com (mailing list archive) |
|---|---|
| State | Deferred |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | r8169: Enable ASPM for recent 1.0/2.5Gbps Realtek NICs | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Guessed tree name to be net-next |
| netdev/fixes_present | success | Fixes tag not required for -next series |
| netdev/subject_prefix | success | Link |
| netdev/cover_letter | success | Series has a cover letter |
| netdev/patch_count | success | Link |
| netdev/header_inline | success | No static functions without inline keyword in header files |
| netdev/build_32bit | success | Errors and warnings before: 0 this patch: 0 |
| netdev/cc_maintainers | success | CCed 7 of 7 maintainers |
| netdev/build_clang | success | Errors and warnings before: 0 this patch: 0 |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Signed-off-by tag matches author and committer |
| netdev/check_selftest | success | No net selftest shell script |
| netdev/verify_fixes | success | No Fixes tag |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 0 this patch: 0 |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 32 lines checked |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/source_inline | success | Was 0 now: 0 |
On 21.02.2023 03:38, Kai-Heng Feng wrote: > 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 <kai.heng.feng@canonical.com> > --- > 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. > + 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()? > + } > + > return work_done; > } >
On Tue, Feb 21, 2023 at 7:09 PM Heiner Kallweit <hkallweit1@gmail.com> wrote: > > On 21.02.2023 03:38, Kai-Heng Feng wrote: > > 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 <kai.heng.feng@canonical.com> > > --- > > 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. > > > + 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. Kai-Heng > > > + } > > + > > return work_done; > > } > > >
On Wed, Feb 22, 2023 at 9:03 PM Kai-Heng Feng <kai.heng.feng@canonical.com> wrote: > > On Tue, Feb 21, 2023 at 7:09 PM Heiner Kallweit <hkallweit1@gmail.com> wrote: > > > > On 21.02.2023 03:38, Kai-Heng Feng wrote: > > > 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 <kai.heng.feng@canonical.com> > > > --- > > > 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. > > > > > > + 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 > > > > > > + } > > > + > > > return work_done; > > > } > > > > >
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); + 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); + } + return work_done; }
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 <kai.heng.feng@canonical.com> --- v8: - New patch. drivers/net/ethernet/realtek/r8169_main.c | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-)