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

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] The commit message does not describe the valid-window recovery
  that the code implements.
- [Low] The new one-step code reads the global jiffies_64 directly in two
  places.
- [Low] In netc_port_onestep_sync_enqueue(), the queue-full check
  (skb_queue_len(&np->onestep_queue) >= NETC_ONESTEP_QTH) jumps to…
- [Low] netc_port_enable_onestep() sets np->onestep_state =
  NETC_ONESTEP_IDLE without checking the current state.

--

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

This adds one-step Sync offload for NETC switch ports and advertises
HWTSTAMP_TX_ONESTEP_SYNC. The per-port PM_SINGLE_STEP register is shared,
so the tagger hands each one-step Sync to the switch driver. The driver
queues them and sends them one at a time from a work item using a To_Port
subtype 3 tag. Lost responses are recovered through tstamp_timeout_work
and a 2^30 ns validity window.

> Since
> the tag timestamp is only 30 bits wide and the hardware accounts for a
> single wrap at most, also schedule the work when the PTP time has
> advanced beyond NETC_ONESTEP_VALID_WINDOW since the in-flight frame was
> queued, as its correction field can no longer be computed correctly.

[Severity: Low]
This paragraph doesn't seem to match what netc_onestep_timeout() does. I
see three differences.

First, the check never reads the PTP clock. It compares jiffies_64 with
np->onestep_tx_time + 1 + nsecs_to_jiffies64(NETC_ONESTEP_VALID_WINDOW).
The code comment says this is on purpose, because the PHC may be stepped
backwards.

Second, np->onestep_tx_time is set in netc_port_onestep_work() just before
the frame is programmed and passed to onestep_sync_xmit(). It is not set
when the frame is put on onestep_queue.

Third, netc_onestep_timeout() is only called from
netc_port_onestep_sync_enqueue(). That means this check only resumes the
drain when a new Sync is enqueued. If no new Sync arrives, queued frames
wait for the 5 second tstamp_timeout_work.

Could the commit message be reworded to describe this?

> diff --git a/drivers/net/dsa/netc/netc_ptp.c b/drivers/net/dsa/netc/netc_ptp.c
> index 756c69a02064e..c3875bce80bc1 100644
> --- a/drivers/net/dsa/netc/netc_ptp.c
> +++ b/drivers/net/dsa/netc/netc_ptp.c

[ ... ]

> @@ -68,12 +96,286 @@ static int netc_get_ts_req_id(struct netc_port *np)
>  	return ts_req_id;
>  }
>  

[ ... ]

> +void netc_port_disable_onestep(struct netc_port *np)
> +{
> +	struct sk_buff_head free_list;
> +
> +	if (!dsa_port_is_user(np->dp))
> +		return;
> +
> +	__skb_queue_head_init(&free_list);
> +
> +	spin_lock_bh(&np->onestep_lock);
> +	skb_queue_splice_init(&np->onestep_queue, &free_list);
> +	np->onestep_state = NETC_ONESTEP_PORT_INACTIVE;
> +	spin_unlock_bh(&np->onestep_lock);
> +
> +	cancel_work_sync(&np->onestep_work);
> +	netc_tx_dropped_add(np->dp->user, skb_queue_len(&free_list));
> +	__skb_queue_purge(&free_list);
> +}
> +
> +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);
> +}

[Severity: Low]
Is it safe to set NETC_ONESTEP_IDLE here without checking the current
state? netc_port_enable_onestep() is called from both netc_port_enable()
and netc_mac_link_up(). The single-outstanding-frame rule for
PM_SINGLE_STEP depends on NETC_ONESTEP_IN_FLIGHT, and this write
discards it.

Take a link flap:

netc_mac_link_down()
  netc_port_disable_onestep()
    onestep_state = NETC_ONESTEP_PORT_INACTIVE
    cancel_work_sync(&np->onestep_work)
  netc_port_mac_tx_graceful_stop()

cancel_work_sync() only waits for the worker. A subtype 3 frame already
passed to dsa_enqueue_skb()->dev_queue_xmit() may still be in the conduit
qdisc, the ENETC TX ring or the switch port egress queue. The graceful
stop only drains the MAC TX FIFO.

netc_mac_link_up()
  netc_port_enable_onestep()
    onestep_state = NETC_ONESTEP_IDLE

The next Sync then reprograms PM_SINGLE_STEP while the older frame may
still be pending. Could the older frame go out with the new frame's
offset and checksum-update settings? Could it also get a wrong
correctionField if it waited longer than 2^30 ns?

A Sync sent between .port_enable and the first mac_link_up looks mostly
blocked, because phylink keeps carrier off until mac_link_up.

[ ... ]

> +	NETC_SKB_CB(skb)->tstamp = tstamp;
> +	NETC_SKB_CB(skb)->ts_req_id = ts_req_id;
> +	NETC_SKB_CB(skb)->ptp_flag = NETC_PTP_FLAG_ONESTEP;
> +	NETC_SKB_CB(dummy_skb)->ts_req_id = ts_req_id;
> +	NETC_SKB_CB(dummy_skb)->ptp_tx_time = jiffies_64;
> +	NETC_SKB_CB(dummy_skb)->ptp_flag = NETC_PTP_FLAG_ONESTEP;
> +	np->onestep_tx_time = NETC_SKB_CB(dummy_skb)->ptp_tx_time;

[Severity: Low]
Should this be get_jiffies_64() instead of a direct read of jiffies_64?
include/linux/jiffies.h says:

  The 64-bit value is not atomic on 32-bit systems - you MUST NOT read it
  without sampling the sequence number in jiffies_lock.
  get_jiffies_64() will do this for you as appropriate.

netc_port_prepare_twostep() fills the same ptp_tx_time field, for entries
on the same tstamp_queue, with get_jiffies_64().

On 32-bit builds (possible through COMPILE_TEST), a torn read could make
netc_port_tstamp_timeout_work() reclaim this token's ts_req_id early or
very late. The time_after64(jiffies_64, expire_time) check in
netc_onestep_timeout() below has the same problem. It could report the
in-flight frame as expired at the wrong time.

[ ... ]

> +static bool netc_onestep_timeout(struct netc_port *np)
> +{
> +	u64 expire_time;
> +
> +	/* Use monotonic jiffies_64, as the PHC may be stepped backwards.
> +	 * Add one tick since the ns-to-jiffies conversion rounds down, so
> +	 * the software window is never shorter than the hardware window.
> +	 */
> +	expire_time = np->onestep_tx_time + 1 +
> +		      nsecs_to_jiffies64(NETC_ONESTEP_VALID_WINDOW);
> +	if (np->onestep_state == NETC_ONESTEP_IN_FLIGHT &&
> +	    time_after64(jiffies_64, expire_time))
> +		return true;
> +
> +	return false;
> +}
> +
> +void netc_port_onestep_sync_enqueue(struct dsa_switch *ds, int port,
> +				    struct sk_buff *skb)
> +{
> +	struct netc_port *np = NETC_PORT(ds, port);
> +
> +	spin_lock_bh(&np->onestep_lock);
> +	if (unlikely(np->onestep_state == NETC_ONESTEP_PORT_INACTIVE))
> +		goto free_skb;
> +
> +	if (unlikely(skb_queue_len(&np->onestep_queue) >= NETC_ONESTEP_QTH))
> +		goto free_skb;
> +
> +	__skb_queue_tail(&np->onestep_queue, skb);
> +	if (likely(np->onestep_state == NETC_ONESTEP_IDLE) ||
> +	    netc_onestep_timeout(np)) {

[Severity: Low]
Can the queue-full check keep the drain stalled after
NETC_ONESTEP_VALID_WINDOW has passed?

If the response for the in-flight frame is lost, onestep_state stays
NETC_ONESTEP_IN_FLIGHT. Suppose onestep_queue reaches 512 entries before
the window (about 1.07s) expires. From then on, every enqueue takes the
goto free_skb path and never calls netc_onestep_timeout().

The only recovery left is netc_port_tstamp_timeout_work() expiring the
token after NETC_TSTAMP_TIMEOUT (5 seconds). Every Sync that arrives
before then is dropped.

Would it be better to evaluate netc_onestep_timeout() before rejecting a
frame because the queue is full?

> +		np->onestep_state = NETC_ONESTEP_SCHEDULED;
> +		schedule_work(&np->onestep_work);
> +	}

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261012095924.1129312-1-wei.fang%40oss.nxp.com