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

[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 more severe
> issue(s) in other patches of this series. This mail is only an FYI, in case
> you want to address the nit picks below when you respin.
>
> Low priority / nit picks for your consideration:
>
> - [Low] The new kernel-doc for `@phc_lock` in struct efx_ptp_data
>   (drivers/net/ethernet/sfc/ptp.c:275-276) says the lock "Serializes PHC…
>
> Pre-existing issues:
> - [High] This problem wasn't introduced by this patch, but
>   `_efx_phc_adjtime()` in drivers/net/ethernet/sfc/ptp.c (line 2144)…
> - [Medium] Baseline decision: not a defect of this patch (pre-existing,
>   Medium, in a driver the patch does not touch).
> - [Low] Baseline decision: false positive, fixed later in the series.
>
> --
>
> sfc: ptp: serialize host and MC synchronization transactions
>
> This adds a phc_lock mutex to struct efx_ptp_data. The mutex is held
> across the whole efx_ptp_synchronize() transaction and around the PHC
> adjfine/adjtime/settime operations, so the PPS worker and the hwtstamp
> enable path no longer clobber each other's handshake flag and timesets.
> It also bails out of efx_ptp_synchronize() when efx_mcdi_rpc_start()
> fails.
>

[...]

> [Severity: Medium]
> This is a pre-existing issue, and it is not in a file this patch
> touches. The Siena copy of this function still has the same pattern:
>
> 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);
>     ...
> }
>
> It takes no host lock around the handshake flag. When rpc_start fails
> it still goes on to efx_siena_mcdi_rpc_finish(), and it processes
> results into ptp->timeset[] after the MCDI interface has been released.
> Both racing callers exist there as well: efx_ptp_pps_worker() and
> efx_ptp_change_mode().
>
> The Siena efx_phc_settime() also still does an unserialized gettime
> followed by adjtime. current_adjfreq is written in efx_phc_adjfine() and
> read in efx_phc_adjtime() with no lock.
>
> Would it make sense to send a separate sfc/siena patch with the same
> fix?


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

[...]

> [Severity: High]
> This is a pre-existing issue, but should this be
> ptp_data->ns_to_nic_time() rather than going back through
> efx->ptp_data? The ptp_data argument comes from container_of() on the
> registered phc_clock_info, so it is always valid here.
>
> efx_realloc_channels() clears efx->ptp_data while it tears down the old
> channels, and the PHC stays registered for that whole time:
>
> drivers/net/ethernet/sfc/efx_channels.c:efx_realloc_channels() {
>     ...
> out:
>         efx->ptp_data = NULL;
>         /* Destroy unused channel structures */
>         for (i = 0; i < efx->n_channels; i++) {
>     ...
>         efx->ptp_data = ptp_data;
>     ...
> }
>
> A clock_settime() or clock_adjtime(ADJ_SETOFFSET) on the PHC that runs
> at the same time as ethtool -G could then do:
>
> efx_phc_settime() or efx_phc_adjtime()
>   mutex_lock(&ptp_data->phc_lock)
>   _efx_phc_adjtime()
>     efx->ptp_data->ns_to_nic_time()    <-- efx->ptp_data is NULL
>
> The PHC ioctl path does not take RTNL, and efx_realloc_channels() does
> not take phc_lock, so nothing seems to stop the two from overlapping.
>
> With this patch the oops now happens while phc_lock is held. Would that
> leave the mutex locked for good? Every later efx_ptp_synchronize() call
> would then block, both from the PPS worker and from
> efx_ptp_change_mode(), which runs under RTNL for SIOCSHWTSTAMP.
>
> Since this function is already being reworked here, using the ptp_data
> argument would avoid the NULL dereference.
>

Ed, are you happy if we tackle this in a follow up patch since it is a
pre-existing issue?

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