Re: [PATCH v6 net-next 9/9] net: dsa: netc: add PTP one-step timestamping support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] net: dsa: netc: premature onestep enable and state reset on link up
--

commit bfd63636ea3f54065627cb205cd9578a3ead9864
Author: Wei Fang <wei.fang@nxp.com>

net: dsa: netc: add PTP one-step timestamping support

This commit adds one-step PTP Sync offload support for NETC switch ports by
queuing one-step Sync frames per port and draining them from a workqueue item
that programs the shared PM_SINGLE_STEP register and inserts a To_Port
subtype 3 tag.

> diff --git a/drivers/net/dsa/netc/netc_main.c b/drivers/net/dsa/netc/netc_main.c
> index d69df3060b471..bef19d7d8f16c 100644
> --- a/drivers/net/dsa/netc/netc_main.c
> +++ b/drivers/net/dsa/netc/netc_main.c

[ ... ]

> @@ -1676,6 +1677,7 @@ static int netc_port_enable(struct dsa_switch *ds, int port,
>  		return err;
>  	}
>  
> +	netc_port_enable_onestep(np);
>  	np->enable = true;
>  
>  	return 0;

[ ... ]

> @@ -2561,6 +2564,7 @@ static void netc_mac_link_up(struct phylink_config *config,
>  	netc_port_set_rx_pause(np, rx_pause);
>  	netc_port_mac_tx_enable(np);
>  	netc_port_mac_rx_enable(np);
> +	netc_port_enable_onestep(np);
>  }

[ ... ]

> diff --git a/drivers/net/dsa/netc/netc_ptp.c b/drivers/net/dsa/netc/netc_ptp.c
> index e4a06737ed0b0..830fd9e4ca751 100644
> --- a/drivers/net/dsa/netc/netc_ptp.c
> +++ b/drivers/net/dsa/netc/netc_ptp.c

[ ... ]

> @@ -31,6 +43,16 @@ static void netc_port_tstamp_timeout_work(struct work_struct *work)
>  				    "Port %d ts_req_id %u which seems lost\n",
>  				    np->dp->index, NETC_SKB_CB(skb)->ts_req_id);
>  
> +		/* If the timed-out skb is the one holding the in-flight
> +		 * one-step frame's ts_req_id, resume the drain. Otherwise
> +		 * the frames queued on onestep_queue would never be sent or
> +		 * freed unless new traffic happened to arrive.
> +		 */
> +		if (NETC_SKB_CB(skb)->ptp_flag == NETC_PTP_FLAG_ONESTEP &&
> +		    NETC_SKB_CB(skb)->ts_req_id == np->onestep_ts_req_id &&
> +		    np->onestep_state == NETC_ONESTEP_IN_FLIGHT)
> +			resume_onestep = true;
> +
>  		__skb_unlink(skb, &np->tstamp_queue);
>  		__skb_queue_tail(&free_list, skb);
>  	}

[ ... ]

> +void netc_port_enable_onestep(struct netc_port *np)
> +{
> +	if (!dsa_port_is_user(np->dp))
> +		return;
> +
> +	spin_lock_bh(&np->onestep_lock);
> +	np->onestep_state = NETC_ONESTEP_IDLE;
> +	spin_unlock_bh(&np->onestep_lock);
> +}

[ ... ]

> @@ -68,12 +97,286 @@ static int netc_get_ts_req_id(struct netc_port *np)
>  void netc_port_ptp_init(struct netc_port *np)
>  {
>  	/* Initialize to invalid entry IDs */
>  	for (int i = 0; i < NETC_PTP_MAX; i++)
>  		np->ptp_ipft_eid[i] = NTMP_NULL_ENTRY_ID;
>  
> +	spin_lock_init(&np->onestep_lock);
> +	__skb_queue_head_init(&np->onestep_queue);
> +	INIT_WORK(&np->onestep_work, netc_port_onestep_work);
> +
>  	spin_lock_init(&np->tstamp_lock);

[Severity: Medium]
Can enabling one-step timestamping in both netc_port_enable() and
netc_mac_link_up() corrupt np->onestep_state across interface bringup?

In netc_port_ptp_init(), np->onestep_state is left at 0 (NETC_ONESTEP_IDLE)
rather than NETC_ONESTEP_PORT_INACTIVE. When a user port is opened via
dsa_user_open() -> dsa_port_enable_rt(), netc_port_enable() runs before
phylink_start(dp->pl) and calls netc_port_enable_onestep(np), setting
np->onestep_state = NETC_ONESTEP_IDLE while the physical link is still down
and the port MAC TX path is still disabled (POR_TXDIS is set and
PM_CMD_CFG_TX_EN is 0 until netc_mac_link_up() runs). Because
pl->old_link_state starts as false in phylink_start(), phylink_resolve() does
not call netc_mac_link_down() while the link stays down during autonegotiation
or when no cable is plugged in.

Since DSA user net_devices use IFF_NO_QUEUE, one-step Sync frames transmitted
between dsa_user_open() and netc_mac_link_up() pass the
NETC_ONESTEP_PORT_INACTIVE check in netc_port_onestep_sync_enqueue():

netc_port_onestep_sync_enqueue()
  -> schedule_work(&np->onestep_work)
netc_port_onestep_work()
  -> allocates ts_req_id and dummy_skb on np->tstamp_queue
  -> np->onestep_state = NETC_ONESTEP_IN_FLIGHT
  -> tagger_data->onestep_sync_xmit(skb, ndev)

The first Sync frame is sent to the disabled MAC and dropped without a
timestamp response while subsequent Sync frames accumulate in
np->onestep_queue.

When the link then comes up, netc_mac_link_up() calls
netc_port_enable_onestep(np) a second time, unconditionally overwriting
np->onestep_state with NETC_ONESTEP_IDLE even though a frame is already in
flight or scheduled. As a result:

1. When netc_port_tstamp_timeout_work() later reclaims the timed-out
   dummy_skb, np->onestep_state == NETC_ONESTEP_IN_FLIGHT is false, so
   resume_onestep is not set and any skbs waiting in np->onestep_queue are
   stranded until another Sync frame arrives or the port goes down.

2. Alternatively, if a frame is still in flight when netc_mac_link_up()
   resets np->onestep_state to NETC_ONESTEP_IDLE, the next Sync frame
   arriving at netc_port_onestep_sync_enqueue() sees NETC_ONESTEP_IDLE and
   immediately schedules netc_port_onestep_work(), reprogramming
   NETC_PM_SINGLE_STEP(0) before the earlier frame finishes.

Should netc_port_ptp_init() initialize np->onestep_state to
NETC_ONESTEP_PORT_INACTIVE, with netc_port_enable_onestep(np) called only from
netc_mac_link_up()?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261103021024.3121936-1-wei.fang@oss.nxp.com?part=9