[PATCH net-next] r8169: Use a raw_spinlock_t for the register locks.

[PATCH net-next] r8169: Use a raw_spinlock_t for the register locks.

[Sebastian Andrzej Siewior]

The driver's interrupt service routine is requested with the
IRQF_NO_THREAD if MSI is available. This means that the routine is
invoked in hardirq context even on PREEMPT_RT. The routine itself is
relatively short and schedules a worker, performs register access and
schedules NAPI. On PREEMPT_RT, scheduling NAPI from hardirq results in
waking ksoftirqd for further processing so using NAPI threads with this
driver is highly recommended since it NULL routes the threaded-IRQ
efforts.

Adding rtl_hw_aspm_clkreq_enable() to the ISR is problematic on
PREEMPT_RT because the function uses spinlock_t locks which become
sleeping locks on PREEMPT_RT. The locks are only used to protect
register access and don't nest into other functions or locks. They are
also not used for unbounded period of time. Therefore it looks okay to
convert them to raw_spinlock_t.

Convert the three locks which are used from the interrupt service
routine to raw_spinlock_t.

Fixes: e1ed3e4d91112 ("r8169: disable ASPM during NAPI poll")
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---

This is the only ethernet driver doing IRQF_NO_THREAD. It was recently
enabled and based on thread it was argued to offer better performance
with threaded interrupts and NAPI threads. So I'm trying this instead of
reverting the whole thing since it does not seem necessary. Maybe the
NAPI-threads could some special treats, I take a look.

 drivers/net/ethernet/realtek/r8169_main.c | 44 +++++++++++------------
 1 file changed, 22 insertions(+), 22 deletions(-)

diff --git a/drivers/net/ethernet/realtek/r8169_main.c b/drivers/net/ethernet/realtek/r8169_main.c
index a7e376e7e689c..4b19803a7dd01 100644
--- a/drivers/net/ethernet/realtek/r8169_main.c
+++ b/drivers/net/ethernet/realtek/r8169_main.c
@@ -616,10 +616,10 @@ struct rtl8169_private {
 		struct work_struct work;
 	} wk;
 
-	spinlock_t config25_lock;
-	spinlock_t mac_ocp_lock;
+	raw_spinlock_t config25_lock;
+	raw_spinlock_t mac_ocp_lock;
 
-	spinlock_t cfg9346_usage_lock;
+	raw_spinlock_t cfg9346_usage_lock;
 	int cfg9346_usage_count;
 
 	unsigned supports_gmii:1;
@@ -671,20 +671,20 @@ static void rtl_lock_config_regs(struct rtl8169_private *tp)
 {
 	unsigned long flags;
 
-	spin_lock_irqsave(&tp->cfg9346_usage_lock, flags);
+	raw_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);
+	raw_spin_unlock_irqrestore(&tp->cfg9346_usage_lock, flags);
 }
 
 static void rtl_unlock_config_regs(struct rtl8169_private *tp)
 {
 	unsigned long flags;
 
-	spin_lock_irqsave(&tp->cfg9346_usage_lock, flags);
+	raw_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);
+	raw_spin_unlock_irqrestore(&tp->cfg9346_usage_lock, flags);
 }
 
 static void rtl_pci_commit(struct rtl8169_private *tp)
@@ -698,10 +698,10 @@ 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);
+	raw_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);
+	raw_spin_unlock_irqrestore(&tp->config25_lock, flags);
 }
 
 static void rtl_mod_config5(struct rtl8169_private *tp, u8 clear, u8 set)
@@ -709,10 +709,10 @@ 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);
+	raw_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);
+	raw_spin_unlock_irqrestore(&tp->config25_lock, flags);
 }
 
 static bool rtl_is_8125(struct rtl8169_private *tp)
@@ -899,9 +899,9 @@ 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);
+	raw_spin_lock_irqsave(&tp->mac_ocp_lock, flags);
 	__r8168_mac_ocp_write(tp, reg, data);
-	spin_unlock_irqrestore(&tp->mac_ocp_lock, flags);
+	raw_spin_unlock_irqrestore(&tp->mac_ocp_lock, flags);
 }
 
 static u16 __r8168_mac_ocp_read(struct rtl8169_private *tp, u32 reg)
@@ -919,9 +919,9 @@ 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);
+	raw_spin_lock_irqsave(&tp->mac_ocp_lock, flags);
 	val = __r8168_mac_ocp_read(tp, reg);
-	spin_unlock_irqrestore(&tp->mac_ocp_lock, flags);
+	raw_spin_unlock_irqrestore(&tp->mac_ocp_lock, flags);
 
 	return val;
 }
@@ -932,10 +932,10 @@ static void r8168_mac_ocp_modify(struct rtl8169_private *tp, u32 reg, u16 mask,
 	unsigned long flags;
 	u16 data;
 
-	spin_lock_irqsave(&tp->mac_ocp_lock, flags);
+	raw_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);
+	raw_spin_unlock_irqrestore(&tp->mac_ocp_lock, flags);
 }
 
 /* Work around a hw issue with RTL8168g PHY, the quirk disables
@@ -1420,14 +1420,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);
+	raw_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);
+	raw_spin_unlock_irqrestore(&tp->config25_lock, flags);
 
 	switch (tp->mac_version) {
 	case RTL_GIGA_MAC_VER_02 ... RTL_GIGA_MAC_VER_06:
@@ -5179,9 +5179,9 @@ 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);
+	raw_spin_lock_init(&tp->cfg9346_usage_lock);
+	raw_spin_lock_init(&tp->config25_lock);
+	raw_spin_lock_init(&tp->mac_ocp_lock);
 
 	dev->tstats = devm_netdev_alloc_pcpu_stats(&pdev->dev,
 						   struct pcpu_sw_netstats);
-- 
2.40.1

Re: [PATCH net-next] r8169: Use a raw_spinlock_t for the register locks.

[Heiner Kallweit]

On 22.05.2023 15:41, Sebastian Andrzej Siewior wrote:
> The driver's interrupt service routine is requested with the
> IRQF_NO_THREAD if MSI is available. This means that the routine is
> invoked in hardirq context even on PREEMPT_RT. The routine itself is
> relatively short and schedules a worker, performs register access and
> schedules NAPI. On PREEMPT_RT, scheduling NAPI from hardirq results in
> waking ksoftirqd for further processing so using NAPI threads with this
> driver is highly recommended since it NULL routes the threaded-IRQ
> efforts.
> 
> Adding rtl_hw_aspm_clkreq_enable() to the ISR is problematic on
> PREEMPT_RT because the function uses spinlock_t locks which become
> sleeping locks on PREEMPT_RT. The locks are only used to protect
> register access and don't nest into other functions or locks. They are
> also not used for unbounded period of time. Therefore it looks okay to
> convert them to raw_spinlock_t.
> 
> Convert the three locks which are used from the interrupt service
> routine to raw_spinlock_t.
> 
> Fixes: e1ed3e4d91112 ("r8169: disable ASPM during NAPI poll")
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> ---
> 
> This is the only ethernet driver doing IRQF_NO_THREAD. It was recently
> enabled and based on thread it was argued to offer better performance
> with threaded interrupts and NAPI threads. So I'm trying this instead of
> reverting the whole thing since it does not seem necessary. Maybe the
> NAPI-threads could some special treats, I take a look.
> 
>  drivers/net/ethernet/realtek/r8169_main.c | 44 +++++++++++------------
>  1 file changed, 22 insertions(+), 22 deletions(-)
> 

Reviewed-by: Heiner Kallweit <hkallweit1@gmail.com>

Re: [PATCH net-next] r8169: Use a raw_spinlock_t for the register locks.

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net.git (main)
by Jakub Kicinski <kuba@kernel.org>:

On Mon, 22 May 2023 15:41:21 +0200 you wrote:
> The driver's interrupt service routine is requested with the
> IRQF_NO_THREAD if MSI is available. This means that the routine is
> invoked in hardirq context even on PREEMPT_RT. The routine itself is
> relatively short and schedules a worker, performs register access and
> schedules NAPI. On PREEMPT_RT, scheduling NAPI from hardirq results in
> waking ksoftirqd for further processing so using NAPI threads with this
> driver is highly recommended since it NULL routes the threaded-IRQ
> efforts.
> 
> [...]

Here is the summary with links:
  - [net-next] r8169: Use a raw_spinlock_t for the register locks.
    https://git.kernel.org/netdev/net/c/d6c36cbc5e53

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html