Re: [PATCH net v2 2/2] sfc: ptp: avoid racing PPS timestamp updates

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 1 · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] The fix only covers drivers/net/ethernet/sfc/ptp.c.

--

sfc: ptp: avoid racing PPS timestamp updates

This commit removes the shared host_time_pps field from efx_ptp_data.
efx_ptp_process_times() now writes the PPS timestamp into storage that
the caller provides. efx_ptp_pps_worker() passes a stack-local
pps_event_time, and efx_ptp_change_mode() passes NULL.

> diff --git a/drivers/net/ethernet/sfc/ptp.c b/drivers/net/ethernet/sfc/ptp.c
> index c2f8b94592718..116b38079d8e6 100644
> --- a/drivers/net/ethernet/sfc/ptp.c
> +++ b/drivers/net/ethernet/sfc/ptp.c

[ ... ]

> @@ -1504,14 +1508,15 @@ static void efx_ptp_pps_worker(struct work_struct *work)
>  {
>  	struct efx_ptp_data *ptp =
>  		container_of(work, struct efx_ptp_data, pps_work);
> +	struct pps_event_time host_time_pps = {};
>  	struct efx_nic *efx = ptp->efx;
>  	struct ptp_clock_event ptp_evt;
>  
> -	if (efx_ptp_synchronize(efx, PTP_SYNC_ATTEMPTS))
> +	if (efx_ptp_synchronize(efx, PTP_SYNC_ATTEMPTS, &host_time_pps))
>  		return;
>  
>  	ptp_evt.type = PTP_CLOCK_PPSUSR;
> -	ptp_evt.pps_times = ptp->host_time_pps;
> +	ptp_evt.pps_times = host_time_pps;
>  	ptp_clock_event(ptp->phc_clock, &ptp_evt);
>  }

[Severity: Medium]
Should the same change also go into drivers/net/ethernet/sfc/siena/ptp.c?

That file was split off from the code that commit 7c236c43b838 added,
and it still has the shared field. In the Siena copy of
efx_ptp_process_times():

	ptp->host_time_pps = *last_time;
	pps_sub_ts(&ptp->host_time_pps, delta);

The Siena efx_ptp_pps_worker() then copies it after synchronize returns:

	if (efx_ptp_synchronize(efx, PTP_SYNC_ATTEMPTS))
		return;

	ptp_evt.type = PTP_CLOCK_PPSUSR;
	ptp_evt.pps_times = ptp->host_time_pps;

Meanwhile, the timestamping config path can still rewrite that field:

siena_ptp_set_ts_config()
  efx_siena_ptp_change_mode()
    efx_ptp_synchronize(efx, PTP_SYNC_ATTEMPTS * 2)
      efx_ptp_process_times()

This is the same partially updated PPS timestamp race that the commit
message describes for efx_ptp_pps_worker() and change_mode.

The Siena efx_ptp_synchronize() also takes no driver lock around the
transaction, while the sfc version holds phc_lock. Only the MCDI request
itself is serialized. Two concurrent synchronizations could therefore
overlap on these:

	WRITE_ONCE(*start, 0) on the shared DMA start flag
	the ptp->timeset[] writes in efx_ptp_process_times()
	the sync stats counters

Since the patch carries the same Fixes tag, stable kernels that have the
Siena split would get this fix only for sfc. Could Siena get the same
treatment, meaning a caller-owned pps_event_time, NULL from
efx_siena_ptp_change_mode(), and serialization of efx_ptp_synchronize()?
If Siena is left out on purpose, could the commit message say so?

> @@ -1847,7 +1852,8 @@ int efx_ptp_change_mode(struct efx_nic *efx, bool enable_wanted,
>  				rc = efx_ptp_start(efx);
>  			if (rc == 0) {
>  				rc = efx_ptp_synchronize(efx,
> -							 PTP_SYNC_ATTEMPTS * 2);
> +							 PTP_SYNC_ATTEMPTS * 2,
> +							 NULL);
>  				if (rc != 0)
>  					efx_ptp_stop(efx);
>  			}

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261015114104.2531624-1-alex.austin%40amd.com