Re: [PATCH v5 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] dsa: torn read of jiffies_64 on 32-bit architectures
--

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

net: dsa: netc: add PTP one-step timestamping support
    
This commit adds one-step Sync offload for NETC switch ports and advertises
HWTSTAMP_TX_ONESTEP_SYNC. It introduces a per-port queue that is drained
from a work handler to properly sequence one-step Sync frames.

> 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

[ ... ]

> @@ -116,7 +418,8 @@ static void netc_port_onestep_work(struct work_struct *work)

[ ... ]

> +	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;

[Severity: Medium]
Is it safe to read jiffies_64 directly here? On 32-bit architectures, a
direct read of this 64-bit variable performs two 32-bit loads without
synchronization. 

If a timer tick updates jiffies_64 between the two loads, the read value
can tear and jump forward or backward significantly. Should this use
get_jiffies_64() to safely capture the transmit time?

> +	NETC_SKB_CB(dummy_skb)->ptp_flag = NETC_PTP_FLAG_ONESTEP;
> +	np->onestep_tx_time = NETC_SKB_CB(dummy_skb)->ptp_tx_time;
> +	np->onestep_ts_req_id = ts_req_id;

[ ... ]

> +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))

[Severity: Medium]
Does the direct evaluation of jiffies_64 here risk a torn read on 32-bit 
systems? 

A torn read during this check could cause massive premature timeouts (which 
would incorrectly abort the in-flight state of one-step frames) or missed 
timeouts that leak request IDs. 

Could we use get_jiffies_64() instead, similar to how it is handled in 
netc_port_prepare_twostep()?

> +		return true;
> +
> +	return false;
> +}

[ ... ]

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