Re: [PATCH net RESEND 1/2] net: macb: Preserve one-step mode on rejected timestamp requests

[Paolo Abeni <pabeni@redhat.com>]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
Full review at: https://netdev-ai.bots.linux.dev/sashiko/
---
net: macb: Preserve one-step mode on rejected timestamp requests

This moves the gem_ptp_set_one_step_sync() calls in gem_set_hwtst() out of
the tx_type switch and down past the rx_filter validation, so a request
rejected with -ERANGE no longer changes NCR.OSSMODE.  The cached
bp->tstamp_config and the TXBDCTRL/RXBDCTRL programming are unchanged.

> diff --git a/drivers/net/ethernet/cadence/macb_ptp.c b/drivers/net/ethernet/cadence/macb_ptp.c
> index e5195d7dac1d5..51659bbdfac9a 100644
> --- a/drivers/net/ethernet/cadence/macb_ptp.c
> +++ b/drivers/net/ethernet/cadence/macb_ptp.c
> @@ -418,11 +418,9 @@ int gem_set_hwtst(struct net_device *netdev,
>  	case HWTSTAMP_TX_OFF:
>  		break;
>  	case HWTSTAMP_TX_ONESTEP_SYNC:
> -		gem_ptp_set_one_step_sync(bp, 1);
>  		tx_bd_control = TSTAMP_ALL_FRAMES;
>  		break;
>  	case HWTSTAMP_TX_ON:
> -		gem_ptp_set_one_step_sync(bp, 0);
>  		tx_bd_control = TSTAMP_ALL_FRAMES;
>  		break;
>  	default:
> @@ -460,6 +458,11 @@ int gem_set_hwtst(struct net_device *netdev,
>  		return -ERANGE;
>  	}
>  
> +	if (tstamp_config->tx_type == HWTSTAMP_TX_ONESTEP_SYNC)
> +		gem_ptp_set_one_step_sync(bp, 1);
> +	else if (tstamp_config->tx_type == HWTSTAMP_TX_ON)
> +		gem_ptp_set_one_step_sync(bp, 0);
> +
>  	bp->tstamp_config = *tstamp_config;

[Severity: High]
Does this leave the same hardware/cache divergence reachable for the
HWTSTAMP_TX_OFF transition that the commit message sets out to remove?

Neither arm of the new conditional runs for HWTSTAMP_TX_OFF, but the next
statement caches the request unconditionally and the call after it programs
TXBDCTRL = TSTAMP_DISABLED:

	if (tstamp_config->tx_type == HWTSTAMP_TX_ONESTEP_SYNC)
		gem_ptp_set_one_step_sync(bp, 1);
	else if (tstamp_config->tx_type == HWTSTAMP_TX_ON)
		gem_ptp_set_one_step_sync(bp, 0);

	bp->tstamp_config = *tstamp_config;

	if (gem_ptp_set_ts_mode(bp, tx_bd_control, rx_bd_control) != 0)

So for the sequence

  SIOCSHWTSTAMP{tx_type = HWTSTAMP_TX_ONESTEP_SYNC}   -> OSSMODE = 1
  SIOCSHWTSTAMP{tx_type = HWTSTAMP_TX_OFF}            -> returns 0

gem_get_hwtst() then reports HWTSTAMP_TX_OFF via

	*tstamp_config = bp->tstamp_config;

while gem_ptp_set_one_step_sync() in macb_ptp.c is the only writer of
MACB_BIT(OSSMODE) in the driver, so One Step Synchro Mode stays enabled in
NCR.  Does the MAC then keep rewriting the originTimestamp of transmitted
PTP Sync frames?  The driver assumes it does elsewhere, since
ptp_one_step_sync() in macb_main.c is used to suppress software FCS/CRC
insertion for exactly those frames.

Would an unconditional call be preferable to the two-arm conditional, for
example:

	gem_ptp_set_one_step_sync(bp,
				  tstamp_config->tx_type == HWTSTAMP_TX_ONESTEP_SYNC);

[Severity: Medium]
This isn't a bug introduced by this patch, but while the call site is being
moved, is the read-modify-write inside gem_ptp_set_one_step_sync()
sufficiently serialized?

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

The SRTSM update in the rx_filter switch of gem_set_hwtst() does the same
thing:

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

Both run from process context with BHs enabled and without bp->lock, while
other NCR updaters take bp->lock, for example macb_tx_restart() from
NAPI/softirq context:

	spin_lock(&bp->lock);
	macb_writel(bp, NCR, macb_readl(bp, NCR) | MACB_BIT(TSTART));
	spin_unlock(&bp->lock);

and macb_tx_lpi_set(), which asserts it:

	lockdep_assert_held(&bp->lock);

	ncr = macb_readl(bp, NCR);

Can a concurrent locked writer's bit be lost if it lands between the
macb_readl() and macb_writel() here?  In the worst case the stale write-back
would drop MACB_BIT(RE) | MACB_BIT(TE) just restored by macb_mac_link_up(),
leaving RX/TX disabled while the stack considers the link up; more commonly
a TSTART or TXLPIEN update is lost.  The same unlocked pattern already
exists in macb_mac_link_up() after it releases bp->lock and in the RXUBR
errata path of macb_interrupt(), so this predates the patch, but would
wrapping both NCR read-modify-writes in gem_set_hwtst() with
spin_lock_irqsave(&bp->lock, ...) be the right direction?

[Severity: Low]
This is a pre-existing inconsistency rather than something this patch
introduces, but since gem_set_hwtst() is being touched: gem_get_ts_info()
in macb_main.c advertises only

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

while the rx_filter switch in gem_set_hwtst() accepts the nine
HWTSTAMP_FILTER_PTP_V2_* values and reports back a filter the ts_info
bitmap says is unsupported:

		rx_bd_control =  TSTAMP_ALL_PTP_FRAMES;
		tstamp_config->rx_filter = HWTSTAMP_FILTER_PTP_V2_EVENT;

Neither the legacy ioctl path via net_hwtstamp_validate() nor the ethtool
netlink tsconfig path checks the request against the driver's advertised
bitmap, so userspace can end up with an rx_filter value that is absent from
the supported set.  Should HWTSTAMP_FILTER_PTP_V2_EVENT be added to
info->rx_filters?

For reference, the related concern about HWTSTAMP_FILTER_PTP_V1_L4_SYNC and
HWTSTAMP_FILTER_PTP_V1_L4_DELAY_REQ returning success while leaving
rx_bd_control at TSTAMP_DISABLED is addressed later in this series by "net:
macb: Use all-frame timestamping for PTPv1 RX filters", which folds both
cases into the TSTAMP_ALL_FRAMES arm.
-- 
This is an AI-generated review.