[PATCH net 0/2] net: macb: Fix two lock warnings when WOL is used

[PATCH net 0/2] net: macb: Fix two lock warnings when WOL is used

[Kevin Hao]

Hi,

This patch series addresses two lock warnings that occur when using WOL as a
wakeup source on my AMD ZynqMP board.

---
Cc: Nicolas Ferre <nicolas.ferre@microchip.com>
Cc: Claudiu Beznea <claudiu.beznea@tuxon.dev>
Cc: Andrew Lunn <andrew+netdev@lunn.ch>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: Eric Dumazet <edumazet@google.com>
Cc: Jakub Kicinski <kuba@kernel.org>
Cc: Paolo Abeni <pabeni@redhat.com>
Cc: Vineeth Karumanchi <vineeth.karumanchi@amd.com>
Cc: Harini Katakam <harini.katakam@amd.com>

---
Kevin Hao (2):
      net: macb: Move devm_{free,request}_irq() out of spin lock area
      net: macb: Protect access to net_device::in_ptr with RCU lock

 drivers/net/ethernet/cadence/macb_main.c | 16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)
---
base-commit: 6ba8fb373522ad9ee3e828c8e77d8bd1acf3dc33
change-id: 20260311-macb-irq-39b8a3333bd8

Best regards,
-- 
Kevin Hao <haokexin@gmail.com>

[PATCH net 1/2] net: macb: Move devm_{free,request}_irq() out of spin lock area

[Kevin Hao]

The devm_free_irq() and devm_request_irq() functions should not be
executed in an atomic context.

During device suspend, all userspace processes and most kernel threads
are frozen. Additionally, we flush all tx/rx status, disable all macb
interrupts, and halt rx operations. Therefore, it is safe to split the
region protected by bp->lock into two independent sections, allowing
devm_free_irq() and devm_request_irq() to run in a non-atomic context.
This modification resolves the following lockdep warning:
  BUG: sleeping function called from invalid context at kernel/locking/mutex.c:591
  in_atomic(): 1, irqs_disabled(): 1, non_block: 0, pid: 501, name: rtcwake
  preempt_count: 1, expected: 0
  RCU nest depth: 1, expected: 0
  7 locks held by rtcwake/501:
   #0: ffff0008038c3408 (sb_writers#5){.+.+}-{0:0}, at: vfs_write+0xf8/0x368
   #1: ffff0008049a5e88 (&of->mutex#2){+.+.}-{4:4}, at: kernfs_fop_write_iter+0xbc/0x1c8
   #2: ffff00080098d588 (kn->active#70){.+.+}-{0:0}, at: kernfs_fop_write_iter+0xcc/0x1c8
   #3: ffff800081c84888 (system_transition_mutex){+.+.}-{4:4}, at: pm_suspend+0x1ec/0x290
   #4: ffff0008009ba0f8 (&dev->mutex){....}-{4:4}, at: device_suspend+0x118/0x4f0
   #5: ffff800081d00458 (rcu_read_lock){....}-{1:3}, at: rcu_lock_acquire+0x4/0x48
   #6: ffff0008031fb9e0 (&bp->lock){-.-.}-{3:3}, at: macb_suspend+0x144/0x558
  irq event stamp: 8682
  hardirqs last  enabled at (8681): [<ffff8000813c7d7c>] _raw_spin_unlock_irqrestore+0x44/0x88
  hardirqs last disabled at (8682): [<ffff8000813c7b58>] _raw_spin_lock_irqsave+0x38/0x98
  softirqs last  enabled at (7322): [<ffff8000800f1b4c>] handle_softirqs+0x52c/0x588
  softirqs last disabled at (7317): [<ffff800080010310>] __do_softirq+0x20/0x2c
  CPU: 1 UID: 0 PID: 501 Comm: rtcwake Not tainted 7.0.0-rc3-next-20260310-yocto-standard+ #125 PREEMPT
  Hardware name: ZynqMP ZCU102 Rev1.1 (DT)
  Call trace:
   show_stack+0x24/0x38 (C)
   __dump_stack+0x28/0x38
   dump_stack_lvl+0x64/0x88
   dump_stack+0x18/0x24
   __might_resched+0x200/0x218
   __might_sleep+0x38/0x98
   __mutex_lock_common+0x7c/0x1378
   mutex_lock_nested+0x38/0x50
   free_irq+0x68/0x2b0
   devm_irq_release+0x24/0x38
   devres_release+0x40/0x80
   devm_free_irq+0x48/0x88
   macb_suspend+0x298/0x558
   device_suspend+0x218/0x4f0
   dpm_suspend+0x244/0x3a0
   dpm_suspend_start+0x50/0x78
   suspend_devices_and_enter+0xec/0x560
   pm_suspend+0x194/0x290
   state_store+0x110/0x158
   kobj_attr_store+0x1c/0x30
   sysfs_kf_write+0xa8/0xd0
   kernfs_fop_write_iter+0x11c/0x1c8
   vfs_write+0x248/0x368
   ksys_write+0x7c/0xf8
   __arm64_sys_write+0x28/0x40
   invoke_syscall+0x4c/0xe8
   el0_svc_common+0x98/0xf0
   do_el0_svc+0x28/0x40
   el0_svc+0x54/0x1e0
   el0t_64_sync_handler+0x84/0x130
   el0t_64_sync+0x198/0x1a0

Fixes: 558e35ccfe95 ("net: macb: WoL support for GEM type of Ethernet controller")
Signed-off-by: Kevin Hao <haokexin@gmail.com>
Cc: stable@vger.kernel.org
---
 drivers/net/ethernet/cadence/macb_main.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
index ae5f0c95cf9b5df17aa34333175400f1bf1e2621..f290bc44020e76e681f403306cba998e540a4991 100644
--- a/drivers/net/ethernet/cadence/macb_main.c
+++ b/drivers/net/ethernet/cadence/macb_main.c
@@ -5962,6 +5962,7 @@ static int __maybe_unused macb_suspend(struct device *dev)
 			/* write IP address into register */
 			tmp |= MACB_BFEXT(IP, be32_to_cpu(ifa->ifa_local));
 		}
+		spin_unlock_irqrestore(&bp->lock, flags);
 
 		/* Change interrupt handler and
 		 * Enable WoL IRQ on queue 0
@@ -5974,11 +5975,12 @@ static int __maybe_unused macb_suspend(struct device *dev)
 				dev_err(dev,
 					"Unable to request IRQ %d (error %d)\n",
 					bp->queues[0].irq, err);
-				spin_unlock_irqrestore(&bp->lock, flags);
 				return err;
 			}
+			spin_lock_irqsave(&bp->lock, flags);
 			queue_writel(bp->queues, IER, GEM_BIT(WOL));
 			gem_writel(bp, WOL, tmp);
+			spin_unlock_irqrestore(&bp->lock, flags);
 		} else {
 			err = devm_request_irq(dev, bp->queues[0].irq, macb_wol_interrupt,
 					       IRQF_SHARED, netdev->name, bp->queues);
@@ -5986,13 +5988,13 @@ static int __maybe_unused macb_suspend(struct device *dev)
 				dev_err(dev,
 					"Unable to request IRQ %d (error %d)\n",
 					bp->queues[0].irq, err);
-				spin_unlock_irqrestore(&bp->lock, flags);
 				return err;
 			}
+			spin_lock_irqsave(&bp->lock, flags);
 			queue_writel(bp->queues, IER, MACB_BIT(WOL));
 			macb_writel(bp, WOL, tmp);
+			spin_unlock_irqrestore(&bp->lock, flags);
 		}
-		spin_unlock_irqrestore(&bp->lock, flags);
 
 		enable_irq_wake(bp->queues[0].irq);
 	}
@@ -6059,6 +6061,8 @@ static int __maybe_unused macb_resume(struct device *dev)
 		queue_readl(bp->queues, ISR);
 		if (bp->caps & MACB_CAPS_ISR_CLEAR_ON_WRITE)
 			queue_writel(bp->queues, ISR, -1);
+		spin_unlock_irqrestore(&bp->lock, flags);
+
 		/* Replace interrupt handler on queue 0 */
 		devm_free_irq(dev, bp->queues[0].irq, bp->queues);
 		err = devm_request_irq(dev, bp->queues[0].irq, macb_interrupt,
@@ -6067,10 +6071,8 @@ static int __maybe_unused macb_resume(struct device *dev)
 			dev_err(dev,
 				"Unable to request IRQ %d (error %d)\n",
 				bp->queues[0].irq, err);
-			spin_unlock_irqrestore(&bp->lock, flags);
 			return err;
 		}
-		spin_unlock_irqrestore(&bp->lock, flags);
 
 		disable_irq_wake(bp->queues[0].irq);
 

-- 
2.53.0

[PATCH net 2/2] net: macb: Protect access to net_device::in_ptr with RCU lock

[Kevin Hao]

Access to net_device::in_ptr and its members should be protected by RCU
lock. This resolves the following RCU check warning:
  WARNING: suspicious RCU usage
  7.0.0-rc3-next-20260310-yocto-standard+ #122 Not tainted
  -----------------------------
  drivers/net/ethernet/cadence/macb_main.c:5944 suspicious rcu_dereference_check() usage!

  other info that might help us debug this:

  rcu_scheduler_active = 2, debug_locks = 1
  5 locks held by rtcwake/518:
   #0: ffff000803ab1408 (sb_writers#5){.+.+}-{0:0}, at: vfs_write+0xf8/0x368
   #1: ffff0008090bf088 (&of->mutex#2){+.+.}-{4:4}, at: kernfs_fop_write_iter+0xbc/0x1c8
   #2: ffff00080098d588 (kn->active#70){.+.+}-{0:0}, at: kernfs_fop_write_iter+0xcc/0x1c8
   #3: ffff800081c84888 (system_transition_mutex){+.+.}-{4:4}, at: pm_suspend+0x1ec/0x290
   #4: ffff0008009ba0f8 (&dev->mutex){....}-{4:4}, at: device_suspend+0x118/0x4f0

  stack backtrace:
  CPU: 3 UID: 0 PID: 518 Comm: rtcwake Not tainted 7.0.0-rc3-next-20260310-yocto-standard+ #122 PREEMPT
  Hardware name: ZynqMP ZCU102 Rev1.1 (DT)
  Call trace:
   show_stack+0x24/0x38 (C)
   __dump_stack+0x28/0x38
   dump_stack_lvl+0x64/0x88
   dump_stack+0x18/0x24
   lockdep_rcu_suspicious+0x134/0x1d8
   macb_suspend+0xd8/0x4c0
   device_suspend+0x218/0x4f0
   dpm_suspend+0x244/0x3a0
   dpm_suspend_start+0x50/0x78
   suspend_devices_and_enter+0xec/0x560
   pm_suspend+0x194/0x290
   state_store+0x110/0x158
   kobj_attr_store+0x1c/0x30
   sysfs_kf_write+0xa8/0xd0
   kernfs_fop_write_iter+0x11c/0x1c8
   vfs_write+0x248/0x368
   ksys_write+0x7c/0xf8
   __arm64_sys_write+0x28/0x40
   invoke_syscall+0x4c/0xe8
   el0_svc_common+0x98/0xf0
   do_el0_svc+0x28/0x40
   el0_svc+0x54/0x1e0
   el0t_64_sync_handler+0x84/0x130
   el0t_64_sync+0x198/0x1a0

Fixes: 0cb8de39a776 ("net: macb: Add ARP support to WOL")
Signed-off-by: Kevin Hao <haokexin@gmail.com>
Cc: stable@vger.kernel.org
---
 drivers/net/ethernet/cadence/macb_main.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
index f290bc44020e76e681f403306cba998e540a4991..d35bb5f079cd103eea5af7584576c9582a18d22a 100644
--- a/drivers/net/ethernet/cadence/macb_main.c
+++ b/drivers/net/ethernet/cadence/macb_main.c
@@ -5915,13 +5915,16 @@ static int __maybe_unused macb_suspend(struct device *dev)
 
 	if (bp->wol & MACB_WOL_ENABLED) {
 		/* Check for IP address in WOL ARP mode */
+		rcu_read_lock();
 		idev = __in_dev_get_rcu(bp->dev);
 		if (idev)
 			ifa = rcu_dereference(idev->ifa_list);
 		if ((bp->wolopts & WAKE_ARP) && !ifa) {
 			netdev_err(netdev, "IP address not assigned as required by WoL walk ARP\n");
+			rcu_read_unlock();
 			return -EOPNOTSUPP;
 		}
+
 		spin_lock_irqsave(&bp->lock, flags);
 
 		/* Disable Tx and Rx engines before  disabling the queues,
@@ -5963,6 +5966,7 @@ static int __maybe_unused macb_suspend(struct device *dev)
 			tmp |= MACB_BFEXT(IP, be32_to_cpu(ifa->ifa_local));
 		}
 		spin_unlock_irqrestore(&bp->lock, flags);
+		rcu_read_unlock();
 
 		/* Change interrupt handler and
 		 * Enable WoL IRQ on queue 0

-- 
2.53.0

Re: [PATCH net 2/2] net: macb: Protect access to net_device::in_ptr with RCU lock

[Théo Lebrun]

Hello Kevin,

On Sun Mar 15, 2026 at 12:44 PM CET, Kevin Hao wrote:
> @@ -5915,13 +5915,16 @@ static int __maybe_unused macb_suspend(struct device *dev)
>  
>  	if (bp->wol & MACB_WOL_ENABLED) {
>  		/* Check for IP address in WOL ARP mode */
> +		rcu_read_lock();
>  		idev = __in_dev_get_rcu(bp->dev);
>  		if (idev)
>  			ifa = rcu_dereference(idev->ifa_list);
>  		if ((bp->wolopts & WAKE_ARP) && !ifa) {
>  			netdev_err(netdev, "IP address not assigned as required by WoL walk ARP\n");
> +			rcu_read_unlock();
>  			return -EOPNOTSUPP;
>  		}
> +
>  		spin_lock_irqsave(&bp->lock, flags);
>  
>  		/* Disable Tx and Rx engines before  disabling the queues,
> @@ -5963,6 +5966,7 @@ static int __maybe_unused macb_suspend(struct device *dev)
>  			tmp |= MACB_BFEXT(IP, be32_to_cpu(ifa->ifa_local));
>  		}
>  		spin_unlock_irqrestore(&bp->lock, flags);
> +		rcu_read_unlock();
>  
>  		/* Change interrupt handler and
>  		 * Enable WoL IRQ on queue 0

Instead of making the RCU critical section extend so much, you could
dereference ifa->ifa_local into a stack variable. In particular, it
would avoid the RCU critical section covering a spinlock critical
section.

Thanks,

--
Théo Lebrun, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH net 1/2] net: macb: Move devm_{free,request}_irq() out of spin lock area

[Théo Lebrun]

On Sun Mar 15, 2026 at 12:44 PM CET, Kevin Hao wrote:
> @@ -5962,6 +5962,7 @@ static int __maybe_unused macb_suspend(struct device *dev)
>  			/* write IP address into register */
>  			tmp |= MACB_BFEXT(IP, be32_to_cpu(ifa->ifa_local));
>  		}
> +		spin_unlock_irqrestore(&bp->lock, flags);
>  
>  		/* Change interrupt handler and
>  		 * Enable WoL IRQ on queue 0
> @@ -5974,11 +5975,12 @@ static int __maybe_unused macb_suspend(struct device *dev)
>  				dev_err(dev,
>  					"Unable to request IRQ %d (error %d)\n",
>  					bp->queues[0].irq, err);
> -				spin_unlock_irqrestore(&bp->lock, flags);
>  				return err;
>  			}
> +			spin_lock_irqsave(&bp->lock, flags);
>  			queue_writel(bp->queues, IER, GEM_BIT(WOL));
>  			gem_writel(bp, WOL, tmp);
> +			spin_unlock_irqrestore(&bp->lock, flags);
>  		} else {
>  			err = devm_request_irq(dev, bp->queues[0].irq, macb_wol_interrupt,
>  					       IRQF_SHARED, netdev->name, bp->queues);
> @@ -5986,13 +5988,13 @@ static int __maybe_unused macb_suspend(struct device *dev)
>  				dev_err(dev,
>  					"Unable to request IRQ %d (error %d)\n",
>  					bp->queues[0].irq, err);
> -				spin_unlock_irqrestore(&bp->lock, flags);
>  				return err;
>  			}
> +			spin_lock_irqsave(&bp->lock, flags);
>  			queue_writel(bp->queues, IER, MACB_BIT(WOL));
>  			macb_writel(bp, WOL, tmp);
> +			spin_unlock_irqrestore(&bp->lock, flags);
>  		}
> -		spin_unlock_irqrestore(&bp->lock, flags);
>  
>  		enable_irq_wake(bp->queues[0].irq);
>  	}

So it used to be that approximatively the whole macb_suspend() function
was ran under the bp->lock spinlock. Now you split it in two to avoid
calling IRQ functions in atomic context:
 - (1) the disable queues & silence IRQs part and,
 - (2) the enable WOL part (IER and WOL reg writes).

Why do you need to grab bp->lock for the 2nd part? All queues are
disabled anyway and IRQs masked. BH features like our work queues are
disabled during the dev_pm_ops.suspend() calls anyway. Maybe I am
forgetting? Or this was just out of caution?

Thanks,

--
Théo Lebrun, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH net 1/2] net: macb: Move devm_{free,request}_irq() out of spin lock area

[Kevin Hao]

[-- Attachment #1: Type: text/plain, Size: 2434 bytes --]

On Mon, Mar 16, 2026 at 07:11:34PM +0100, Théo Lebrun wrote:
> On Sun Mar 15, 2026 at 12:44 PM CET, Kevin Hao wrote:
> > @@ -5962,6 +5962,7 @@ static int __maybe_unused macb_suspend(struct device *dev)
> >  			/* write IP address into register */
> >  			tmp |= MACB_BFEXT(IP, be32_to_cpu(ifa->ifa_local));
> >  		}
> > +		spin_unlock_irqrestore(&bp->lock, flags);
> >  
> >  		/* Change interrupt handler and
> >  		 * Enable WoL IRQ on queue 0
> > @@ -5974,11 +5975,12 @@ static int __maybe_unused macb_suspend(struct device *dev)
> >  				dev_err(dev,
> >  					"Unable to request IRQ %d (error %d)\n",
> >  					bp->queues[0].irq, err);
> > -				spin_unlock_irqrestore(&bp->lock, flags);
> >  				return err;
> >  			}
> > +			spin_lock_irqsave(&bp->lock, flags);
> >  			queue_writel(bp->queues, IER, GEM_BIT(WOL));
> >  			gem_writel(bp, WOL, tmp);
> > +			spin_unlock_irqrestore(&bp->lock, flags);
> >  		} else {
> >  			err = devm_request_irq(dev, bp->queues[0].irq, macb_wol_interrupt,
> >  					       IRQF_SHARED, netdev->name, bp->queues);
> > @@ -5986,13 +5988,13 @@ static int __maybe_unused macb_suspend(struct device *dev)
> >  				dev_err(dev,
> >  					"Unable to request IRQ %d (error %d)\n",
> >  					bp->queues[0].irq, err);
> > -				spin_unlock_irqrestore(&bp->lock, flags);
> >  				return err;
> >  			}
> > +			spin_lock_irqsave(&bp->lock, flags);
> >  			queue_writel(bp->queues, IER, MACB_BIT(WOL));
> >  			macb_writel(bp, WOL, tmp);
> > +			spin_unlock_irqrestore(&bp->lock, flags);
> >  		}
> > -		spin_unlock_irqrestore(&bp->lock, flags);
> >  
> >  		enable_irq_wake(bp->queues[0].irq);
> >  	}
> 
> So it used to be that approximatively the whole macb_suspend() function
> was ran under the bp->lock spinlock. Now you split it in two to avoid
> calling IRQ functions in atomic context:
>  - (1) the disable queues & silence IRQs part and,
>  - (2) the enable WOL part (IER and WOL reg writes).
> 
> Why do you need to grab bp->lock for the 2nd part? All queues are
> disabled anyway and IRQs masked. BH features like our work queues are
> disabled during the dev_pm_ops.suspend() calls anyway. Maybe I am
> forgetting?

You are right. I agree that the lock may not be necessary in this scenario.

> Or this was just out of caution?

This is just out of cautious consideration. Do you think I should delete these
spinlocks?

Thanks,
Kevin

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH net 2/2] net: macb: Protect access to net_device::in_ptr with RCU lock

[Kevin Hao]

[-- Attachment #1: Type: text/plain, Size: 2026 bytes --]

On Mon, Mar 16, 2026 at 06:59:35PM +0100, Théo Lebrun wrote:
> Hello Kevin,
> 
> On Sun Mar 15, 2026 at 12:44 PM CET, Kevin Hao wrote:
> > @@ -5915,13 +5915,16 @@ static int __maybe_unused macb_suspend(struct device *dev)
> >  
> >  	if (bp->wol & MACB_WOL_ENABLED) {
> >  		/* Check for IP address in WOL ARP mode */
> > +		rcu_read_lock();
> >  		idev = __in_dev_get_rcu(bp->dev);
> >  		if (idev)
> >  			ifa = rcu_dereference(idev->ifa_list);
> >  		if ((bp->wolopts & WAKE_ARP) && !ifa) {
> >  			netdev_err(netdev, "IP address not assigned as required by WoL walk ARP\n");
> > +			rcu_read_unlock();
> >  			return -EOPNOTSUPP;
> >  		}
> > +
> >  		spin_lock_irqsave(&bp->lock, flags);
> >  
> >  		/* Disable Tx and Rx engines before  disabling the queues,
> > @@ -5963,6 +5966,7 @@ static int __maybe_unused macb_suspend(struct device *dev)
> >  			tmp |= MACB_BFEXT(IP, be32_to_cpu(ifa->ifa_local));
> >  		}
> >  		spin_unlock_irqrestore(&bp->lock, flags);
> > +		rcu_read_unlock();
> >  
> >  		/* Change interrupt handler and
> >  		 * Enable WoL IRQ on queue 0
> 
> Instead of making the RCU critical section extend so much, you could
> dereference ifa->ifa_local into a stack variable. In particular, it
> would avoid the RCU critical section covering a spinlock critical
> section.

I initially considered using a local variable before submitting this, as I also
believe that `ifa->ifa_local` is unlikely to be modified or freed in this
context. However, I ultimately decided to protect these sections with RCU for
the following reasons:

- It is logically more consistent to protect access to `ifa->ifa_local` with
  RCU locking.

- For section already under spinlock protection, adding RCU locking introduces
  negligible overhead, especially in a scenario like this.

That said, I do not have a strong preference for either approach. If you prefer
using a local variable to keep the RCU region shorter, I can prepare a v2 with
that change.

Thanks,
Kevin

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH net 2/2] net: macb: Protect access to net_device::in_ptr with RCU lock

[Théo Lebrun]

On Tue Mar 17, 2026 at 2:27 AM CET, Kevin Hao wrote:
> On Mon, Mar 16, 2026 at 06:59:35PM +0100, Théo Lebrun wrote:
>> On Sun Mar 15, 2026 at 12:44 PM CET, Kevin Hao wrote:
>> > @@ -5915,13 +5915,16 @@ static int __maybe_unused macb_suspend(struct device *dev)
>> >  
>> >  	if (bp->wol & MACB_WOL_ENABLED) {
>> >  		/* Check for IP address in WOL ARP mode */
>> > +		rcu_read_lock();
>> >  		idev = __in_dev_get_rcu(bp->dev);
>> >  		if (idev)
>> >  			ifa = rcu_dereference(idev->ifa_list);
>> >  		if ((bp->wolopts & WAKE_ARP) && !ifa) {
>> >  			netdev_err(netdev, "IP address not assigned as required by WoL walk ARP\n");
>> > +			rcu_read_unlock();
>> >  			return -EOPNOTSUPP;
>> >  		}
>> > +
>> >  		spin_lock_irqsave(&bp->lock, flags);
>> >  
>> >  		/* Disable Tx and Rx engines before  disabling the queues,
>> > @@ -5963,6 +5966,7 @@ static int __maybe_unused macb_suspend(struct device *dev)
>> >  			tmp |= MACB_BFEXT(IP, be32_to_cpu(ifa->ifa_local));
>> >  		}
>> >  		spin_unlock_irqrestore(&bp->lock, flags);
>> > +		rcu_read_unlock();
>> >  
>> >  		/* Change interrupt handler and
>> >  		 * Enable WoL IRQ on queue 0
>> 
>> Instead of making the RCU critical section extend so much, you could
>> dereference ifa->ifa_local into a stack variable. In particular, it
>> would avoid the RCU critical section covering a spinlock critical
>> section.
>
> I initially considered using a local variable before submitting this, as I also
> believe that `ifa->ifa_local` is unlikely to be modified or freed in this
> context. However, I ultimately decided to protect these sections with RCU for
> the following reasons:
>
> - It is logically more consistent to protect access to `ifa->ifa_local` with
>   RCU locking.
>
> - For section already under spinlock protection, adding RCU locking introduces
>   negligible overhead, especially in a scenario like this.
>
> That said, I do not have a strong preference for either approach. If you prefer
> using a local variable to keep the RCU region shorter, I can prepare a v2 with
> that change.

I was not questioning whether this region should be protected, but
rather how long you made the RCU critical section. The smaller the
better, especially if you can remove a spinlock from it.

On PREEMPT_RT kernels it could even cause trouble because spinlocks
become sleep-able and that is not allowed inside RCU read-side critical
section.

So yes, I do insist that a tiny RCU is better; something like:

static int macb_suspend(struct device *dev)
{
	u32 ifa_local;

	// ...

	if (bp->wol & MACB_WOL_ENABLED) {
		/* Check for IP address in WOL ARP mode */
		rcu_read_lock();
		idev = __in_dev_get_rcu(bp->dev);
		if (idev)
			ifa = rcu_dereference(idev->ifa_list);
		if (ifa)
			ifa_local = be32_to_cpu(ifa->ifa_local);
		rcu_read_unlock();

		if ((bp->wolopts & WAKE_ARP) && !ifa) {
			netdev_err(netdev, "IP address not assigned as required by WoL walk ARP\n");
			return -EOPNOTSUPP;
		}

		// ...
	}

	// ...
}

Thanks,

--
Théo Lebrun, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH net 1/2] net: macb: Move devm_{free,request}_irq() out of spin lock area

[Théo Lebrun]

On Tue Mar 17, 2026 at 2:25 AM CET, Kevin Hao wrote:
> On Mon, Mar 16, 2026 at 07:11:34PM +0100, Théo Lebrun wrote:
>> On Sun Mar 15, 2026 at 12:44 PM CET, Kevin Hao wrote:
>> > @@ -5962,6 +5962,7 @@ static int __maybe_unused macb_suspend(struct device *dev)
>> >  			/* write IP address into register */
>> >  			tmp |= MACB_BFEXT(IP, be32_to_cpu(ifa->ifa_local));
>> >  		}
>> > +		spin_unlock_irqrestore(&bp->lock, flags);
>> >  
>> >  		/* Change interrupt handler and
>> >  		 * Enable WoL IRQ on queue 0
>> > @@ -5974,11 +5975,12 @@ static int __maybe_unused macb_suspend(struct device *dev)
>> >  				dev_err(dev,
>> >  					"Unable to request IRQ %d (error %d)\n",
>> >  					bp->queues[0].irq, err);
>> > -				spin_unlock_irqrestore(&bp->lock, flags);
>> >  				return err;
>> >  			}
>> > +			spin_lock_irqsave(&bp->lock, flags);
>> >  			queue_writel(bp->queues, IER, GEM_BIT(WOL));
>> >  			gem_writel(bp, WOL, tmp);
>> > +			spin_unlock_irqrestore(&bp->lock, flags);
>> >  		} else {
>> >  			err = devm_request_irq(dev, bp->queues[0].irq, macb_wol_interrupt,
>> >  					       IRQF_SHARED, netdev->name, bp->queues);
>> > @@ -5986,13 +5988,13 @@ static int __maybe_unused macb_suspend(struct device *dev)
>> >  				dev_err(dev,
>> >  					"Unable to request IRQ %d (error %d)\n",
>> >  					bp->queues[0].irq, err);
>> > -				spin_unlock_irqrestore(&bp->lock, flags);
>> >  				return err;
>> >  			}
>> > +			spin_lock_irqsave(&bp->lock, flags);
>> >  			queue_writel(bp->queues, IER, MACB_BIT(WOL));
>> >  			macb_writel(bp, WOL, tmp);
>> > +			spin_unlock_irqrestore(&bp->lock, flags);
>> >  		}
>> > -		spin_unlock_irqrestore(&bp->lock, flags);
>> >  
>> >  		enable_irq_wake(bp->queues[0].irq);
>> >  	}
>> 
>> So it used to be that approximatively the whole macb_suspend() function
>> was ran under the bp->lock spinlock. Now you split it in two to avoid
>> calling IRQ functions in atomic context:
>>  - (1) the disable queues & silence IRQs part and,
>>  - (2) the enable WOL part (IER and WOL reg writes).
>> 
>> Why do you need to grab bp->lock for the 2nd part? All queues are
>> disabled anyway and IRQs masked. BH features like our work queues are
>> disabled during the dev_pm_ops.suspend() calls anyway. Maybe I am
>> forgetting?
>
> You are right. I agree that the lock may not be necessary in this scenario.
>
>> Or this was just out of caution?
>
> This is just out of cautious consideration. Do you think I should delete these
> spinlocks?

Nah, after all no one is going to be bothered by those locks that
almost never happen. They have been here for a while after all. The
macb_resume() ones as well are probably useless because IRQs are
disabled.

Anyway, for this patch:

Reviewed-by: Théo Lebrun <theo.lebrun@bootlin.com>

Thanks,

--
Théo Lebrun, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH net 2/2] net: macb: Protect access to net_device::in_ptr with RCU lock

[Kevin Hao]

[-- Attachment #1: Type: text/plain, Size: 3613 bytes --]

On Tue, Mar 17, 2026 at 04:54:11PM +0100, Théo Lebrun wrote:
> On Tue Mar 17, 2026 at 2:27 AM CET, Kevin Hao wrote:
> > On Mon, Mar 16, 2026 at 06:59:35PM +0100, Théo Lebrun wrote:
> >> On Sun Mar 15, 2026 at 12:44 PM CET, Kevin Hao wrote:
> >> > @@ -5915,13 +5915,16 @@ static int __maybe_unused macb_suspend(struct device *dev)
> >> >  
> >> >  	if (bp->wol & MACB_WOL_ENABLED) {
> >> >  		/* Check for IP address in WOL ARP mode */
> >> > +		rcu_read_lock();
> >> >  		idev = __in_dev_get_rcu(bp->dev);
> >> >  		if (idev)
> >> >  			ifa = rcu_dereference(idev->ifa_list);
> >> >  		if ((bp->wolopts & WAKE_ARP) && !ifa) {
> >> >  			netdev_err(netdev, "IP address not assigned as required by WoL walk ARP\n");
> >> > +			rcu_read_unlock();
> >> >  			return -EOPNOTSUPP;
> >> >  		}
> >> > +
> >> >  		spin_lock_irqsave(&bp->lock, flags);
> >> >  
> >> >  		/* Disable Tx and Rx engines before  disabling the queues,
> >> > @@ -5963,6 +5966,7 @@ static int __maybe_unused macb_suspend(struct device *dev)
> >> >  			tmp |= MACB_BFEXT(IP, be32_to_cpu(ifa->ifa_local));
> >> >  		}
> >> >  		spin_unlock_irqrestore(&bp->lock, flags);
> >> > +		rcu_read_unlock();
> >> >  
> >> >  		/* Change interrupt handler and
> >> >  		 * Enable WoL IRQ on queue 0
> >> 
> >> Instead of making the RCU critical section extend so much, you could
> >> dereference ifa->ifa_local into a stack variable. In particular, it
> >> would avoid the RCU critical section covering a spinlock critical
> >> section.
> >
> > I initially considered using a local variable before submitting this, as I also
> > believe that `ifa->ifa_local` is unlikely to be modified or freed in this
> > context. However, I ultimately decided to protect these sections with RCU for
> > the following reasons:
> >
> > - It is logically more consistent to protect access to `ifa->ifa_local` with
> >   RCU locking.
> >
> > - For section already under spinlock protection, adding RCU locking introduces
> >   negligible overhead, especially in a scenario like this.
> >
> > That said, I do not have a strong preference for either approach. If you prefer
> > using a local variable to keep the RCU region shorter, I can prepare a v2 with
> > that change.
> 
> I was not questioning whether this region should be protected, but
> rather how long you made the RCU critical section. The smaller the
> better, especially if you can remove a spinlock from it.
> 
> On PREEMPT_RT kernels it could even cause trouble because spinlocks
> become sleep-able and that is not allowed inside RCU read-side critical
> section.

I'm a bit confused by this comment. As you know, the ndo_start_xmit() callback
is executed in a context where the RCU lock is acquired. Many network drivers
use spinlocks in their ndo_start_xmit() callbacks. Am I missing something obvious?

> 
> So yes, I do insist that a tiny RCU is better; something like:
> 
> static int macb_suspend(struct device *dev)
> {
> 	u32 ifa_local;
> 
> 	// ...
> 
> 	if (bp->wol & MACB_WOL_ENABLED) {
> 		/* Check for IP address in WOL ARP mode */
> 		rcu_read_lock();
> 		idev = __in_dev_get_rcu(bp->dev);
> 		if (idev)
> 			ifa = rcu_dereference(idev->ifa_list);
> 		if (ifa)
> 			ifa_local = be32_to_cpu(ifa->ifa_local);
> 		rcu_read_unlock();
> 
> 		if ((bp->wolopts & WAKE_ARP) && !ifa) {
> 			netdev_err(netdev, "IP address not assigned as required by WoL walk ARP\n");
> 			return -EOPNOTSUPP;
> 		}
> 
> 		// ...
> 	}
> 
> 	// ...
> }

Will reduce the scope of RCU lock in v2. Thanks, Lebrun.

Thanks,
Kevin

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]