Re: [PATCH net-next v4 8/8] net: dsa: microchip: Add two-step PTP support for KSZ8463

[Vladimir Oltean]

On Tue, Jan 27, 2026 at 10:06:50AM +0100, Bastien Curutchet (Schneider Electric) wrote:
> The KSZ8463 switch supports PTP but it's not supported by driver.
> 
> Add L2 two-step PTP support for the KSZ8463. IPv4 and IPv6 layers aren't
> supported. Neither is one-step PTP.
> 
> The pdelay_req and pdelay_resp timestamps share one interrupt bit status.
> So introduce last_tx_is_pdelayresp to keep track of the last sent event
> type. Use it to retrieve the relevant timestamp when the interrupt is
> caught.
> 
> Signed-off-by: Bastien Curutchet (Schneider Electric) <bastien.curutchet@xxxxxxxxxxx>
> ---
> @@ -518,6 +535,8 @@ void ksz_port_txtstamp(struct dsa_switch *ds, int port, struct sk_buff *skb)
>  	if (!hdr)
>  		return;
>  
> +	prt->last_tx_is_pdelayresp = false;
> +
>  	ptp_msg_type = ptp_get_msgtype(hdr, type);
>  
>  	switch (ptp_msg_type) {
> @@ -528,6 +547,7 @@ void ksz_port_txtstamp(struct dsa_switch *ds, int port, struct sk_buff *skb)
>  	case PTP_MSGTYPE_PDELAY_REQ:
>  		break;
>  	case PTP_MSGTYPE_PDELAY_RESP:
> +		prt->last_tx_is_pdelayresp = true;
>  		if (prt->tstamp_config.tx_type == HWTSTAMP_TX_ONESTEP_P2P) {
>  			KSZ_SKB_CB(skb)->ptp_type = type;
>  			KSZ_SKB_CB(skb)->update_correction = true;
> @@ -972,7 +992,17 @@ void ksz_ptp_clock_unregister(struct dsa_switch *ds)
>  
>  static int ksz_read_ts(struct ksz_port *port, u16 reg, u32 *ts)
>  {
> -	return ksz_read32(port->ksz_dev, reg, ts);
> +	u16 ts_reg = reg;
> +
> +	/**
> +	 * On KSZ8463 DREQ and DRESP timestamps share one interrupt line
> +	 * so we have to check the nature of the latest event sent to know
> +	 * where the timestamp is located
> +	 */
> +	if (ksz_is_ksz8463(port->ksz_dev) && port->last_tx_is_pdelayresp)
> +		ts_reg += KSZ8463_DRESP_TS_OFFSET;
> +
> +	return ksz_read32(port->ksz_dev, ts_reg, ts);
>  }

There is a race condition here.

ksz_port_txtstamp() is called "at line rate" - it doesn't wait for the
timestamping of the currently in progress skb to finish.

See the order in
static netdev_tx_t dsa_user_xmit(struct sk_buff *skb, struct net_device *dev)
{
	struct dsa_user_priv *p = netdev_priv(dev);
	struct sk_buff *nskb;

	dev_sw_netstats_tx_add(dev, 1, skb->len);

	memset(skb->cb, 0, sizeof(skb->cb));

	dsa_skb_tx_timestamp(p, skb); // calls ksz_port_txtstamp()

	...

	nskb = p->xmit(skb, dev); // calls ksz9893_xmit() -> ksz_defer_xmit()
	if (!nskb) {
		kfree_skb(skb); // deferred xmit enters this code path
		return NETDEV_TX_OK;
	}

	return dsa_enqueue_skb(nskb, dev);
}

The deferred worker gets scheduled much later, time during which a
second PTP packet may be transmitted from the stack.

If you let the second skb change the port->last_tx_is_pdelayresp which
ksz_ptp_msg_thread_fn() -> ksz_read_ts() thinks is associated with the
first skb, you're in big trouble.

You need to set port->last_tx_is_pdelayresp in the atomic section where
you know for sure that there's a single TX timestampable skb in flight.
There's no explicit lock which creates that atomic section, but the fact
that the worker kthread of the tagger processes work items one by one is
what gives you that guarantee.