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

[vladimir . oltean]

The driver uses a work item on the system workqueue
(priv->tx_onestep_tstamp) for deferred transmission of packets with
one-step TX timestamping requests. The reason is that the MAC supports
a single such packet in flight, but we cannot block the rate at which
user space enqueues them.

The problem is that the skb queue is never explicitly drained, and it
can hold packets even after the interface goes down or (worse) the
driver is unbound from the device. Especially the last point is
critical, because the work item will attempt to use freed data
structures of the netdev.

The priv->tx_onestep_tstamp work item (enetc_tx_onestep_tstamp)
processes one item from the priv->tx_skbs queue at a time, and gets
rescheduled on each one-step PTP packet TX completion.

If we cancelled the work item while NAPI was still enabled, there would
be no guarantee that NAPI would not reenable it. So the cancellation
needs to be after napi_disable().

Cancelling the work item waits for enetc_tx_onestep_tstamp() to finish
sending the current packet if already scheduled. The packet will be put
in the disabled TX BD ring, where nothing will happen with it until
enetc_free_rxtx_rings() later reclaims its memory (*).

However, priv->tx_skbs may contain more packets than just this one, and
because NAPI is disabled, enetc_clean_tx_ring() is unable to take care
of the rest. So we still have to clean up the remainder from the queue
and reset the ENETC_TX_ONESTEP_TSTAMP_IN_PROGRESS flag back for use.

On driver unbind, the problem should be solved by virtue of the fact
that unregister_netdev() calls netif_close_many() and that triggers this
same code path.

(*) Even if we add a check for ENETC_TX_DOWN in enetc_tx_onestep_tstamp(),
it is unavoidable that racing one-step PTP packets will be enqueued in a
disabled TX ring. This is because the work item runs asynchronously and
can miss that flag getting set. Think below:

CPU A                                CPU B
enetc_tx_onestep_tstamp()
-> test_bit(ENETC_TX_DOWN)
   // says not down
                                     enetc_stop()
                                     -> set_bit(ENETC_TX_DOWN)
                                     -> enetc_wait_bdrs()
                                        // waits for the BDs in the
                                        // ring to be transmitted,
                                        // but the PTP frame is
                                        // still queued in software
                                     -> enetc_disable_tx_bdrs()
-> enetc_start_xmit()

So I don't see any point in adding an ENETC_TX_DOWN test in the work
item.

Fixes: 7294380c5211 ("enetc: support PTP Sync packet one-step timestamping")
Reported-by: Wei Fang <wei.fang@xxxxxxx>
Signed-off-by: Vladimir Oltean <vladimir.oltean@xxxxxxx>
---
v2->v3: patch is new
---
 drivers/net/ethernet/freescale/enetc/enetc.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/net/ethernet/freescale/enetc/enetc.c b/drivers/net/ethernet/freescale/enetc/enetc.c
index 62cdcaab3f3f..892490ff1ebe 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);
+	skb_queue_purge(&priv->tx_skbs);
+	clear_bit_unlock(ENETC_TX_ONESTEP_TSTAMP_IN_PROGRESS, &priv->flags);
+
 	enetc_clear_interrupts(priv);
 }
 EXPORT_SYMBOL_GPL(enetc_stop);
-- 
2.43.0