[PATCH RFC 0/6] r8169: disable ASPM during NAPI poll

[PATCH RFC 0/6] r8169: disable ASPM during NAPI poll

[Heiner Kallweit]

This is a rework of ideas from Kai-Heng on how to avoid the known
ASPM issues whilst still allowing for a maximum of ASPM-related power
savings. As a prerequisite some locking is added first.

This change affects a bigger number of supported chip versions,
therefore this series comes as RFC first for further testing.

Heiner Kallweit (6):
  r8169: use spinlock to protect mac ocp register access
  r8169: use spinlock to protect access to registers Config2 and Config5
  r8169: enable cfg9346 config register access in atomic context
  r8169: prepare rtl_hw_aspm_clkreq_enable for usage in atomic context
  r8169: disable ASPM during NAPI poll
  r8169: remove ASPM restrictions now that ASPM is disabled during NAPI
    poll

 drivers/net/ethernet/realtek/r8169_main.c | 145 +++++++++++++++-------
 1 file changed, 100 insertions(+), 45 deletions(-)

-- 
2.39.2

[PATCH RFC 1/6] r8169: use spinlock to protect mac ocp register access

[Heiner Kallweit]

For disabling ASPM during NAPI poll we'll have to access mac ocp
registers in atomic context. This could result in races because
a mac ocp read consists of a write to register OCPDR, followed
by a read from the same register. Therefore add a spinlock to
protext access to mac ocp registers.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 drivers/net/ethernet/realtek/r8169_main.c | 37 ++++++++++++++++++++---
 1 file changed, 33 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/realtek/r8169_main.c b/drivers/net/ethernet/realtek/r8169_main.c
index 45147a101..259eac5b0 100644
--- a/drivers/net/ethernet/realtek/r8169_main.c
+++ b/drivers/net/ethernet/realtek/r8169_main.c
@@ -613,6 +613,8 @@ struct rtl8169_private {
 		struct work_struct work;
 	} wk;
 
+	spinlock_t mac_ocp_lock;
+
 	unsigned supports_gmii:1;
 	unsigned aspm_manageable:1;
 	dma_addr_t counters_phys_addr;
@@ -847,7 +849,7 @@ static int r8168_phy_ocp_read(struct rtl8169_private *tp, u32 reg)
 		(RTL_R32(tp, GPHY_OCP) & 0xffff) : -ETIMEDOUT;
 }
 
-static void r8168_mac_ocp_write(struct rtl8169_private *tp, u32 reg, u32 data)
+static void __r8168_mac_ocp_write(struct rtl8169_private *tp, u32 reg, u32 data)
 {
 	if (rtl_ocp_reg_failure(reg))
 		return;
@@ -855,7 +857,16 @@ static void r8168_mac_ocp_write(struct rtl8169_private *tp, u32 reg, u32 data)
 	RTL_W32(tp, OCPDR, OCPAR_FLAG | (reg << 15) | data);
 }
 
-static u16 r8168_mac_ocp_read(struct rtl8169_private *tp, u32 reg)
+static void r8168_mac_ocp_write(struct rtl8169_private *tp, u32 reg, u32 data)
+{
+	unsigned long flags;
+
+	spin_lock_irqsave(&tp->mac_ocp_lock, flags);
+	__r8168_mac_ocp_write(tp, reg, data);
+	spin_unlock_irqrestore(&tp->mac_ocp_lock, flags);
+}
+
+static u16 __r8168_mac_ocp_read(struct rtl8169_private *tp, u32 reg)
 {
 	if (rtl_ocp_reg_failure(reg))
 		return 0;
@@ -865,12 +876,28 @@ static u16 r8168_mac_ocp_read(struct rtl8169_private *tp, u32 reg)
 	return RTL_R32(tp, OCPDR);
 }
 
+static u16 r8168_mac_ocp_read(struct rtl8169_private *tp, u32 reg)
+{
+	unsigned long flags;
+	u16 val;
+
+	spin_lock_irqsave(&tp->mac_ocp_lock, flags);
+	val = __r8168_mac_ocp_read(tp, reg);
+	spin_unlock_irqrestore(&tp->mac_ocp_lock, flags);
+
+	return val;
+}
+
 static void r8168_mac_ocp_modify(struct rtl8169_private *tp, u32 reg, u16 mask,
 				 u16 set)
 {
-	u16 data = r8168_mac_ocp_read(tp, reg);
+	unsigned long flags;
+	u16 data;
 
-	r8168_mac_ocp_write(tp, reg, (data & ~mask) | set);
+	spin_lock_irqsave(&tp->mac_ocp_lock, flags);
+	data = __r8168_mac_ocp_read(tp, reg);
+	__r8168_mac_ocp_write(tp, reg, (data & ~mask) | set);
+	spin_unlock_irqrestore(&tp->mac_ocp_lock, flags);
 }
 
 /* Work around a hw issue with RTL8168g PHY, the quirk disables
@@ -5176,6 +5203,8 @@ static int rtl_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
 	tp->eee_adv = -1;
 	tp->ocp_base = OCP_STD_PHY_BASE;
 
+	spin_lock_init(&tp->mac_ocp_lock);
+
 	dev->tstats = devm_netdev_alloc_pcpu_stats(&pdev->dev,
 						   struct pcpu_sw_netstats);
 	if (!dev->tstats)
-- 
2.39.2

[PATCH RFC 2/6] r8169: use spinlock to protect access to registers Config2 and Config5

[Heiner Kallweit]

For disabling ASPM during NAPI poll we'll have to access both registers
in atomic context. Use a spinlock to protect access.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 drivers/net/ethernet/realtek/r8169_main.c | 47 ++++++++++++++++++-----
 1 file changed, 37 insertions(+), 10 deletions(-)

diff --git a/drivers/net/ethernet/realtek/r8169_main.c b/drivers/net/ethernet/realtek/r8169_main.c
index 259eac5b0..e6f3f1947 100644
--- a/drivers/net/ethernet/realtek/r8169_main.c
+++ b/drivers/net/ethernet/realtek/r8169_main.c
@@ -613,6 +613,7 @@ struct rtl8169_private {
 		struct work_struct work;
 	} wk;
 
+	spinlock_t config25_lock;
 	spinlock_t mac_ocp_lock;
 
 	unsigned supports_gmii:1;
@@ -677,6 +678,28 @@ static void rtl_pci_commit(struct rtl8169_private *tp)
 	RTL_R8(tp, ChipCmd);
 }
 
+static void rtl_mod_config2(struct rtl8169_private *tp, u8 clear, u8 set)
+{
+	unsigned long flags;
+	u8 val;
+
+	spin_lock_irqsave(&tp->config25_lock, flags);
+	val = RTL_R8(tp, Config2);
+	RTL_W8(tp, Config2, (val & ~clear) | set);
+	spin_unlock_irqrestore(&tp->config25_lock, flags);
+}
+
+static void rtl_mod_config5(struct rtl8169_private *tp, u8 clear, u8 set)
+{
+	unsigned long flags;
+	u8 val;
+
+	spin_lock_irqsave(&tp->config25_lock, flags);
+	val = RTL_R8(tp, Config5);
+	RTL_W8(tp, Config5, (val & ~clear) | set);
+	spin_unlock_irqrestore(&tp->config25_lock, flags);
+}
+
 static bool rtl_is_8125(struct rtl8169_private *tp)
 {
 	return tp->mac_version >= RTL_GIGA_MAC_VER_61;
@@ -1363,6 +1386,7 @@ static void __rtl8169_set_wol(struct rtl8169_private *tp, u32 wolopts)
 		{ WAKE_MAGIC, Config3, MagicPacket }
 	};
 	unsigned int i, tmp = ARRAY_SIZE(cfg);
+	unsigned long flags;
 	u8 options;
 
 	rtl_unlock_config_regs(tp);
@@ -1381,12 +1405,14 @@ static void __rtl8169_set_wol(struct rtl8169_private *tp, u32 wolopts)
 			r8168_mac_ocp_modify(tp, 0xc0b6, BIT(0), 0);
 	}
 
+	spin_lock_irqsave(&tp->config25_lock, flags);
 	for (i = 0; i < tmp; i++) {
 		options = RTL_R8(tp, cfg[i].reg) & ~cfg[i].mask;
 		if (wolopts & cfg[i].opt)
 			options |= cfg[i].mask;
 		RTL_W8(tp, cfg[i].reg, options);
 	}
+	spin_unlock_irqrestore(&tp->config25_lock, flags);
 
 	switch (tp->mac_version) {
 	case RTL_GIGA_MAC_VER_02 ... RTL_GIGA_MAC_VER_06:
@@ -1398,10 +1424,10 @@ static void __rtl8169_set_wol(struct rtl8169_private *tp, u32 wolopts)
 	case RTL_GIGA_MAC_VER_34:
 	case RTL_GIGA_MAC_VER_37:
 	case RTL_GIGA_MAC_VER_39 ... RTL_GIGA_MAC_VER_63:
-		options = RTL_R8(tp, Config2) & ~PME_SIGNAL;
 		if (wolopts)
-			options |= PME_SIGNAL;
-		RTL_W8(tp, Config2, options);
+			rtl_mod_config2(tp, 0, PME_SIGNAL);
+		else
+			rtl_mod_config2(tp, PME_SIGNAL, 0);
 		break;
 	default:
 		break;
@@ -2704,8 +2730,8 @@ static void rtl_hw_aspm_clkreq_enable(struct rtl8169_private *tp, bool enable)
 {
 	/* Don't enable ASPM in the chip if OS can't control ASPM */
 	if (enable && tp->aspm_manageable) {
-		RTL_W8(tp, Config5, RTL_R8(tp, Config5) | ASPM_en);
-		RTL_W8(tp, Config2, RTL_R8(tp, Config2) | ClkReqEn);
+		rtl_mod_config5(tp, 0, ASPM_en);
+		rtl_mod_config2(tp, 0, ClkReqEn);
 
 		switch (tp->mac_version) {
 		case RTL_GIGA_MAC_VER_46 ... RTL_GIGA_MAC_VER_48:
@@ -2728,8 +2754,8 @@ static void rtl_hw_aspm_clkreq_enable(struct rtl8169_private *tp, bool enable)
 			break;
 		}
 
-		RTL_W8(tp, Config2, RTL_R8(tp, Config2) & ~ClkReqEn);
-		RTL_W8(tp, Config5, RTL_R8(tp, Config5) & ~ASPM_en);
+		rtl_mod_config2(tp, ClkReqEn, 0);
+		rtl_mod_config5(tp, ASPM_en, 0);
 	}
 
 	udelay(10);
@@ -2890,7 +2916,7 @@ static void rtl_hw_start_8168e_1(struct rtl8169_private *tp)
 	RTL_W32(tp, MISC, RTL_R32(tp, MISC) | TXPLA_RST);
 	RTL_W32(tp, MISC, RTL_R32(tp, MISC) & ~TXPLA_RST);
 
-	RTL_W8(tp, Config5, RTL_R8(tp, Config5) & ~Spi_en);
+	rtl_mod_config5(tp, Spi_en, 0);
 }
 
 static void rtl_hw_start_8168e_2(struct rtl8169_private *tp)
@@ -2923,7 +2949,7 @@ static void rtl_hw_start_8168e_2(struct rtl8169_private *tp)
 
 	RTL_W8(tp, DLLPR, RTL_R8(tp, DLLPR) | PFM_EN);
 	RTL_W32(tp, MISC, RTL_R32(tp, MISC) | PWM_EN);
-	RTL_W8(tp, Config5, RTL_R8(tp, Config5) & ~Spi_en);
+	rtl_mod_config5(tp, Spi_en, 0);
 
 	rtl_hw_aspm_clkreq_enable(tp, true);
 }
@@ -2946,7 +2972,7 @@ static void rtl_hw_start_8168f(struct rtl8169_private *tp)
 	RTL_W8(tp, MCU, RTL_R8(tp, MCU) & ~NOW_IS_OOB);
 	RTL_W8(tp, DLLPR, RTL_R8(tp, DLLPR) | PFM_EN);
 	RTL_W32(tp, MISC, RTL_R32(tp, MISC) | PWM_EN);
-	RTL_W8(tp, Config5, RTL_R8(tp, Config5) & ~Spi_en);
+	rtl_mod_config5(tp, Spi_en, 0);
 
 	rtl8168_config_eee_mac(tp);
 }
@@ -5203,6 +5229,7 @@ static int rtl_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
 	tp->eee_adv = -1;
 	tp->ocp_base = OCP_STD_PHY_BASE;
 
+	spin_lock_init(&tp->config25_lock);
 	spin_lock_init(&tp->mac_ocp_lock);
 
 	dev->tstats = devm_netdev_alloc_pcpu_stats(&pdev->dev,
-- 
2.39.2

[PATCH RFC 3/6] r8169: enable cfg9346 config register access in atomic context

[Heiner Kallweit]

For disabling ASPM during NAPI poll we'll have to unlock access
to the config registers in atomic context. Other code parts
running with config register access unlocked are partially
longer and can sleep. Add a usage counter to enable parallel
execution of code parts requiring unlocked config registers.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 drivers/net/ethernet/realtek/r8169_main.c | 18 ++++++++++++++++--
 1 file changed, 16 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/realtek/r8169_main.c b/drivers/net/ethernet/realtek/r8169_main.c
index e6f3f1947..61cbf498f 100644
--- a/drivers/net/ethernet/realtek/r8169_main.c
+++ b/drivers/net/ethernet/realtek/r8169_main.c
@@ -616,6 +616,9 @@ struct rtl8169_private {
 	spinlock_t config25_lock;
 	spinlock_t mac_ocp_lock;
 
+	spinlock_t cfg9346_usage_lock;
+	int cfg9346_usage_count;
+
 	unsigned supports_gmii:1;
 	unsigned aspm_manageable:1;
 	dma_addr_t counters_phys_addr;
@@ -664,12 +667,22 @@ static inline struct device *tp_to_dev(struct rtl8169_private *tp)
 
 static void rtl_lock_config_regs(struct rtl8169_private *tp)
 {
-	RTL_W8(tp, Cfg9346, Cfg9346_Lock);
+	unsigned long flags;
+
+	spin_lock_irqsave(&tp->cfg9346_usage_lock, flags);
+	if (!--tp->cfg9346_usage_count)
+		RTL_W8(tp, Cfg9346, Cfg9346_Lock);
+	spin_unlock_irqrestore(&tp->cfg9346_usage_lock, flags);
 }
 
 static void rtl_unlock_config_regs(struct rtl8169_private *tp)
 {
-	RTL_W8(tp, Cfg9346, Cfg9346_Unlock);
+	unsigned long flags;
+
+	spin_lock_irqsave(&tp->cfg9346_usage_lock, flags);
+	if (!tp->cfg9346_usage_count++)
+		RTL_W8(tp, Cfg9346, Cfg9346_Unlock);
+	spin_unlock_irqrestore(&tp->cfg9346_usage_lock, flags);
 }
 
 static void rtl_pci_commit(struct rtl8169_private *tp)
@@ -5229,6 +5242,7 @@ static int rtl_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
 	tp->eee_adv = -1;
 	tp->ocp_base = OCP_STD_PHY_BASE;
 
+	spin_lock_init(&tp->cfg9346_usage_lock);
 	spin_lock_init(&tp->config25_lock);
 	spin_lock_init(&tp->mac_ocp_lock);
 
-- 
2.39.2

[PATCH RFC 4/6] r8169: prepare rtl_hw_aspm_clkreq_enable for usage in atomic context

[Heiner Kallweit]

Bail out if the function is used with chip versions that don't support
ASPM configuration. In addition remove the delay, it tuned out that
it's not needed, also vendor driver r8125 doesn't have it.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 drivers/net/ethernet/realtek/r8169_main.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/realtek/r8169_main.c b/drivers/net/ethernet/realtek/r8169_main.c
index 61cbf498f..96af31aea 100644
--- a/drivers/net/ethernet/realtek/r8169_main.c
+++ b/drivers/net/ethernet/realtek/r8169_main.c
@@ -2741,6 +2741,9 @@ static void rtl_disable_exit_l1(struct rtl8169_private *tp)
 
 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) {
 		rtl_mod_config5(tp, 0, ASPM_en);
@@ -2770,8 +2773,6 @@ static void rtl_hw_aspm_clkreq_enable(struct rtl8169_private *tp, bool enable)
 		rtl_mod_config2(tp, ClkReqEn, 0);
 		rtl_mod_config5(tp, ASPM_en, 0);
 	}
-
-	udelay(10);
 }
 
 static void rtl_set_fifo_size(struct rtl8169_private *tp, u16 rx_stat,
-- 
2.39.2

[PATCH RFC 5/6] r8169: disable ASPM during NAPI poll

[Heiner Kallweit]

Several chip versions have problems with ASPM, what may result in
rx_missed errors or tx timeouts. The root cause isn't known but
experience shows that disabling ASPM during NAPI poll can avoid
these problems.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 drivers/net/ethernet/realtek/r8169_main.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/realtek/r8169_main.c b/drivers/net/ethernet/realtek/r8169_main.c
index 96af31aea..2897b9bf2 100644
--- a/drivers/net/ethernet/realtek/r8169_main.c
+++ b/drivers/net/ethernet/realtek/r8169_main.c
@@ -4577,6 +4577,10 @@ static irqreturn_t rtl8169_interrupt(int irq, void *dev_instance)
 	}
 
 	if (napi_schedule_prep(&tp->napi)) {
+		rtl_unlock_config_regs(tp);
+		rtl_hw_aspm_clkreq_enable(tp, false);
+		rtl_lock_config_regs(tp);
+
 		rtl_irq_disable(tp);
 		__napi_schedule(&tp->napi);
 	}
@@ -4636,9 +4640,14 @@ static int rtl8169_poll(struct napi_struct *napi, int budget)
 
 	work_done = rtl_rx(dev, tp, budget);
 
-	if (work_done < budget && napi_complete_done(napi, work_done))
+	if (work_done < budget && napi_complete_done(napi, work_done)) {
 		rtl_irq_enable(tp);
 
+		rtl_unlock_config_regs(tp);
+		rtl_hw_aspm_clkreq_enable(tp, true);
+		rtl_lock_config_regs(tp);
+	}
+
 	return work_done;
 }
 
-- 
2.39.2

[PATCH RFC 6/6] r8169: remove ASPM restrictions now that ASPM is disabled during NAPI poll

[Heiner Kallweit]

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;
-
 	tp->dash_type = rtl_check_dash(tp);
 
 	tp->cp_cmd = RTL_R16(tp, CPlusCmd) & CPCMD_MASK;
-- 
2.39.2

Re: [PATCH RFC 0/6] r8169: disable ASPM during NAPI poll

[Kai-Heng Feng]

On Sun, Feb 26, 2023 at 5:43 AM Heiner Kallweit <hkallweit1@gmail.com> wrote:
>
> This is a rework of ideas from Kai-Heng on how to avoid the known
> ASPM issues whilst still allowing for a maximum of ASPM-related power
> savings. As a prerequisite some locking is added first.
>
> This change affects a bigger number of supported chip versions,
> therefore this series comes as RFC first for further testing.

Thanks for the series.

Tested-by: Kai-Heng Feng <kai.heng.feng@canonical.com>

>
> Heiner Kallweit (6):
>   r8169: use spinlock to protect mac ocp register access
>   r8169: use spinlock to protect access to registers Config2 and Config5
>   r8169: enable cfg9346 config register access in atomic context
>   r8169: prepare rtl_hw_aspm_clkreq_enable for usage in atomic context
>   r8169: disable ASPM during NAPI poll
>   r8169: remove ASPM restrictions now that ASPM is disabled during NAPI
>     poll
>
>  drivers/net/ethernet/realtek/r8169_main.c | 145 +++++++++++++++-------
>  1 file changed, 100 insertions(+), 45 deletions(-)
>
> --
> 2.39.2
>

Re: [PATCH RFC 0/6] r8169: disable ASPM during NAPI poll

[Simon Horman]

On Wed, Mar 01, 2023 at 02:50:43PM +0800, Kai-Heng Feng wrote:
> On Sun, Feb 26, 2023 at 5:43 AM Heiner Kallweit <hkallweit1@gmail.com> wrote:
> >
> > This is a rework of ideas from Kai-Heng on how to avoid the known
> > ASPM issues whilst still allowing for a maximum of ASPM-related power
> > savings. As a prerequisite some locking is added first.
> >
> > This change affects a bigger number of supported chip versions,
> > therefore this series comes as RFC first for further testing.
> 
> Thanks for the series.
> 
> Tested-by: Kai-Heng Feng <kai.heng.feng@canonical.com>

FWIIW, I also looked over this series and it looks good to me.

Reviewed-by: Simon Horman <simon.horman@corigine.com>

Re: [PATCH RFC 0/6] r8169: disable ASPM during NAPI poll

[Holger Hoffstätte]

On 2023-02-25 22:43, Heiner Kallweit wrote:
> This is a rework of ideas from Kai-Heng on how to avoid the known
> ASPM issues whilst still allowing for a maximum of ASPM-related power
> savings. As a prerequisite some locking is added first.
> 
> This change affects a bigger number of supported chip versions,
> therefore this series comes as RFC first for further testing.
> 
> Heiner Kallweit (6):
>    r8169: use spinlock to protect mac ocp register access
>    r8169: use spinlock to protect access to registers Config2 and Config5
>    r8169: enable cfg9346 config register access in atomic context
>    r8169: prepare rtl_hw_aspm_clkreq_enable for usage in atomic context
>    r8169: disable ASPM during NAPI poll
>    r8169: remove ASPM restrictions now that ASPM is disabled during NAPI
>      poll
> 
>   drivers/net/ethernet/realtek/r8169_main.c | 145 +++++++++++++++-------
>   1 file changed, 100 insertions(+), 45 deletions(-)
> 

I've been running this 24/7 for a few days now without any problems, so for
the whole series:

Tested-by: Holger Hoffstätte <holger@applied-asynchrony.com>

Thanks!
Holger

Re: [PATCH RFC 1/6] r8169: use spinlock to protect mac ocp register access

[Bjorn Helgaas]

On Sat, Feb 25, 2023 at 10:44:10PM +0100, Heiner Kallweit wrote:
> For disabling ASPM during NAPI poll we'll have to access mac ocp
> registers in atomic context. This could result in races because
> a mac ocp read consists of a write to register OCPDR, followed
> by a read from the same register. Therefore add a spinlock to
> protext access to mac ocp registers.

protect

Re: [PATCH RFC 3/6] r8169: enable cfg9346 config register access in atomic context

[Bjorn Helgaas]

On Sat, Feb 25, 2023 at 10:45:28PM +0100, Heiner Kallweit wrote:
> For disabling ASPM during NAPI poll we'll have to unlock access
> to the config registers in atomic context. Other code parts
> running with config register access unlocked are partially
> longer and can sleep.

"partially longer"?  Not sure what was intended.

Re: [PATCH RFC 4/6] r8169: prepare rtl_hw_aspm_clkreq_enable for usage in atomic context

[Bjorn Helgaas]

On Sat, Feb 25, 2023 at 10:46:05PM +0100, Heiner Kallweit wrote:
> Bail out if the function is used with chip versions that don't support
> ASPM configuration. In addition remove the delay, it tuned out that

turned out?

> it's not needed, also vendor driver r8125 doesn't have it.

Re: [PATCH RFC 6/6] r8169: remove ASPM restrictions now that ASPM is disabled during NAPI poll

[Bjorn Helgaas]

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

Re: [PATCH RFC 6/6] r8169: remove ASPM restrictions now that ASPM is disabled during NAPI poll

[Heiner Kallweit]

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