Re: [PATCH v3 net 7/7] net: enetc: drain and cancel one-step TX tstamp queue when going down

[netdev-bot+sashiko]

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

Critical: 0 · High: 0 (1) · Medium: 4 · Low: 0

- [Medium] The drain/cancel/flag-reset is added to enetc_stop(), which is
  not only the "going down" path but also the generic "pause traffic"…
- [Medium] The new cancel_work_sync() + skb_queue_purge(&priv->tx_skbs) +
  clear_bit_unlock(ENETC_TX_ONESTEP_TSTAMP_IN_PROGRESS) sequence in…
- [Medium] The patch's justification footnote accepts that
  enetc_tx_onestep_tstamp() can call enetc_start_xmit() after…
- [Medium] The new unconditional
  clear_bit_unlock(ENETC_TX_ONESTEP_TSTAMP_IN_PROGRESS) in enetc_stop()…

Pre-existing issues:
- [High] enetc_tx_onestep_tstamp() (enetc.c:2996-2998) dequeues an skb
  from priv->tx_skbs and calls enetc_start_xmit() while discarding the…

--

net: enetc: drain and cancel one-step TX tstamp queue when going down

The patch cancels the priv->tx_onestep_tstamp work item, purges the
priv->tx_skbs queue and resets ENETC_TX_ONESTEP_TSTAMP_IN_PROGRESS in
enetc_stop(), after napi_disable(), so that no deferred one-step PTP Sync
packet survives ifdown or driver unbind.

> diff --git a/drivers/net/ethernet/freescale/enetc/enetc.c b/drivers/net/ethernet/freescale/enetc/enetc.c
> index 62cdcaab3f3fb..892490ff1ebee 100644
> --- a/drivers/net/ethernet/freescale/enetc/enetc.c
> +++ b/drivers/net/ethernet/freescale/enetc/enetc.c
> @@ -3110,6 +3110,10 @@ void enetc_stop(struct net_device *ndev)
>  		napi_disable(&priv->int_vector[i]->napi);
>  	}
>  
> +	cancel_work_sync(&priv->tx_onestep_tstamp);

[Severity: Medium]
The commit message says the late frame submitted by the work item "will be
put in the disabled TX BD ring, where nothing will happen with it until
enetc_free_rxtx_rings() later reclaims its memory".  Does that cover the
hardware producer index as well?

enetc_map_tx_buffs() ends in enetc_update_tx_ring_tail():

	/* includes wmb() */
	enetc_wr_reg_hot(tx_ring->tpir, tx_ring->next_to_use);

so a submission that lands after enetc_disable_tx_bdrs() advances
ENETC_TBPIR and tx_ring->next_to_use.  Neither enetc_stop(),
enetc_free_tx_ring() nor enetc_assign_tx_resource() rolls those back, and
enetc_setup_txbdr() adopts the register values instead of clearing them on
ENETC v1:

	/* For ENETC v1, clearing PI/CI registers for Tx not supported,
	 * adjust sw indexes
	 */
	tx_ring->next_to_use = enetc_txbdr_rd(hw, idx, ENETC_TBPIR);
	tx_ring->next_to_clean = enetc_txbdr_rd(hw, idx, ENETC_TBCIR) &
				 ENETC_TBCIR_IDX_MASK;

	if (tx_ring->next_to_use != tx_ring->next_to_clean &&
	    !is_enetc_rev1(si)) {

After enetc_reconfigure() (enetc_stop(); enetc_free_rxtx_rings(); new
resources; enetc_setup_bdrs(); enetc_start();) an ENETC v1 part would come
back up with TBPIR != TBCIR on a freshly allocated, zero-filled BD ring, and
the driver's next_to_clean would point at a scrubbed swbd whose skb and DMA
mapping were already released by enetc_free_tx_frame().  Should the stop or
teardown path reconcile TBPIR with next_to_use, or keep the one-step frame
out of the disabled ring in the first place?

> +	skb_queue_purge(&priv->tx_skbs);

[Severity: Medium]
enetc_stop() is also the generic "pause traffic" helper, not only the going
down path.  It has three callers: enetc_close(), enetc_reconfigure() and
enetc_set_coalesce().  In the last two the interface stays administratively
up, so is the unconditional purge here dropping one-step Sync packets for
which the driver already returned NETDEV_TX_OK in enetc_start_xmit()?

	if (enetc_cb->flag & ENETC_F_TX_ONESTEP_SYNC_TSTAMP) {
		if (test_and_set_bit_lock(ENETC_TX_ONESTEP_TSTAMP_IN_PROGRESS,
					  &priv->flags)) {
			skb_queue_tail(&priv->tx_skbs, skb);
			return NETDEV_TX_OK;
		}
	}

That would make ethtool -C, XDP program attach/detach and hwtstamp
reconfiguration silently discard accepted PTP Sync frames.

Related to the same call sites, the footnote's reclaim argument does not seem
to hold for enetc_set_coalesce(), which is just:

	if (netif_running(ndev) && changed) {
		/* reconfigure the operation mode of h/w interrupts,
		 * traffic needs to be paused in the process
		 */
		enetc_stop(ndev);
		enetc_start(ndev);
	}

There is no enetc_free_rxtx_rings() in between, so a descriptor left in the
ring by the flushed work item is not reclaimed but transmitted once the ring
is re-enabled.  Should the drain live in enetc_close() instead, or be
conditional on the interface actually going down?

[Severity: High]
This isn't a bug introduced by this patch, but it limits what the new purge
can reclaim.  enetc_tx_onestep_tstamp() discards the return value of
enetc_start_xmit():

	skb = skb_dequeue(&priv->tx_skbs);
	if (skb)
		enetc_start_xmit(skb, priv->ndev);

enetc_start_xmit() has NETDEV_TX_BUSY exits (the LSO and TSO paths, plus the
linear one):

		if (enetc_bd_unused(tx_ring) < ENETC_TXBDS_NEEDED(count)) {
			netif_stop_subqueue(ndev, tx_ring->index);
			return NETDEV_TX_BUSY;
		}

which neither transmit nor free the skb, because only a qdisc caller can
requeue it.  Does the work item leak that skb, given the dequeued reference
is the only one left?

The one-step branch has already run test_and_set_bit_lock() before those
returns, so does ENETC_TX_ONESTEP_TSTAMP_IN_PROGRESS also stay set with no
packet in flight?  The work item is only rescheduled from
enetc_clean_tx_ring() on a one-step TX completion, and that schedule_work()
happens inside the cleanup loop while tx_ring->next_to_clean is published
only after the loop, so the worker can observe a stale, nearly-full ring:

enetc_poll() -> enetc_clean_tx_ring()
	schedule_work(&priv->tx_onestep_tstamp);   /* inside the loop */
	...
	tx_ring->next_to_clean = i;                /* after the loop */

Such an abandoned skb is neither on priv->tx_skbs for the new
skb_queue_purge() nor in a TX ring for enetc_free_rxtx_rings(), so it would
survive ifdown and unbind too.

> +	clear_bit_unlock(ENETC_TX_ONESTEP_TSTAMP_IN_PROGRESS, &priv->flags);

[Severity: Medium]
The peer consumer touches exactly this state under the TX lock:

enetc_tx_onestep_tstamp()
	netif_tx_lock_bh(priv->ndev);
	clear_bit_unlock(ENETC_TX_ONESTEP_TSTAMP_IN_PROGRESS, &priv->flags);
	skb = skb_dequeue(&priv->tx_skbs);

Should the new drain and flag reset take netif_tx_lock_bh() as well?
netif_tx_stop_all_queues() only sets QUEUE_STATE_DRV_XOFF, it is not
netif_tx_disable() and takes no per-queue TX lock, so an ndo_start_xmit that
already passed the netif_xmit_frozen_or_stopped() check inside
HARD_TX_LOCK can still be running enetc_start_xmit() concurrently, and
enetc_start_xmit() has no ENETC_TX_DOWN test.

For the ndo_stop path this looks harmless, since __dev_close_many() calls
dev_deactivate_many(head, true) and waits for some_qdisc_is_busy() before
ndo_stop.  But enetc_set_coalesce() and enetc_reconfigure() call enetc_stop()
on a live interface with no qdisc deactivation, so:

CPU A                                  CPU B
enetc_start_xmit()                     enetc_stop()
                                       -> skb_queue_purge(&priv->tx_skbs)
                                       -> clear_bit_unlock(IN_PROGRESS)
-> test_and_set_bit_lock(IN_PROGRESS)
-> skb_queue_tail(&priv->tx_skbs, skb)

In the enetc_reconfigure() case the racing frame is released by
enetc_free_rxtx_rings() -> enetc_free_tx_frame(), which produces no TX
completion, so nothing calls schedule_work(&priv->tx_onestep_tstamp) again.
enetc_start() clears only ENETC_TX_DOWN and enetc_tx_onestep_tstamp_init() is
just INIT_WORK plus skb_queue_head_init:

	INIT_WORK(&priv->tx_onestep_tstamp, enetc_tx_onestep_tstamp);
	skb_queue_head_init(&priv->tx_skbs);

so can the IN_PROGRESS bit stay set for the rest of the up cycle, with every
subsequent one-step Sync appended to priv->tx_skbs and never sent?

[Severity: Medium]
This clear also releases the gate that enforces the MAC's "one single-step
frame in flight" rule, and the commit message accepts that cancel_work_sync()
may join the work item only after it pushed its frame into the disabled ring.

enetc_set_coalesce() then re-enables the same ring without reinitializing it,
via enetc_start() -> enetc_enable_txbdr():

	tbmr = enetc_txbdr_rd(hw, idx, ENETC_TBMR);
	tbmr |= ENETC_TBMR_EN;
	enetc_txbdr_wr(hw, idx, ENETC_TBMR, tbmr);

so the stale one-step descriptor is fetched and transmitted at a point where
the gate is already clear.  A new one-step Sync accepted right after
enetc_start() reprograms the single global single-step register:

	val |= ENETC_SET_SINGLE_STEP_OFFSET(offset);
	if (udp)
		val |= ENETC_PM0_SINGLE_STEP_CH;
	enetc_port_mac_wr(si, ENETC_PM0_SINGLE_STEP, val);

Can the correction field offset and the UDP checksum selector then be applied
to the wrong frame?  And when the stale frame finally completes, its
schedule_work() makes enetc_tx_onestep_tstamp() clear a gate that now belongs
to the newer frame, so does the one-in-flight invariant stay broken for
subsequent frames as well?

> +
>  	enetc_clear_interrupts(priv);
>  }
>  EXPORT_SYMBOL_GPL(enetc_stop);

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260915222735.1016937-1-vladimir.oltean%40nxp.com