Re: [PATCH net-next 3/4] net: macb: Disable one-step mode when TX timestamping is off

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 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).

- [Medium] gem_set_hwtst() now performs an unconditional read-modify-
  write of NCR under bp->lock (macb_ptp.c:446-448), including for…

--

net: macb: Disable one-step mode when TX timestamping is off

gem_set_hwtst() left NCR.OSSMODE set when switching from
HWTSTAMP_TX_ONESTEP_SYNC to HWTSTAMP_TX_OFF.  The patch always includes
OSSMODE in the NCR update mask and sets the bit only for
HWTSTAMP_TX_ONESTEP_SYNC, so the empty-mask check around the NCR
read-modify-write is dropped.

> diff --git a/drivers/net/ethernet/cadence/macb_ptp.c b/drivers/net/ethernet/cadence/macb_ptp.c
> index b6d17fef429f6..6376b663130d8 100644
> --- a/drivers/net/ethernet/cadence/macb_ptp.c
> +++ b/drivers/net/ethernet/cadence/macb_ptp.c

[ ... ]

> @@ -445,11 +444,9 @@ int gem_set_hwtst(struct net_device *netdev,
>  	}
>  
>  	spin_lock_irqsave(&bp->lock, flags);
> -	if (ncr_mask) {
> -		regval = macb_readl(bp, NCR);
> -		regval = (regval & ~ncr_mask) | ncr_bits;
> -		macb_writel(bp, NCR, regval);
> -	}
> +	regval = macb_readl(bp, NCR);
> +	regval = (regval & ~ncr_mask) | ncr_bits;
> +	macb_writel(bp, NCR, regval);

[Severity: Medium]
Is bp->lock enough to make this NCR read-modify-write atomic against the
other NCR writers in the driver?

macb_tx_lpi_set() in macb_main.c documents bp->lock as the intended
serialisation for NCR:

	lockdep_assert_held(&bp->lock);

	ncr = macb_readl(bp, NCR);

but several other NCR read-modify-write sites do not take it.
macb_mac_link_down():

	/* Disable Rx and Tx */
	ctrl = macb_readl(bp, NCR) & ~(MACB_BIT(RE) | MACB_BIT(TE));
	macb_writel(bp, NCR, ctrl);

macb_mac_link_up() drops bp->lock first and only then touches NCR:

	spin_unlock_irqrestore(&bp->lock, flags);
	...
	/* Enable Rx and Tx; Enable PTP unicast */
	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));

macb_interrupt_misc() does the same thing from hardirq context, and
macb_hresp_error_task() from a work item, neither under bp->lock.

gem_set_hwtst() runs from ndo_hwtstamp_set under rtnl, while
mac_link_up/mac_link_down run from phylink_resolve() under
pl->state_mutex, so can these two run concurrently on different CPUs?

If so, can the OSSMODE clear be lost like this?

CPU0 phylink_resolve()->macb_mac_link_up()
	ctrl = macb_readl(bp, NCR);		/* OSSMODE still set */

CPU1 gem_set_hwtst(HWTSTAMP_TX_OFF)
	regval = (regval & ~MACB_BIT(OSSMODE));
	macb_writel(bp, NCR, regval);
	bp->tstamp_config = *tstamp_config;	/* records TX_OFF */

CPU0
	macb_writel(bp, NCR, ctrl | MACB_BIT(RE) | MACB_BIT(TE));
						/* OSSMODE back on */

That leaves one-step insertion enabled in hardware while the cached
config reports HWTSTAMP_TX_OFF, which is the divergence the commit
message describes removing.

The reverse direction looks newly reachable too, since a
HWTSTAMP_TX_OFF plus HWTSTAMP_FILTER_NONE request previously skipped the
register access entirely and now always writes NCR:

CPU0 gem_set_hwtst()
	regval = macb_readl(bp, NCR);		/* RE|TE set */

CPU1 macb_mac_link_down()
	macb_writel(bp, NCR, ctrl & ~(RE|TE));

CPU0
	macb_writel(bp, NCR, regval & ~ncr_mask);	/* RE|TE restored */

Would it make sense to convert the remaining NCR read-modify-write sites
to take bp->lock, so the assumption this patch relies on actually holds?
Note spin_lock_irqsave() here does not exclude the macb_interrupt_misc()
NCR update running on another CPU.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260922-codex-macb-hwtstamp-submit-v1-0-9d1abaa53296%40khu.ac.kr