RE: [PATCH net v2] net: libwx: fix races in Tx timestamp handling

["Jiawen Wu" <jiawenwu@trustnetic.com>]

On Wed, Sep 16, 2026 7:51 AM, Jacob Keller wrote:
> On 9/14/2026 1:00 AM, Jiawen Wu wrote:
> > diff --git a/drivers/net/ethernet/wangxun/libwx/wx_lib.c b/drivers/net/ethernet/wangxun/libwx/wx_lib.c
> > index ed5aad7857bd..940ba2c6150f 100644
> > --- a/drivers/net/ethernet/wangxun/libwx/wx_lib.c
> > +++ b/drivers/net/ethernet/wangxun/libwx/wx_lib.c
> > @@ -1677,19 +1682,34 @@ static netdev_tx_t wx_xmit_frame_ring(struct sk_buff *skb,
> >  		wx->atr(tx_ring, first, ptype);
> >
> >  	if (wx_tx_map(tx_ring, first, hdr_len))
> > -		goto cleanup_tx_tstamp;
> > +		goto out_drop;
> >
> >  	return NETDEV_TX_OK;
> >  out_drop:
> > -	dev_kfree_skb_any(first->skb);
> > -	first->skb = NULL;
> > -cleanup_tx_tstamp:
> > +	/* The hardware will never report a timestamp for a frame it did not
> > +	 * transmit, so drop the request. Only do so if it is still ours: the
> > +	 * PTP worker may already have completed it and a concurrent transmit
> > +	 * may have submitted a new one.
> > +	 */
> 
> Could you explain this a bit more? The hardware doesn't report a
> timestamp if the frame didn't transmit.. but you say here the PTP worker
> may have already cleaned this up? How goes that work here? I guess
> clean_tx_tstamp label may execute even if the timestamp has actually
> happened and already completed? I'm not quite following this logic.

I think I was somewhat misled by the AI, the comment is wrong.

The label is not reachable for a frame what was actually transmitted.
It has only two entries, and both are before the frame is handed to the
hardware. No timestamp can be latched for such a frame, so the request
has to be cancelled. That part of the comment is accurate.

What is wrong is the reason I gave for the ownership check. The PTP
worker cannot have completed the request for a frame that was never
transmitted - it only consumes the slot when WX_TSC_1588_CTL_VALID is
set, and there is no new latch without a transmission. Bringing the
worker in there is simply incorrect.

I'll fix the comment like this:

/* The frame never reached the hardware, so no timestamp will ever be reported
 * for it and the request has to be cancelled. The slot is shared, though:
 * wx_ptp_clear_tx_timestamp() or wx_ptp_tx_hang() may have dropped our request
 * request already, and a transmit on another queue can have claimed the slot
 * since. Only cancel it while it is still ours, otherwise we would free
 * somebody else's skb and release their in-progress bit.
 */

> 
> >  	if (unlikely(tx_flags & WX_TX_FLAGS_TSTAMP)) {
> > -		dev_kfree_skb_any(wx->ptp_tx_skb);
> > -		wx->ptp_tx_skb = NULL;
> > -		wx->tx_hwtstamp_errors++;
> > -		clear_bit_unlock(WX_STATE_PTP_TX_IN_PROGRESS, wx->state);
> > +		struct sk_buff *ptp_tx_skb = NULL;
> > +		unsigned long flags;
> > +
> > +		spin_lock_irqsave(&wx->ptp_tx_lock, flags);
> > +		if (wx->ptp_tx_skb == skb) {
> > +			ptp_tx_skb = wx->ptp_tx_skb;
> > +			wx->ptp_tx_skb = NULL;
> > +			clear_bit(WX_STATE_PTP_TX_IN_PROGRESS, wx->state);
> > +		}
> > +		spin_unlock_irqrestore(&wx->ptp_tx_lock, flags);
> > +
> > +		if (ptp_tx_skb) {
> > +			dev_kfree_skb_any(ptp_tx_skb);
> > +			wx->tx_hwtstamp_errors++;
> > +		}
> >  	}
> > +	dev_kfree_skb_any(first->skb);
> > +	first->skb = NULL;
> >
> >  	return NETDEV_TX_OK;
> >  }
>