[PATCH net] net: macb: take bp->lock around NCR read-modify-writes

[PATCH net] net: macb: take bp->lock around NCR read-modify-writes

[Théo Lebrun]

NCR is read-modify-written from many contexts:

 - macb_mac_link_down() clears RE|TE,
 - macb_mac_link_up() sets RE|TE|PTPUNI,
 - macb_hresp_error_task() clears then re-sets RE|TE,
 - macb_start_xmit() / macb_tx_restart() / macb_tx_error_task() set
   TSTART (already under bp->lock),
 - macb_interrupt() might toggle RE (also under bp->lock).

The first three risk concurrent RMW with anyone from the list as they
don't grab bp->lock.

Theoretical bugfix only, it has never reproduced on hardware. Also note
it does *not* take the lock hoping to protect against full races
inbetween BH scheduling concurrently. We only want to ensure consistent
NCR RMW operations.

Fixes: 7897b071ac3b ("net: macb: convert to phylink")
Cc: stable@vger.kernel.org
Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>
---
 drivers/net/ethernet/cadence/macb_main.c | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
index b8234ac4b602..b58d3f01f2cc 100644
--- a/drivers/net/ethernet/cadence/macb_main.c
+++ b/drivers/net/ethernet/cadence/macb_main.c
@@ -777,6 +777,7 @@ static void macb_mac_link_down(struct phylink_config *config, unsigned int mode,
 	struct net_device *netdev = to_net_dev(config->dev);
 	struct macb *bp = netdev_priv(netdev);
 	struct macb_queue *queue;
+	unsigned long flags;
 	unsigned int q;
 	u32 ctrl;
 
@@ -786,8 +787,10 @@ static void macb_mac_link_down(struct phylink_config *config, unsigned int mode,
 				     bp->rx_intr_mask | MACB_TX_INT_FLAGS | MACB_BIT(HRESP));
 
 	/* Disable Rx and Tx */
+	spin_lock_irqsave(&bp->lock, flags);
 	ctrl = macb_readl(bp, NCR) & ~(MACB_BIT(RE) | MACB_BIT(TE));
 	macb_writel(bp, NCR, ctrl);
+	spin_unlock_irqrestore(&bp->lock, flags);
 
 	netif_tx_stop_all_queues(netdev);
 }
@@ -940,11 +943,13 @@ static void macb_mac_link_up(struct phylink_config *config,
 	}
 
 	/* Enable Rx and Tx; Enable PTP unicast */
+	spin_lock_irqsave(&bp->lock, flags);
 	ctrl = macb_readl(bp, NCR);
 	if (gem_has_ptp(bp))
 		ctrl |= MACB_BIT(PTPUNI);
 
 	macb_writel(bp, NCR, ctrl | MACB_BIT(RE) | MACB_BIT(TE));
+	spin_unlock_irqrestore(&bp->lock, flags);
 
 	netif_tx_wake_all_queues(netdev);
 }
@@ -1995,6 +2000,7 @@ static void macb_hresp_error_task(struct work_struct *work)
 	struct macb *bp = from_work(bp, work, hresp_err_bh_work);
 	struct net_device *netdev = bp->netdev;
 	struct macb_queue *queue;
+	unsigned long flags;
 	unsigned int q;
 	u32 ctrl;
 
@@ -2003,9 +2009,11 @@ static void macb_hresp_error_task(struct work_struct *work)
 					 MACB_TX_INT_FLAGS |
 					 MACB_BIT(HRESP));
 	}
+	spin_lock_irqsave(&bp->lock, flags);
 	ctrl = macb_readl(bp, NCR);
 	ctrl &= ~(MACB_BIT(RE) | MACB_BIT(TE));
 	macb_writel(bp, NCR, ctrl);
+	spin_unlock_irqrestore(&bp->lock, flags);
 
 	netif_tx_stop_all_queues(netdev);
 	netif_carrier_off(netdev);
@@ -2022,8 +2030,10 @@ static void macb_hresp_error_task(struct work_struct *work)
 			     MACB_TX_INT_FLAGS |
 			     MACB_BIT(HRESP));
 
-	ctrl |= MACB_BIT(RE) | MACB_BIT(TE);
-	macb_writel(bp, NCR, ctrl);
+	spin_lock_irqsave(&bp->lock, flags);
+	ctrl = MACB_BIT(RE) | MACB_BIT(TE);
+	macb_writel(bp, NCR, macb_readl(bp, NCR) | ctrl);
+	spin_unlock_irqrestore(&bp->lock, flags);
 
 	netif_carrier_on(netdev);
 	netif_tx_start_all_queues(netdev);

---
base-commit: 994db8ab9d90c64dd641b7ead6efe2eaea7a50dc
change-id: 20260918-macb-ncr-rmw-be5137e60f0b

Best regards,
--  
Théo Lebrun <theo.lebrun@bootlin.com>

Re: [PATCH net] net: macb: take bp->lock around NCR read-modify-writes

[Nicolai Buchwitz]

Hi Théo

On 18.9.2026 21:59, Théo Lebrun wrote:
> NCR is read-modify-written from many contexts:
> 
>  - macb_mac_link_down() clears RE|TE,
>  - macb_mac_link_up() sets RE|TE|PTPUNI,
>  - macb_hresp_error_task() clears then re-sets RE|TE,
>  - macb_start_xmit() / macb_tx_restart() / macb_tx_error_task() set
>    TSTART (already under bp->lock),
>  - macb_interrupt() might toggle RE (also under bp->lock).
> 
> The first three risk concurrent RMW with anyone from the list as they
> don't grab bp->lock.

There is a fourth, in at91ether_interrupt():

      if (intstatus & MACB_BIT(RXUBR)) {
              ctl = macb_readl(bp, NCR);
              macb_writel(bp, NCR, ctl & ~MACB_BIT(RE));
              wmb();
              macb_writel(bp, NCR, ctl | MACB_BIT(RE));
      }

I guess you have skipped this as the emac variants using this are not 
SMP?

> [...]

Reviewed-by: Nicolai Buchwitz <nb@tipi-net.de>

Regards
Nicolai

Re: [PATCH net] net: macb: take bp->lock around NCR read-modify-writes

[Théo Lebrun]

Hello Nicolai,

On Fri Sep 18, 2026 at 10:31 PM CEST, Nicolai Buchwitz wrote:
> On 18.9.2026 21:59, Théo Lebrun wrote:
>> NCR is read-modify-written from many contexts:
>> 
>>  - macb_mac_link_down() clears RE|TE,
>>  - macb_mac_link_up() sets RE|TE|PTPUNI,
>>  - macb_hresp_error_task() clears then re-sets RE|TE,
>>  - macb_start_xmit() / macb_tx_restart() / macb_tx_error_task() set
>>    TSTART (already under bp->lock),
>>  - macb_interrupt() might toggle RE (also under bp->lock).
>> 
>> The first three risk concurrent RMW with anyone from the list as they
>> don't grab bp->lock.
>
> There is a fourth, in at91ether_interrupt():
>
>       if (intstatus & MACB_BIT(RXUBR)) {
>               ctl = macb_readl(bp, NCR);
>               macb_writel(bp, NCR, ctl & ~MACB_BIT(RE));
>               wmb();
>               macb_writel(bp, NCR, ctl | MACB_BIT(RE));
>       }
>
> I guess you have skipped this as the emac variants using this are not 
> SMP?

Actually I skipped over this at91ether one because at91ether functions
use zero locking. But as you point out, yes, RMWs are safe from
interrupt context on a non-SMP platform.

> Reviewed-by: Nicolai Buchwitz <nb@tipi-net.de>

Thanks!

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