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

[Bastien Curutchet <bastien.curutchet@bootlin.com>]

Hi Vladimir,

On 2/2/26 2:43 PM, Vladimir Oltean wrote:
> 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@bootlin.com>
>> ---
>> @@ -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.

Thank you for the explanations. I suspected a race condition here but I 
didn't know how to mitigate it. I tested a new version on my side with 
port->last_tx_is_pdelayresp set in the ksz_port_deferred_xmit() worker 
and it works fine.

I'll wait for net-next to open back to send this new version.


Best regards,
Bastien