Re: [PATCH net v3 2/3] sfc: ptp: serialize host and MC synchronization transactions

[netdev-bot+sashiko@kernel.org]

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

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

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 Siena driver keeps its own copy of the PTP code in
  drivers/net/ethernet/sfc/siena/ptp.c (built into sfc-siena by…
- [Low] The new kernel-doc for @phc_lock in struct efx_ptp_data
  (drivers/net/ethernet/sfc/ptp.c line 276) says the lock "Serializes…

Pre-existing issues:
- [Medium] At this commit, efx_ptp_pps_worker()
  (drivers/net/ethernet/sfc/ptp.c line 1511) copies the shared…

--

sfc: ptp: serialize host and MC synchronization transactions

This adds a phc_lock mutex to struct efx_ptp_data. The lock is held for
the whole efx_ptp_synchronize() transaction and for the PHC adjfine,
adjtime and settime operations. It also makes efx_ptp_synchronize()
bail out when efx_mcdi_rpc_start() fails, instead of finishing an RPC
that was never started.

> diff --git a/drivers/net/ethernet/sfc/ptp.c b/drivers/net/ethernet/sfc/ptp.c
> index 2247e5e3e25a8..eedab53e308af 100644
> --- a/drivers/net/ethernet/sfc/ptp.c
> +++ b/drivers/net/ethernet/sfc/ptp.c

[ ... ]

> @@ -272,6 +273,8 @@ struct efx_ptp_rxfilter {
>   * frequency adjustment into a fixed point fractional nanosecond format.
>   * @current_adjfreq: Current ppb adjustment.
>   * @phc_clock: Pointer to registered phc device (if primary function)
> + * @phc_lock: Serializes PHC commands and synchronization, and protects
> + *	      @current_adjfreq

[Severity: Low]
This isn't a bug, but does this comment claim more than the code does?
efx_phc_gettime(), the .gettime64 op, sends MC_CMD_PTP_OP_READ_NIC_TIME
without taking phc_lock. Only efx_phc_settime() holds the lock when it
calls efx_phc_gettime():

efx_phc_gettime() {
    ...
	rc = efx_mcdi_rpc(efx, MC_CMD_PTP, inbuf, sizeof(inbuf),
			  outbuf, sizeof(outbuf), NULL);
    ...
}

efx_phc_enable() also changes nic_ts_enabled without the lock. Only
adjfine, adjtime, settime and synchronize are serialized.

Could the comment name the operations the lock actually covers? Or
should gettime take the lock as well?

>   * @phc_clock_info: Registration structure for phc device
>   * @pps_work: pps work task for handling pps events
>   * @pps_workwq: pps work queue

[ ... ]

> @@ -1027,11 +1038,14 @@ static int efx_ptp_synchronize(struct efx_nic *efx, unsigned int num_readings)
>  	MCDI_SET_QWORD(synch_buf, PTP_IN_SYNCHRONIZE_START_ADDR,
>  		       ptp->start.dma_addr);
>  
> +	mutex_lock(&ptp->phc_lock);
> +
>  	/* Clear flag that signals MC ready */
>  	WRITE_ONCE(*start, 0);
>  	rc = efx_mcdi_rpc_start(efx, MC_CMD_PTP, synch_buf,
>  				MC_CMD_PTP_IN_SYNCHRONIZE_LEN);
> -	EFX_WARN_ON_ONCE_PARANOID(rc);
> +	if (rc)
> +		goto out;

[Severity: Medium]
Does the Siena driver need the same fix?

drivers/net/ethernet/sfc/siena/ptp.c is a separate copy that is built
into sfc-siena (siena/Makefile lists ptp.o). Its efx_ptp_synchronize()
still matches the version this patch replaces:

drivers/net/ethernet/sfc/siena/ptp.c:efx_ptp_synchronize() {
    ...
	WRITE_ONCE(*start, 0);
	rc = efx_siena_mcdi_rpc_start(efx, MC_CMD_PTP, synch_buf,
				      MC_CMD_PTP_IN_SYNCHRONIZE_LEN);
	EFX_WARN_ON_ONCE_PARANOID(rc);
    ...
}

No lock is held there. efx_ptp_pps_worker() and
efx_siena_ptp_change_mode() can still race. The second one is reached
from ptp_set_ts_config() in siena.c. Each can clear the other's
handshake flag and overwrite ptp->timeset[] and ptp->host_time_pps.

efx_siena_mcdi_rpc_start() can return -ENETDOWN early, before
efx_mcdi_acquire_sync(), when mc_bist_for_other_fn is set or the mode
is MCDI_MODE_FAIL. The Siena function still calls
efx_siena_mcdi_rpc_finish() in that case, so it waits on and releases
an MCDI interface it never acquired.

Siena's efx_phc_settime() still calls efx_phc_gettime() and then
efx_phc_adjtime() as two separate steps, with no lock held.

current_adjfreq is still written without a lock in efx_phc_adjfine()
and efx_ptp_start(), and read in efx_phc_adjtime(). Patch 1 of this
series removes the efx_ptp_start() reset only from sfc. Nothing under
sfc/siena/ has a phc_lock.

The later "sfc: ptp: avoid racing PPS timestamp updates" patch fixes a
PPS race that is also still present in Siena:

drivers/net/ethernet/sfc/siena/ptp.c:efx_ptp_pps_worker() {
    ...
	ptp_evt.pps_times = ptp->host_time_pps;
    ...
}

All of this code descends from 7c236c43b838, the commit named in the
Fixes: tag. Should there be a companion sfc/siena patch, or at least a
note in the commit message, so that stable backports cover Siena too?

>  
>  	/* Wait for start from MCDI (or timeout) */
>  	timeout = jiffies + msecs_to_jiffies(MAX_SYNCHRONISE_WAIT_MS);

[ ... ]

> @@ -1062,11 +1076,13 @@ static int efx_ptp_synchronize(struct efx_nic *efx, unsigned int num_readings)
>  			++ptp->no_time_syncs;
>  	}
>  
> +out:
>  	/* Increment the bad syncs counter if the synchronize fails, whatever
>  	 * the reason.
>  	 */
>  	if (rc != 0)
>  		++ptp->bad_syncs;
> +	mutex_unlock(&ptp->phc_lock);

[Severity: Medium]
This is a pre-existing issue and was not introduced by this patch, but
ptp->host_time_pps is still read after phc_lock has been dropped.
efx_ptp_pps_worker() copies it after efx_ptp_synchronize() returns:

efx_ptp_pps_worker() {
    ...
	ptp_evt.pps_times = ptp->host_time_pps;
    ...
}

At the same time,
efx_ptp_change_mode()->efx_ptp_synchronize()->efx_ptp_process_times()
can be rewriting the field in two steps under the lock:

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

Can the worker then pass a half-updated PPS timestamp, or the other
caller's timestamp, to ptp_clock_event()?

The next patch in the series, "sfc: ptp: avoid racing PPS timestamp
updates", appears to fix this for sfc. It returns the timestamp through
a caller-owned struct pps_event_time in efx_ptp_pps_worker() and passes
NULL from efx_ptp_change_mode().

>  
>  	return rc;
>  }

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