Re: [PATCH net v3 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 2 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 2 · 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 leaves the forked Siena driver unfixed.
- [Medium] The commit message understates the bug it fixes and has an
  invalid Fixes: tag.

Pre-existing issues:
- [Low] This problem wasn't introduced by this patch, but the kernel-doc
  for struct efx_ptp_data describes `@current_adjfreq: Current ppb…

--

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

This patch removes the line that resets ptp->current_adjfreq to 0 in
efx_ptp_start() in drivers/net/ethernet/sfc/ptp.c. According to the commit
message, firmware keeps the clock frequency across MC_CMD_PTP_OP_DISABLE
and MC_CMD_PTP_OP_ENABLE. Zeroing the cached value therefore puts the driver
out of sync with the firmware.

> FW does not touch the clock frequency state on MC_CMD_PTP_OP_DISABLE or
> MC_CMD_PTP_OP_ENABLE, so the driver doesn't need to either. Doing so
> just desyncs the driver's cached current_adjfreq from what FW still has
> programmed.

[Severity: Medium]
Could the commit message describe the effect users see? efx_phc_adjtime()
sends the cached value to firmware on every time step:

drivers/net/ethernet/sfc/ptp.c:efx_phc_adjtime() {
    ...
	MCDI_SET_QWORD(inbuf, PTP_IN_ADJUST_FREQ, ptp_data->current_adjfreq);
    ...
}

So before this patch, the first clock step after any efx_ptp_start() set
the NIC frequency to 0. That covers a SIOCSHWTSTAMP mode change, ifup, and
the reset_required path in efx_ptp_worker().

The servo's frequency correction was lost. Meanwhile, ptp_clock_adjtime()
kept reporting the old dialed_frequency to userspace.

That is more than a cached value going out of sync. Stating it would help
backporters judge how serious the fix is.

> Fixes: ffe9554c17bf ("sfc: Add support for IEEE-1588 PTP")

Is this hash right? ffe9554c17bf does not resolve to any commit. The commit
with this subject appears to be 7c236c43b838, which the other patches in
this series also use:

Fixes: 7c236c43b838 ("sfc: Add support for IEEE-1588 PTP")

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

[Severity: Medium]
Does the Siena driver need the same change? Its copy of efx_ptp_start()
still has the same assignment:

drivers/net/ethernet/sfc/siena/ptp.c:efx_ptp_start() {
    ...
	ptp->evt_frag_idx = 0;
	ptp->current_adjfreq = 0;
    ...
}

Siena reaches efx_ptp_start() through the same paths:

- efx_siena_ptp_change_mode() on SIOCSHWTSTAMP
- efx_ptp_restart(), called from efx_siena_ptp_start_datapath() on ifup
  and reset recovery
- the reset_required branch of efx_ptp_worker()

Siena's efx_phc_adjtime() then sends the zeroed value to firmware as
PTP_IN_ADJUST_FREQ in MC_CMD_PTP_OP_ADJUST.

The commit message does not say whether the firmware behaviour on
MC_CMD_PTP_OP_DISABLE and MC_CMD_PTP_OP_ENABLE also applies to Siena
firmware.

If it does, can the same desync still happen in sfc-siena?

If it does not, is this change wrong for stable trees from before the
sfc/siena split? In those trees, drivers/net/ethernet/sfc/ptp.c also drives
Siena NICs. Also, 7c236c43b838 added efx_ptp_probe() to siena_probe_nic(),
so this code was first written for Siena firmware.

>  
>  	return 0;
>  

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

 * @current_adjfreq: Current ppb adjustment.

Is this unit correct? efx_phc_adjfine() stores adjustment_ns in this field,
not a ppb value:

drivers/net/ethernet/sfc/ptp.c:efx_phc_adjfine() {
    ...
	ptp_data->current_adjfreq = adjustment_ns;
    ...
}

adjustment_ns is the ppb delta after conversion, using adjfreq_ppb_shift,
into the NIC's fixed point fractional nanosecond format. The same comment
is in drivers/net/ethernet/sfc/siena/ptp.c.

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