Re: [PATCH net v4 3/3] sfc: ptp: avoid racing PPS timestamp updates

[Austin, Alex (DCCG)]

On 30/10/2026 12:53, netdev-bot+sashiko@xxxxxxxxxx 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 · 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] This patch removes the shared ptp->host_time_pps race from
>   drivers/net/ethernet/sfc/ptp.c, but the same race is still present in…
>
> --
>
> sfc: ptp: avoid racing PPS timestamp updates
>
> This patch removes the shared host_time_pps field from struct
> efx_ptp_data in sfc/ptp.c. efx_ptp_synchronize() now returns the PPS
> timestamp through storage owned by the caller. The PPS worker therefore
> no longer copies a field that a concurrent SIOCSHWTSTAMP-driven
> synchronization could be rewriting.
>
>> diff --git a/drivers/net/ethernet/sfc/ptp.c b/drivers/net/ethernet/sfc/ptp.c
>> index e26269fae38d6..37511f234d76d 100644
>> --- a/drivers/net/ethernet/sfc/ptp.c
>> +++ b/drivers/net/ethernet/sfc/ptp.c
> [ ... ]
>
>> @@ -1500,14 +1504,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]
> This fixes the race in sfc/ptp.c. Is the same race still present in the
> Siena copy of this code, drivers/net/ethernet/sfc/siena/ptp.c? That file
> also descends from the Fixes target 7c236c43b838, and this patch doesn't
> touch it.
>
> In siena/ptp.c, efx_ptp_process_times() still writes the shared field:
>
> drivers/net/ethernet/sfc/siena/ptp.c:efx_ptp_process_times() {
>     ...
>         /* Set PPS timestamp to match NIC top of second */
>         ptp->host_time_pps = *last_time;
>         pps_sub_ts(&ptp->host_time_pps, delta);
>     ...
> }
>
> The Siena efx_ptp_pps_worker() copies it after synchronization returns,
> with no lock held:
>
> drivers/net/ethernet/sfc/siena/ptp.c:efx_ptp_pps_worker() {
>     ...
>         ptp_evt.type = PTP_CLOCK_PPSUSR;
>         ptp_evt.pps_times = ptp->host_time_pps;
>         ptp_clock_event(ptp->phc_clock, &ptp_evt);
> }
>
> Meanwhile, the SIOCSHWTSTAMP path can run another synchronization:
>
> efx_siena_ptp_change_mode()
>     efx_ptp_synchronize(efx, PTP_SYNC_ATTEMPTS * 2)
>         efx_ptp_process_times()
>             ptp->host_time_pps = *last_time;
>
> Can this give the partially updated PPS timestamp that the commit message
> describes, but for Siena NICs?
>
> Siena also looks more exposed than sfc was before this series. The Siena
> efx_ptp_synchronize() takes no phc_lock or any other mutex. The
> WRITE_ONCE(*start, 0) handshake runs before efx_siena_mcdi_rpc_start(),
> and the ptp->timeset[] processing runs after efx_siena_mcdi_rpc_finish().
> Could two concurrent synchronizations clobber each other's state there
> too?
>
> The earlier patch in this series, "sfc: ptp: serialize host and MC
> synchronization transactions", seems to have handled that only for sfc.
>
> Should the same caller-owned out-parameter change, along with the
> serialization, also be applied to siena/ptp.c? If not, could the commit
> message say why Siena is left out?
>
> [ ... ]


As before, I think we should tackle Siena issues in a different patchset.

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