Re: [PATCH net v4 1/3] sfc: ptp: drop the stale frequency reset in efx_ptp_start()

[netdev-bot+sashiko]

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

Critical: 0 · High: 0 · Medium: 1 (1) · Low: 0 (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 fix only changes the EF10/EF100 copy of efx_ptp_start() in
  drivers/net/ethernet/sfc/ptp.c.

Pre-existing issues:
- [Medium] At this commit, efx_phc_adjtime() reads
  ptp_data->current_adjfreq (drivers/net/ethernet/sfc/ptp.c:2129) and…
- [Low] The kernel-doc for struct efx_ptp_data says `@current_adjfreq:
  Current ppb adjustment.` (drivers/net/ethernet/sfc/ptp.c:272, and…

--

sfc: ptp: drop the stale frequency reset in efx_ptp_start()

This commit stops efx_ptp_start() from clearing the cached
current_adjfreq. Firmware keeps the clock frequency across PTP disable
and enable. Clearing the cache made the next efx_phc_adjtime() set the
NIC's frequency trim back to 0.

> diff --git a/drivers/net/ethernet/sfc/ptp.c b/drivers/net/ethernet/sfc/ptp.c
> index 00cfd8d5b46ea..957ca08092e7f 100644
> --- a/drivers/net/ethernet/sfc/ptp.c
> +++ b/drivers/net/ethernet/sfc/ptp.c
> @@ -1445,7 +1445,6 @@ static int efx_ptp_start(struct efx_nic *efx)
>  		goto fail;
>  
>  	ptp->evt_frag_idx = 0;
> -	ptp->current_adjfreq = 0;

[Severity: Medium]
Should the Siena copy of efx_ptp_start() get the same change? The Siena
version still clears the cache right after efx_ptp_enable() succeeds:

drivers/net/ethernet/sfc/siena/ptp.c:efx_ptp_start() {
    ...
	rc = efx_ptp_enable(efx);
	if (rc != 0)
		goto fail;

	ptp->evt_frag_idx = 0;
	ptp->current_adjfreq = 0;
    ...
}

Siena's efx_phc_adjtime() also sends the cached value with every time
step:

	MCDI_SET_QWORD(inbuf, PTP_IN_ADJUST_FREQ, ptp_data->current_adjfreq);

On Siena, all the paths named in the commit message still reach that
reset:

  efx_siena_ptp_change_mode() -> efx_ptp_start()    (SIOCSHWTSTAMP)
  efx_ptp_restart() -> efx_ptp_start()              (ifup, reset recovery)
  efx_ptp_worker() -> efx_ptp_start()               (reset_required)

So the next adjtime or settime would set the Siena NIC's frequency trim to
0. Meanwhile the servo and the PTP core still hold the old value.

The Fixes: tag points at 7c236c43b838, which added this code for Siena
hardware. The later patches in the series ("sfc: ptp: serialize host and
MC synchronization transactions" and "sfc: ptp: avoid racing PPS
timestamp updates") don't touch siena/ptp.c either.

This assumes Siena MC firmware also keeps the frequency across
MC_CMD_PTP_OP_DISABLE and MC_CMD_PTP_OP_ENABLE. The commit message only
says this for the firmware it targets. Does Siena firmware behave the
same way? If so, should drivers/net/ethernet/sfc/siena/ptp.c be fixed as
well?

[Severity: Low]
This isn't a bug introduced by this patch, but the kernel-doc for struct
efx_ptp_data, in both drivers/net/ethernet/sfc/ptp.c and
drivers/net/ethernet/sfc/siena/ptp.c, says:

 * @current_adjfreq: Current ppb adjustment.

The field doesn't actually hold ppb. efx_phc_adjfine() stores the
fixed-point fractional ns value that firmware takes:

	adjustment_ns = ((s64)delta * PPB_SCALE_WORD +
			 (1 << (ptp_data->adjfreq_ppb_shift - 1))) >>
			ptp_data->adjfreq_ppb_shift;
	...
	ptp_data->current_adjfreq = adjustment_ns;

Could the comment describe the unit correctly?

[Severity: Medium]
This is a pre-existing issue, but now that efx_phc_adjtime() relies only
on the cached value, can a concurrent adjtime and adjfine leave the cache
out of sync with firmware?

At this commit, nothing serializes these two accesses:

efx_phc_adjtime():
	MCDI_SET_QWORD(inbuf, PTP_IN_ADJUST_FREQ, ptp_data->current_adjfreq);

efx_phc_adjfine():
	rc = efx_mcdi_rpc(efx, MC_CMD_PTP, inadj, sizeof(inadj),
			  NULL, 0, NULL);
	...
	ptp_data->current_adjfreq = adjustment_ns;

posix_clock_adjtime() only takes the clock rwsem for read. So an adjtime
can read the old frequency F0 before an adjfine programs F1. Its RPC then
writes F0 back into firmware while the cache holds F1.

The next patch in the series, "sfc: ptp: serialize host and MC
synchronization transactions", appears to fix this. It adds
ptp_data->phc_lock, holds it around the RPC and the cache update in
efx_phc_adjfine(), and runs _efx_phc_adjtime() under the same lock.

>  
>  	return 0;
>  

To recap, these questions concern the Siena copy of efx_ptp_start(), the
@current_adjfreq kernel-doc, and the adjtime/adjfine race.

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