Re: [PATCH net-next 4/4] net: macb: Clear SRTSM outside PTPv2 receive filters

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] Missing `Fixes:` tag and no stated user-visible impact.

Pre-existing issues:
- [Medium] ABI advertisement vs. implementation mismatch:
  gem_get_ts_info() (drivers/net/ethernet/cadence/macb_main.c:3772-3774)…
- [Medium] PTP timestamping hardware state is lost across system
  suspend/resume while the cached configuration is kept.
- [Medium] Lost-update race on the NCR MMIO register: gem_set_hwtst()
  performs its read-modify-write of NCR under…

--

net: macb: Clear SRTSM outside PTPv2 receive filters

The patch moves MACB_BIT(SRTSM) into the unconditional NCR read-modify-write
mask in gem_set_hwtst(), so the bit is now programmed from the currently
selected RX filter instead of only ever being set. Switching away from a
PTPv2 filter therefore clears it.

A few questions below, mostly about pre-existing behaviour in the same
function.

>     net: macb: Clear SRTSM outside PTPv2 receive filters
>
>     A PTPv2 receive filter sets NCR.SRTSM, but switching to NONE, ALL or a
>     PTPv1 filter leaves it set. As a result, selecting the same filter can
>     produce a different register setting depending on whether PTPv2 was
>     used before.

[Severity: Low]
Should this carry a Fixes: tag?  git blame at the parent commit points the
sticky bit at ab91f0a9b5f4 ("net: macb: Add hardware PTP support"), which
introduced:

	regval = macb_readl(bp, NCR);
	macb_writel(bp, NCR, (regval | MACB_BIT(SRTSM)));

and nothing in the tree ever cleared it since.  The three sibling patches
in this series that touch the same function ("net: macb: Preserve timestamp
settings on rejected requests", "net: macb: Enable RX timestamping for
specific PTPv1 filters", "net: macb: Disable one-step mode when TX
timestamping is off") all carry Fixes: ab91f0a9b5f4.

This patch also depends on the ncr_mask read-modify-write added by
"net: macb: Preserve timestamp settings on rejected requests", so without
a tag stable trees would get patches 1-3 and not this one, leaving the
SRTSM half of the same defect in place.

The changelog stops at "produce a different register setting" and never
names the user-visible effect.  Can the consequence of leaving SRTSM set
for a NONE/ALL/PTPv1 filter be spelled out, so it is clear whether this is
a functional fix or a consistency cleanup?

> diff --git a/drivers/net/ethernet/cadence/macb_ptp.c b/drivers/net/ethernet/cadence/macb_ptp.c
> index 6376b663130d8..637d0da3278c8 100644
> --- a/drivers/net/ethernet/cadence/macb_ptp.c
> +++ b/drivers/net/ethernet/cadence/macb_ptp.c
> @@ -390,7 +390,7 @@ int gem_set_hwtst(struct net_device *netdev,
>  		  struct kernel_hwtstamp_config *tstamp_config,
>  		  struct netlink_ext_ack *extack)
>  {
> -	u32 ncr_mask = MACB_BIT(OSSMODE);
> +	u32 ncr_mask = MACB_BIT(OSSMODE) | MACB_BIT(SRTSM);

[Severity: Medium]
This is a pre-existing issue, but can the NCR update programmed here be
lost?  gem_set_hwtst() does its read-modify-write under bp->lock:

	spin_lock_irqsave(&bp->lock, flags);
	regval = macb_readl(bp, NCR);
	regval = (regval & ~ncr_mask) | ncr_bits;
	macb_writel(bp, NCR, regval);

while macb_mac_link_up() in macb_main.c drops bp->lock and then touches the
same register with no lock held:

	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));

gem_set_hwtst() runs from ndo_hwtstamp_set under rtnl, and
macb_mac_link_up() from the phylink resolve worker under phylink's
state_mutex, so the two are not serialised against each other:

CPU0 (phylink resolve)          CPU1 (SIOCSHWTSTAMP)
ctrl = macb_readl(bp, NCR);
                                spin_lock_irqsave(&bp->lock);
                                macb_writel(bp, NCR, ... SRTSM/OSSMODE ...);
                                bp->tstamp_config = *tstamp_config;
                                spin_unlock_irqrestore(&bp->lock);
macb_writel(bp, NCR, ctrl | RE | TE);

Does that leave the hardware bit disagreeing with bp->tstamp_config, i.e.
the same "the bit does not follow the currently selected filter" symptom
the changelog describes, now for SRTSM as well as OSSMODE?  Would moving
the RE/TE/PTPUNI update in macb_mac_link_up() inside bp->lock be the right
fix, so all NCR read-modify-writes share one lock?

[Severity: Medium]
This isn't a bug introduced by this patch either, but what happens to these
bits across suspend/resume?  macb_resume() in macb_main.c blanks the
register:

	macb_writel(bp, NCR, MACB_BIT(MPE));
	macb_init_hw(bp);

which clears OSSMODE and SRTSM, and macb_init_hw() only programs NCFGR and
the DMA registers.  Grepping the driver, the only writer of GEM_TXBDCTRL
and GEM_RXBDCTRL is gem_set_hwtst():

	gem_writel(bp, TXBDCTRL, GEM_BF(TXTSMODE, tx_bd_control));
	gem_writel(bp, RXBDCTRL, GEM_BF(RXTSMODE, rx_bd_control));

and the re-invoked gem_ptp_init() only restores the timer/TSU and registers
the PTP clock.

bp->tstamp_config survives suspend, so gem_get_hwtst() keeps returning the
old tx_type and rx_filter:

	*tstamp_config = bp->tstamp_config;

while the descriptors no longer carry timestamps, so RX frames get no
hwtstamp attached and every timestamped transmit ends in:

	if (!GEM_BFEXT(DMA_TXVALID, desc->ctrl)) {
		dev_warn_ratelimited(&bp->pdev->dev, "Timestamp not set in TX BD as expected\n");
		return;
	}

with skb_tstamp_tx() never called and no error reported to userspace.  Since
SRTSM is now derived strictly from the cached filter, does this patch extend
that cached-versus-hardware divergence to SRTSM too, and should the resume
path re-apply bp->tstamp_config?

>  	enum macb_bd_control tx_bd_control = TSTAMP_DISABLED;
>  	enum macb_bd_control rx_bd_control = TSTAMP_DISABLED;
>  	int rx_filter = tstamp_config->rx_filter;
> @@ -429,7 +429,6 @@ int gem_set_hwtst(struct net_device *netdev,
>  	case HWTSTAMP_FILTER_PTP_V2_L4_DELAY_REQ:
>  		rx_bd_control = TSTAMP_ALL_PTP_FRAMES;
>  		rx_filter = HWTSTAMP_FILTER_PTP_V2_EVENT;
> -		ncr_mask |= MACB_BIT(SRTSM);
>  		ncr_bits |= MACB_BIT(SRTSM);
>  		break;

[Severity: Medium]
This is pre-existing and not introduced here, but are these PTPv2 filters
reachable through the advertised capabilities?  gem_get_ts_info() in
macb_main.c reports only:

	info->rx_filters =
		(1 << HWTSTAMP_FILTER_NONE) |
		(1 << HWTSTAMP_FILTER_ALL);

yet this switch accepts all nine HWTSTAMP_FILTER_PTP_V2_* values, programs
distinct hardware state for them (rx_bd_control = TSTAMP_ALL_PTP_FRAMES plus
NCR.SRTSM), normalises rx_filter to HWTSTAMP_FILTER_PTP_V2_EVENT, caches it
in bp->tstamp_config and returns it to userspace.  gem_get_hwtst() then
reports PTP_V2_EVENT as active, a filter ethtool -T says is unsupported.

Should gem_get_ts_info() advertise the PTPv2 bits, or should gem_set_hwtst()
reject them with -ERANGE?  This matters for the patch because the newly
unconditional SRTSM clear is only observable on transitions to and from
those unadvertised PTPv2 filters.

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