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

["Austin, Alex (DCCG)" <alex.austin@amd.com>]

On 30/10/2026 12:53, netdev-bot+sashiko@kernel.org wrote:
> Caution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding.
>
>
> 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?
>
From inspection, it looks like the same issue exists on Siena. I think we
should take care of Siena issues in a separate patchset. Additionally, I
don't have a Siena NIC handy to test on.

[...]
> [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.


Yes, this is fixed in the next patch.

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