Re: [PATCH net-next v10 3/3] net: stmmac: dwxgmac2: add XGMAC timestamp interrupt support

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 0 · Medium: 7 · Low: 3

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] The subject says "net: stmmac: dwxgmac2: add XGMAC timestamp
  interrupt support", but the patch also changes EXTTS and cross-…
- [Medium] The recorded EXTTS state (STMMAC_FLAG_EXT_SNAPSHOT_EN and
  plat->ext_snapshot_num) can survive resume after the hardware trigger…
- [Medium] stmmac_ptp_unregister() sets priv->ptp_clock to NULL and
  disarms the trigger under aux_ts_lock, "so a concurrent request cannot…
- [Medium] The patch adds a generic optional hook,
  stmmac_ops->timestamp_interrupt_cfg ("Arm or disarm the timestamp…
- [Medium] Who owns the auxiliary snapshot FIFO, the process-context
  cross-timestamp code (smtg_crosststamp, intel_crosststamp) or the…
- [Medium] net: stmmac: unlocked ptp_clock access in
  stmmac_ptp_register()
- [Medium] net: stmmac: dwxgmac2: concurrent FIFO pop in
  dwxgmac2_timestamp_interrupt()
- [Low] The commit message describes a user-visible bug.
- [Low] intel_crosststamp() takes aux_ts_lock at entry, returns -ENODEV
  if priv->ptp_clock is NULL, and holds the lock until it exits.
- [Low] stmmac_drop_extts_state() always clears the PTP_ACR/PTP_TCR
  trigger bits and logs netdev_err("Dropped the enabled auxiliary…

--

net: stmmac: dwxgmac2: add XGMAC timestamp interrupt support

This patch adds a DWXGMAC2 timestamp interrupt handler. The handler
reports auxiliary snapshots as PTP_CLOCK_EXTTS events. XGMAC_TSIE is armed
on demand through a new stmmac_ops->timestamp_interrupt_cfg() callback.

The patch also reworks EXTTS channel bookkeeping and the use of aux_ts_lock
in the Intel and socfpga cross-timestamp paths. It also changes how
suspend, resume and unregister handle the auxiliary snapshot trigger.

> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-intel.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-intel.c
> index 4308dccbf2570..3f0889c30a70c 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-intel.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-intel.c

[ ... ]

> @@ -424,11 +426,22 @@ static int intel_crosststamp(ktime_t *device,
>  	writel(gpio_value, ioaddr + GMAC_GPIO_STATUS);
>  
>  	/* Time sync done Indication - Interrupt method */
> -	if (!wait_event_interruptible_timeout(priv->tstamp_busy_wait,
> -					      stmmac_cross_ts_isr(priv),
> -					      HZ / 100)) {
> +	ret = wait_event_interruptible_timeout(priv->tstamp_busy_wait,
> +					       stmmac_cross_ts_isr(priv),
> +					       HZ / 100);
> +	if (ret <= 0) {
> +		/* Disarm the trigger on a timeout or an interrupted
> +		 * wait, but do not touch the hardware once the clock has
> +		 * been invalidated by an interface teardown.
> +		 */
> +		if (priv->ptp_clock) {

[Severity: Low]
Can priv->ptp_clock actually be NULL here? intel_crosststamp() now takes
aux_ts_lock on entry, returns -ENODEV if priv->ptp_clock is NULL, and holds
the lock until it returns.

The only runtime path that clears the pointer is stmmac_ptp_unregister(),
and it does that under the same lock:

	mutex_lock(&priv->aux_ts_lock);
	clock = priv->ptp_clock;
	priv->ptp_clock = NULL;

That means this check, and the one before the final PTP_ACR disarm, are
always true. Could these checks be dropped, along with the comment and the
"skips the disarm write afterwards" wording in the commit message?

> +			acr_value = readl(ptpaddr + PTP_ACR);
> +			acr_value &= ~PTP_ACR_MASK;
> +			writel(acr_value, ptpaddr + PTP_ACR);
> +		}

[ ... ]

> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_core.c b/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_core.c
> index 1a88cbaed70ca..ff2956bb89e62 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_core.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_core.c

[ ... ]

> @@ -1154,6 +1160,60 @@ static int dwxgmac2_get_mac_tx_timestamp(struct mac_device_info *hw, u64 *ts)
>  	return 0;
>  }
>  
> +void dwxgmac2_timestamp_interrupt(struct stmmac_priv *priv)
> +{

[ ... ]

> +	/* Runs on every MAC interrupt: only touch the FIFO when the
> +	 * timestamp interrupt was armed, and leave it to the
> +	 * cross-timestamp path while that owns it.
> +	 */
> +	if (priv->plat->flags & STMMAC_FLAG_INT_SNAPSHOT_EN) {
> +		readl(priv->ioaddr + XGMAC_TIMESTAMP_STATUS);
> +		return;
> +	}
> +
> +	if (!(readl(priv->ioaddr + XGMAC_INT_EN) & XGMAC_TSIE))
> +		return;

[Severity: Medium]
Can two instances of this handler drain the FIFO at the same time?
When priv->wol_irq is a separate line, stmmac_request_irq_single() and
stmmac_request_irq_multi_msi() install stmmac_interrupt() or
stmmac_mac_interrupt() on both dev->irq and wol_irq, and both end up
in stmmac_common_interrupt() and so in this function. That is why
stmmac_sync_timestamp_irq() has to sync both lines.

The handler deliberately doesn't gate on the read-to-clear TSIS bit,
and reading XGMAC_TIMESTAMP_STATUS doesn't change ATSNS. If the
timestamp interrupt on dev->irq and a PMT event on wol_irq are
handled on two CPUs at once, both see XGMAC_TSIE armed and read the
same non-zero pending_snapshots.

The drain loop only takes ptp_lock for reading, which doesn't exclude
another reader. get_ptptime() reads PTP_ATNR and then PTP_ATSR, and
the PTP_ATSR read pops the FIFO head. With two CPUs in the loop the
reads interleave across entries, so an event can pair the nanoseconds
of one snapshot with the seconds of another. The FIFO is also popped
2 * pending_snapshots times, and the extra pops read whatever an
empty FIFO returns, which is still reported to userspace as a
PTP_CLOCK_EXTTS event.

Should the count read and the pops be serialised with a spinlock, for
example a dedicated one or ptp_lock taken for writing? Re-reading
ATSNS before each pop would stop the loop at an empty FIFO, but it
wouldn't fix the torn ATNR/ATSR pairs on its own.

[Severity: Medium]
Is STMMAC_FLAG_INT_SNAPSHOT_EN being used as a lock here rather than as
state? smtg_crosststamp() now sets it on entry and clears it on every exit,
only to keep this hardirq handler away from the snapshot FIFO:

	priv->plat->flags |= STMMAC_FLAG_INT_SNAPSHOT_EN;
	/* Wait for a handler that is already past the flag check before
	 * taking over the snapshot FIFO.
	 */
	stmmac_sync_timestamp_irq(priv);
	...
	priv->plat->flags &= ~STMMAC_FLAG_INT_SNAPSHOT_EN;

The handler reads plat->flags with plain loads, while process context does
non-atomic |= and &= on the same word. The handler also uses the XGMAC_TSIE
bit in XGMAC_INT_EN as a second gate.

Correctness then depends on ordering spread across stmmac_enable(),
stmmac_rearm_timestamp_irq(), stmmac_suspend(), stmmac_ptp_unregister()
and core_init. Lockdep can't check any of that.

intel_crosststamp() also sets the same flag but skips the
stmmac_sync_timestamp_irq() drain. So the scheme isn't applied the same way
in both cross-timestamp paths.

Would an IRQ-safe spinlock work better? It would cover the FIFO pop here
and the ATSFC/ATSEN programming in the process-context paths.

> +
> +	/* Reading XGMAC_TIMESTAMP_STATUS clears TSIS, so use the ATSNS
> +	 * count to see the pending snapshots.
> +	 */

[ ... ]

> diff --git a/drivers/net/ethernet/stmicro/stmmac/hwif.c b/drivers/net/ethernet/stmicro/stmmac/hwif.c
> index 265671170bf6e..eba87410f985a 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/hwif.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/hwif.c
> @@ -258,7 +258,7 @@ static const struct stmmac_hwif_entry {
>  		.dma = &dwxgmac210_dma_ops,
>  		.mac = &dwxgmac210_ops,
>  		.vlan = &dwxgmac210_vlan_ops,
> -		.hwtimestamp = &stmmac_ptp,
> +		.hwtimestamp = &dwxgmac2_ptp,

[Severity: Low]
The commit message describes a bug that users can see. With .hwtimestamp =
&stmmac_ptp on DWXGMAC2/DWXLGMAC2, PTP_EXTTS_REQUEST succeeds but no
PTP_CLOCK_EXTTS event is ever delivered. Should this have a Fixes: tag?

The same question applies to the other fixes folded in here. One is the
channel exclusivity check in stmmac_enable(), which could never fire. The
other is the interrupted wait in intel_crosststamp(), which returned a
bogus cross-timestamp. Without Fixes: tags, stable backporters can't find
these fixes or tell how far back they apply.

[ ... ]

> diff --git a/drivers/net/ethernet/stmicro/stmmac/hwif.h b/drivers/net/ethernet/stmicro/stmmac/hwif.h
> index 8ae95e91a36ab..f077c1465a0c2 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/hwif.h
> +++ b/drivers/net/ethernet/stmicro/stmmac/hwif.h
> @@ -317,6 +317,8 @@ struct stmmac_ops {
>  	void (*update_caps)(struct stmmac_priv *priv);
>  	/* Change the interrupt enable setting. Enable takes precedence. */
>  	void (*irq_modify)(struct mac_device_info *hw, u32 disable, u32 enable);
> +	/* Arm or disarm the timestamp interrupt on demand (optional) */
> +	void (*timestamp_interrupt_cfg)(struct stmmac_priv *priv, bool en);

[Severity: Medium]
Only dwxgmac210_ops and dwxlgmac2_ops implement this hook. MAC100/GMAC
already arm the timestamp interrupt on demand, through a static helper in
dwmac1000_core.c with the same signature:

static void dwmac1000_timestamp_interrupt_cfg(struct stmmac_priv *priv, bool en)
{
	...
	u32 intr_mask = readl(ioaddr + GMAC_INT_MASK);
	...
	writel(intr_mask, ioaddr + GMAC_INT_MASK);
}

That helper isn't wired into dwmac1000_ops, though. Doesn't that make the
new generic disarm calls no-ops on MAC100/GMAC?

In stmmac_ptp_unregister(), stmmac_mac_timestamp_interrupt_cfg(priv,
false) does nothing on those cores. Yet the stmmac_drop_hw_extts() call
right after it has a special case for them. ATSEN0 and the flag get
cleared, but GMAC_INT_DISABLE_TIMESTAMP stays cleared, so the interrupt
stays unmasked.

dwmac1000_ptp_enable() also sets STMMAC_FLAG_EXT_SNAPSHOT_EN, so the new
block in stmmac_suspend() runs on GMAC. But nothing actually gets disarmed
there before the clocks are gated.

The locking also differs. The XGMAC hook goes through irq_modify under
hw->irq_ctrl_lock. dwmac1000_timestamp_interrupt_cfg() does an unlocked
read-modify-write of GMAC_INT_MASK, a register that dwmac1000_irq_modify()
updates under that lock.

[ ... ]

> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> index a2177777727ef..1abc7c696cf7e 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c

[ ... ]

> +static void stmmac_drop_extts_state(struct stmmac_priv *priv)
> +{
> +	mutex_lock(&priv->aux_ts_lock);
> +	stmmac_drop_hw_extts(priv);
> +	priv->plat->flags &= ~STMMAC_FLAG_EXT_SNAPSHOT_EN;
> +	priv->plat->ext_snapshot_num = -1;
> +	mutex_unlock(&priv->aux_ts_lock);
> +	netdev_err(priv->dev,
> +		   "Dropped the enabled auxiliary snapshot channel\n");
> +}

[Severity: Low]
Should this check STMMAC_FLAG_EXT_SNAPSHOT_EN or ext_snapshot_num first?
stmmac_resume() calls it whenever stmmac_init_ptp_clk_freq() or
stmmac_init_timestamping() fails. So the register write runs, and
netdev_err() says a channel was dropped, even when no EXTTS channel was
ever enabled.

stmmac_rearm_timestamp_irq() and stmmac_suspend() both check the flag
before doing anything.

> +
> +/**
> + * stmmac_rearm_timestamp_irq - re-arm the timestamp interrupt

[ ... ]

> +	/* Flush the FIFO, re-program the enabled auxiliary snapshot
> +	 * trigger and re-arm the interrupt only if the flush completed.
> +	 */
> +	if (priv->plat->core_type != DWMAC_CORE_XGMAC)
> +		return;

[Severity: Medium]
Can the recorded EXTTS state survive resume after the hardware trigger is
gone? The commit message says "so no stale channel survives". There seem
to be a few paths where it does.

On resume, stmmac_hw_setup()->stmmac_init_dma_engine()->stmmac_reset()
resets the core, and stmmac_init_timestamping() never restores PTP_ACR. On
GMAC, dwmac1000_core_init() also masks the timestamp interrupt again.

This early return affects GMAC4/dwmac5 (including Intel mGBE) and
MAC100/GMAC cores. On those cores the channel is neither restored nor
dropped. STMMAC_FLAG_EXT_SNAPSHOT_EN and ext_snapshot_num stay set, but
nothing is armed in hardware.

In stmmac_resume(), the stmmac_setup_est() failure branch skips both the
rearm and the drop:

		ret = stmmac_setup_est(priv);
		if (ret)
			goto error_stop_dma;

The two PTP failure branches just before it do call
stmmac_drop_extts_state(). The stmmac_hw_setup() failure path (goto
error_unlock) also leaves the state in place. On XGMAC, that leaves the
flag set while TSIE stays disarmed from stmmac_suspend().

The visible effects would be:

- The stmmac_ptp timestamp_interrupt() handler reads a zero channel from
  PTP_ACR and returns. The channel userspace thinks is enabled never
  delivers events.
- intel_crosststamp() and smtg_crosststamp() keep returning -EBUSY
  because of the stale flag.
- An enable for a different channel is accepted, because the PTP_ACR mask
  is empty. It then silently overwrites ext_snapshot_num.

Separately, dwmac1000_ptp_enable() sets and clears the flag but never
updates ext_snapshot_num.

[ ... ]

> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_ptp.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_ptp.c
> index a37405534bebf..4486c800fa71d 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_ptp.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_ptp.c
> @@ -201,35 +201,98 @@ static int stmmac_enable(struct ptp_clock_info *ptp,

[ ... ]

>  		acr_value = readl(ptpaddr + PTP_ACR);
> -		channel = ilog2(FIELD_GET(PTP_ACR_MASK, acr_value));
> -		acr_value &= ~PTP_ACR_MASK;
> +		channel = FIELD_GET(PTP_ACR_MASK, acr_value);
>  
>  		if (on) {
> -			if (FIELD_GET(PTP_ACR_MASK, acr_value)) {
> +			if (channel && rq->extts.index != ilog2(channel)) {
>  				netdev_err(priv->dev,
>  					   "Cannot enable auxiliary snapshot %d as auxiliary snapshot %d is already enabled",
> -					rq->extts.index, channel);
> +					rq->extts.index, ilog2(channel));
>  				mutex_unlock(&priv->aux_ts_lock);
>  				return -EBUSY;
>  			}
>  
> +			if (rq->extts.index >= PTP_ACR_ATSEN_NUM) {
> +				mutex_unlock(&priv->aux_ts_lock);
> +				return -EINVAL;
> +			}

[Severity: Medium]
The subject says "dwxgmac2: add XGMAC timestamp interrupt support". But
this hunk changes EXTTS behaviour that userspace can see on every core
using stmmac_ptp_clock_ops. Should the commit message call these changes
out, or should they go in separate fixes?

The old code cleared the mask before testing it, so this -EBUSY check
could never fire. A second enable silently replaced the first channel.
After this patch, dwmac4/dwmac5 users start getting -EBUSY.

Other behaviour changes folded in:

- Disabling a channel that isn't the armed one is now a silent no-op.
  Before, any disable cleared the whole ATSEN mask.
- extts.index >= 4 is rejected with -EINVAL. stmmac_ptp_register() also
  clamps n_ext_ts to 4 on every core, including MAC100/GMAC. Userspace
  sees this through PTP_CLOCK_GETCAPS.
- intel_crosststamp() used to check only for a zero return from
  wait_event_interruptible_timeout(). An interrupted wait fell through
  with -ERESTARTSYS, read the FIFO, and returned 0 with a bogus device/ART
  pair to PTP_SYS_OFFSET_PRECISE callers. It now returns an error.
- intel_crosststamp() no longer leaves its internal trigger armed in
  PTP_ACR.
- aux_ts_lock init/destroy moves from PTP register/unregister to
  probe/remove.
- stmmac_ptp_unregister() now writes PTP_ACR on every core, or PTP_TCR on
  MAC100/GMAC.

The commit message presents these as design choices for the XGMAC handler.
It doesn't describe them as fixes, or as behaviour changes on dwmac4/5,
Intel mGBE, socfpga and MAC100/GMAC.

[ ... ]

> @@ -363,12 +422,24 @@ void stmmac_ptp_register(struct stmmac_priv *priv)
>   */
>  void stmmac_ptp_unregister(struct stmmac_priv *priv)
>  {

[ ... ]

> +	/* Tear the hardware state down and invalidate the clock before
> +	 * releasing the lock, so a concurrent request cannot re-arm a
> +	 * clock that is going away.
> +	 */
> +	mutex_lock(&priv->aux_ts_lock);
> +	clock = priv->ptp_clock;
> +	priv->ptp_clock = NULL;
> +	stmmac_mac_timestamp_interrupt_cfg(priv, false);
> +	stmmac_drop_hw_extts(priv);
> +	priv->plat->flags &= ~STMMAC_FLAG_EXT_SNAPSHOT_EN;
> +	priv->plat->ext_snapshot_num = -1;
> +	mutex_unlock(&priv->aux_ts_lock);
> +
> +	ptp_clock_unregister(clock);

[Severity: Medium]
Does this guarantee hold on MAC100/GMAC? EXTTS requests on those cores go
through dwmac1000_ptp_enable(). That function takes aux_ts_lock but never
checks priv->ptp_clock:

	case PTP_CLK_REQ_EXTTS:
		mutex_lock(&priv->aux_ts_lock);
		tcr_val = readl(ptpaddr + PTP_TCR);

		if (on) {
			tcr_val |= GMAC_PTP_TCR_ATSEN0;
			tcr_val |= GMAC_PTP_TCR_ATSFC;
			priv->plat->flags |= STMMAC_FLAG_EXT_SNAPSHOT_EN;
		...
		mutex_unlock(&priv->aux_ts_lock);

		dwmac1000_timestamp_interrupt_cfg(priv, on);

Suppose a PTP_EXTTS_REQUEST ioctl is already waiting on aux_ts_lock:

ioctl                               stmmac_ptp_unregister()
dwmac1000_ptp_enable()
  mutex_lock() blocks
                                    mutex_lock()
                                    priv->ptp_clock = NULL
                                    clear ATSEN0 and flag
                                    mutex_unlock()
  set ATSEN0|ATSFC and flag
  writel(PTP_TCR)
  mutex_unlock()
  dwmac1000_timestamp_interrupt_cfg(priv, 1)
                                    ptp_clock_unregister()
                                      waits for the ioctl here

ptp_clock_unregister() only waits for in-flight ioctls after aux_ts_lock
has been dropped. ptp_disable_all_events() doesn't undo this either.

Wouldn't that leave ATSEN0 re-armed, the GMAC timestamp interrupt unmasked
and STMMAC_FLAG_EXT_SNAPSHOT_EN set after the clock is gone? Should
dwmac1000_ptp_enable() get the same !priv->ptp_clock check that
stmmac_enable() now has?

> +	pr_debug("Removed PTP HW clock successfully on %s\n", priv->dev->name);
>  }

[ ... ]

> @@ -201,35 +201,98 @@ static int stmmac_enable(struct ptp_clock_info *ptp,
>  		u8 channel;
>  
>  		mutex_lock(&priv->aux_ts_lock);
> +		/* Refuse to arm once the clock has been invalidated by
> +		 * unregister, or while an internal cross-timestamp owns the
> +		 * FIFO: the PTP_ACR programming below would disarm its
> +		 * trigger.
> +		 */
> +		if (!priv->ptp_clock) {
> +			mutex_unlock(&priv->aux_ts_lock);
> +			return -ENODEV;
> +		}
> +		if (priv->plat->flags & STMMAC_FLAG_INT_SNAPSHOT_EN) {
> +			mutex_unlock(&priv->aux_ts_lock);
> +			return -EBUSY;
> +		}
>  		acr_value = readl(ptpaddr + PTP_ACR);
> -		channel = ilog2(FIELD_GET(PTP_ACR_MASK, acr_value));
> -		acr_value &= ~PTP_ACR_MASK;
> +		channel = FIELD_GET(PTP_ACR_MASK, acr_value);
>  
>  		if (on) {
> -			if (FIELD_GET(PTP_ACR_MASK, acr_value)) {
> +			if (channel && rq->extts.index != ilog2(channel)) {
>  				netdev_err(priv->dev,
>  					   "Cannot enable auxiliary snapshot %d as auxiliary snapshot %d is already enabled",
> -					rq->extts.index, channel);
> +					rq->extts.index, ilog2(channel));
>  				mutex_unlock(&priv->aux_ts_lock);
>  				return -EBUSY;
>  			}
>  
> +			if (rq->extts.index >= PTP_ACR_ATSEN_NUM) {
> +				mutex_unlock(&priv->aux_ts_lock);
> +				return -EINVAL;
> +			}
> +
> +			/* A re-enable of the armed channel re-arms the
> +			 * interrupt as well; disarm it across the FIFO
> +			 * clear below and drain the handler that is
> +			 * already past its checks, so no one touches the
> +			 * FIFO while it is flushed.
> +			 */
> +			if (channel) {
> +				stmmac_mac_timestamp_interrupt_cfg(priv, false);
> +				stmmac_sync_timestamp_irq(priv);
> +			}
> +
>  			priv->plat->flags |= STMMAC_FLAG_EXT_SNAPSHOT_EN;
> +			priv->plat->ext_snapshot_num = rq->extts.index;
>  
>  			/* Enable External snapshot trigger */
>  			acr_value |= PTP_ACR_ATSEN(rq->extts.index);
>  			acr_value |= PTP_ACR_ATSFC;
> +		} else if (channel) {
> +			/* A disable request for a channel that is not the
> +			 * armed one is a no-op: the requested channel is
> +			 * not armed.
> +			 */
> +			if (rq->extts.index != ilog2(channel)) {
> +				mutex_unlock(&priv->aux_ts_lock);
> +				return 0;
> +			}
> +
> +			acr_value &= ~PTP_ACR_MASK;
> +			priv->plat->flags &= ~STMMAC_FLAG_EXT_SNAPSHOT_EN;
> +			priv->plat->ext_snapshot_num = -1;
>  		} else {
>  			priv->plat->flags &= ~STMMAC_FLAG_EXT_SNAPSHOT_EN;
> +			priv->plat->ext_snapshot_num = -1;
>  		}
>  		netdev_dbg(priv->dev, "Auxiliary Snapshot %d %s.\n",
>  			   rq->extts.index, on ? "enabled" : "disabled");
>  		writel(acr_value, ptpaddr + PTP_ACR);
> -		mutex_unlock(&priv->aux_ts_lock);
> -		/* wait for auxts fifo clear to finish */
> +		/* Keep the lock across the FIFO clear so the arm below
> +		 * cannot interleave with a suspend.
> +		 */
>  		ret = readl_poll_timeout(ptpaddr + PTP_ACR, acr_value,
>  					 !(acr_value & PTP_ACR_ATSFC),
>  					 10, 10000);
> +		if (ret && on) {
> +			/* Roll the failed enable back so no half-enabled
> +			 * channel is left.
> +			 */
> +			acr_value = readl(ptpaddr + PTP_ACR);
> +			acr_value &= ~PTP_ACR_ATSEN(rq->extts.index);
> +			writel(acr_value, ptpaddr + PTP_ACR);
> +			priv->plat->flags &= ~STMMAC_FLAG_EXT_SNAPSHOT_EN;
> +			priv->plat->ext_snapshot_num = -1;
> +		}
> +		/* Arm the timestamp interrupt only after the FIFO clear,
> +		 * so the handler cannot see a snapshot that is about to be
> +		 * discarded.
> +		 */
> +		stmmac_mac_timestamp_interrupt_cfg(priv, on && !ret);
> +		/* Drain an in-flight handler before the channel is freed. */
> +		if (!on)
> +			stmmac_sync_timestamp_irq(priv);
> +		mutex_unlock(&priv->aux_ts_lock);
>  		break;
>  	}
>  

[Severity: Medium]
Can these checks race with stmmac_ptp_register()? ptp_clock_register()
publishes /dev/ptpN through posix_clock_register() and
cdev_device_add() before it returns. stmmac_ptp_register() only stores
the result in priv->ptp_clock afterwards, and not under aux_ts_lock.

A consumer that opens the clock as soon as it appears, such as ptp4l
or ts2phc started from a udev rule, can issue PTP_EXTTS_REQUEST or
PTP_SYS_OFFSET_PRECISE inside that window. stmmac_enable(),
intel_crosststamp() and smtg_crosststamp() then see a NULL
priv->ptp_clock and fail with -ENODEV, even though the clock is being
registered rather than torn down. Before this patch those requests
worked in that window.

This is hit on every interface open, since stmmac_setup_ptp()
registers the clock again each time the interface comes up.

Would holding aux_ts_lock across ptp_clock_register() and assigning
priv->ptp_clock from a local variable before dropping it close this?

Separately, intel_crosststamp() and smtg_crosststamp() choose the
errno by reading priv->ptp_clock again after mutex_unlock(). A
concurrent stmmac_ptp_unregister() or stmmac_ptp_register() can change
it in between, so the errno need not match what was seen under the
lock, and the read itself is a data race. Could the errno be picked
before the lock is dropped, or the two checks split the way
stmmac_enable() does them?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261109134506.2794160-1-zxyan0222%40gmail.com