* Re: [PATCH net] net: macb: take bp->lock around NCR read-modify-writes
2026-09-18 19:59 [PATCH net] net: macb: take bp->lock around NCR read-modify-writes Théo Lebrun
@ 2026-09-18 20:31 ` Nicolai Buchwitz
2026-09-21 14:36 ` Théo Lebrun
2026-09-22 8:00 ` netdev-bot+sashiko
1 sibling, 1 reply; 4+ messages in thread
From: Nicolai Buchwitz @ 2026-09-18 20:31 UTC (permalink / raw)
To: Théo Lebrun
Cc: Conor Dooley, Andrew Lunn, David S. Miller, Eric Dumazet,
Jakub Kicinski, Paolo Abeni, Simon Horman, Nicolas Ferre,
Sean Anderson, Antoine Tenart, netdev, linux-kernel,
Vladimir Kondratiev, Gregory CLEMENT, Tawfik Bayouk,
Thomas Petazzoni, stable
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
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH net] net: macb: take bp->lock around NCR read-modify-writes
2026-09-18 19:59 [PATCH net] net: macb: take bp->lock around NCR read-modify-writes Théo Lebrun
2026-09-18 20:31 ` Nicolai Buchwitz
@ 2026-09-22 8:00 ` netdev-bot+sashiko
1 sibling, 0 replies; 4+ messages in thread
From: netdev-bot+sashiko @ 2026-09-22 8:00 UTC (permalink / raw)
To: theo.lebrun
Cc: conor.dooley, andrew+netdev, davem, edumazet, kuba, pabeni,
horms, nicolas.ferre, sean.anderson, atenart, netdev,
linux-kernel, nb, vladimir.kondratiev, gregory.clement,
tawfik.bayouk, thomas.petazzoni, stable
Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider.
Critical: 0 · High: 1 (1) · Medium: 0 (1) · Low: 0
If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).
- [High] Incomplete NCR read-modify-write serialization: the patch takes
bp->lock around the NCR RMW in macb_mac_link_down(),…
Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but the work item
it modifies (bp->hresp_err_bh_work / macb_hresp_error_task) is queued…
- [Medium] macb_hresp_error_task() performs a multi-step RE/TE state
transition that per-RMW locking cannot protect: it clears NCR.RE|TE…
--
net: macb: take bp->lock around NCR read-modify-writes
The patch wraps the NCR read-modify-write sequences in
macb_mac_link_down(), macb_mac_link_up() and macb_hresp_error_task() with
spin_lock_irqsave(&bp->lock)/spin_unlock_irqrestore(), so that they no
longer race with the TSTART and RE writers that already hold bp->lock.
It is described as a theoretical fix that only aims at consistent NCR
read-modify-write operations.
> 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
[ ... ]
> @@ -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);
> }
[Severity: High]
The commit message enumerates the NCR read-modify-write sites, but a few
more exist that still do not take bp->lock. Should they be converted
too, or the changelog mention that they are knowingly left out?
In macb_ptp.c, gem_ptp_set_one_step_sync() does:
reg_val = macb_readl(bp, NCR);
if (enable)
macb_writel(bp, NCR, reg_val | MACB_BIT(OSSMODE));
else
macb_writel(bp, NCR, reg_val & ~MACB_BIT(OSSMODE));
and gem_set_hwtst() does:
regval = macb_readl(bp, NCR);
macb_writel(bp, NCR, (regval | MACB_BIT(SRTSM)));
Both are reached from ndo_hwtstamp_set -> macb_hwtstamp_set(), which
returns -EINVAL unless netif_running(), so they only run while the
interface is up, concurrently with the newly locked phylink callbacks and
with the bp->lock-holding macb_start_xmit()/macb_interrupt() writers.
Can that lose an update in either direction? If the hwtstamp path samples
NCR, macb_mac_link_up() then sets RE|TE(|PTPUNI) under bp->lock, and the
hwtstamp path writes back its stale image, RE/TE end up clear while the
driver has already woken the queues; in the mirror case SRTSM/OSSMODE is
dropped and hardware timestamping is not armed.
The at91ether paths have the same shape. at91ether_interrupt() does an
NCR read-modify-write from hard IRQ with no lock at all:
/* Work-around for EMAC Errata section 41.3.1 */
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));
}
Since the driver registers a single phylink instance with
macb_phylink_ops, EMAC parts use exactly the macb_mac_link_up() /
macb_mac_link_down() callbacks being locked here, while nothing on the
at91ether side takes bp->lock. Can this handler re-arm RE right after
macb_mac_link_down() cleared it, or right after at91ether_stop() cleared
RE|TE and went on to at91ether_free_coherent()? at91ether_start(),
at91ether_stop() and at91ether_open() also do unlocked NCR
read-modify-writes.
Finally, macb_reset_hw() performs the same RE|TE clear plus CLRSTAT
read-modify-write and is reached both with bp->lock held (macb_close(),
macb_suspend()) and without it (macb_init_hw() from macb_open() and
macb_resume()), so the locking rule for NCR is not uniform after this
patch. Would funnelling all NCR read-modify-writes through one locked
helper be preferable?
[ ... ]
> @@ -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);
> }
[ ... ]
> @@ -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);
[Severity: High]
This isn't a bug introduced by this patch, but while looking at the work
item being modified here: bp->hresp_err_bh_work is queued from hard IRQ in
macb_interrupt():
if (status & MACB_BIT(HRESP)) {
queue_work(system_bh_wq, &bp->hresp_err_bh_work);
and it is never cancelled on the close path. macb_close() only does
cancel_delayed_work_sync(&bp->tx_lpi_work) before macb_reset_hw() and
macb_free(bp); the single cancel_work_sync(&bp->hresp_err_bh_work) in the
driver lives in macb_remove().
macb_free() dma_free_coherent()s the descriptor rings and then does:
queue->tx_ring = NULL;
queue->rx_ring = NULL;
If an HRESP interrupt arrives just before dev_close() clears
__LINK_STATE_START, so macb_interrupt()'s !netif_running() bail does not
fire, can the work still run after macb_free()? macb_hresp_error_task()
has no netif_running() check and calls:
bp->macbgem_ops.mog_init_rings(bp);
/* Initialize TX and RX buffers */
macb_init_buffers(bp);
which dereference queue->tx_ring / queue->rx_ring and reprogram
queue->RBQP/TBQP with the freed DMA addresses, then re-enable RE|TE and
call netif_carrier_on() / netif_tx_start_all_queues() on a closed netdev.
Would a cancel_work_sync(&bp->hresp_err_bh_work) in macb_close() (or a
state check in the worker) be appropriate?
> @@ -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);
[Severity: Medium]
This is a pre-existing property rather than something introduced here, and
the changelog does say the lock is not taken "hoping to protect against
full races inbetween BH scheduling concurrently", so this is mostly a note
on what the new critical sections do and do not cover.
macb_hresp_error_task() is a multi-step transition: it clears RE|TE under
bp->lock, drops the lock, rebuilds hardware-visible state outside it
(mog_init_rings() and macb_init_buffers() rewrite descriptor fields and
queue->RBQP/TBQP), and only re-takes the lock for the final unconditional
RE|TE set above.
During that unlocked window macb_mac_link_up() can take bp->lock and set
RE|TE, and an in-flight macb_interrupt() that already sampled RXUBR runs:
ctrl = macb_readl(bp, NCR);
macb_writel(bp, NCR, ctrl & ~MACB_BIT(RE));
wmb();
macb_writel(bp, NCR, ctrl | MACB_BIT(RE));
which restores RE regardless of the worker having deliberately cleared it.
Can the engines then fetch descriptors while the rings are still being
rewritten?
The reverse direction is also visible: if macb_mac_link_down() clears
RE|TE and stops the queues while recovery is in progress, the final hunk
sets RE|TE and calls netif_carrier_on() / netif_tx_start_all_queues()
anyway, undoing the link-down transition. Would it be worth stating in
the changelog that these multi-step sequences remain uncoordinated?
--
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260918-macb-ncr-rmw-v1-1-30c3494f93cd%40bootlin.com
^ permalink raw reply [flat|nested] 4+ messages in thread