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

[Austin, Alex (DCCG)]

On 19/10/2026 12:58, 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 4 potential
> issue(s) to consider.
>
> Critical: 0 · High: 0 · Medium: 2 · Low: 2
>
> 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] Incomplete fix: the Siena sibling driver is left with the same
>   bugs.
> - [Medium] efx_ptp_start() sends MC_CMD_PTP_OP_ENABLE through
>   efx_ptp_enable() (line 1460) with no lock held.
> - [Low] Resolved later in the series (baseline decision: false).
> - [Low] The new kernel-doc for @phc_lock in struct efx_ptp_data (lines
>   276-277) says the lock 'Serializes PHC commands and synchronization,…
>
> --
>
> sfc: ptp: serialize host and MC synchronization transactions
>
> This patch adds a phc_lock mutex to struct efx_ptp_data. The lock is held
> across efx_ptp_synchronize(), the PHC adjfine/adjtime/settime callbacks,
> and the current_adjfreq reset in efx_ptp_start(). efx_ptp_synchronize()
> also now bails out when efx_mcdi_rpc_start() fails, instead of finishing
> an RPC that was never acquired.


[...]

>
>> @@ -1027,11 +1038,14 @@ static int efx_ptp_synchronize(struct efx_nic *efx, unsigned int num_readings)
>>       MCDI_SET_QWORD(synch_buf, PTP_IN_SYNCHRONIZE_START_ADDR,
>>                      ptp->start.dma_addr);
>>
>> +     mutex_lock(&ptp->phc_lock);
>> +
>>       /* Clear flag that signals MC ready */
>>       WRITE_ONCE(*start, 0);
>>       rc = efx_mcdi_rpc_start(efx, MC_CMD_PTP, synch_buf,
>>                               MC_CMD_PTP_IN_SYNCHRONIZE_LEN);
>> -     EFX_WARN_ON_ONCE_PARANOID(rc);
>> +     if (rc)
>> +             goto out;
> [Severity: Medium]
> Does drivers/net/ethernet/sfc/siena/ptp.c need the same fix? It has the
> same code, and every problem fixed here is still present there.
>
> In siena, efx_ptp_synchronize() clears the start flag without a lock. If
> the start fails, it still goes on to efx_siena_mcdi_rpc_finish():
>
> drivers/net/ethernet/sfc/siena/ptp.c:efx_ptp_synchronize() {
>     ...
>         /* Clear flag that signals MC ready */
>         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);
>     ...
> }
>
> efx_siena_mcdi_rpc_start() can return before efx_mcdi_acquire_sync()
> through efx_mcdi_check_supported(), mc_bist_for_other_fn or
> MCDI_MODE_FAIL.
>
> The siena driver also has the same two concurrent callers,
> efx_ptp_pps_worker() and efx_ptp_change_mode(). Its efx_phc_adjfine() and
> efx_ptp_start() write current_adjfreq without a lock. Its
> efx_phc_settime() still does gettime and adjtime as two separate steps.
>
> The Fixes: tag points at 7c236c43b838 ("sfc: Add support for IEEE-1588
> PTP"), the original code that siena/ptp.c was forked from. The commit
> message does not mention Siena. Should there be a companion patch for
> sfc-siena, or a note on why Siena is not affected?


 From inspection, it looks like this issue or similar locking issues do
exist 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. 

[...]

>> @@ -1446,7 +1462,9 @@ static int efx_ptp_start(struct efx_nic *efx)
>>               goto fail;
>>
>>       ptp->evt_frag_idx = 0;
>> +     mutex_lock(&ptp->phc_lock);
>>       ptp->current_adjfreq = 0;
>> +     mutex_unlock(&ptp->phc_lock);
> [Severity: Medium]
> Can a frequency adjustment still be lost here?
>
> Just above this, efx_ptp_start() sends MC_CMD_PTP_OP_ENABLE without
> holding any lock:
>
>         rc = efx_ptp_enable(efx);
>         if (rc != 0)
>                 goto fail;
>
> The firmware enable and the cache reset are therefore not one critical
> section, and efx_phc_adjfine() can run completely between them:
>
> CPU0 (efx_ptp_start)            CPU1 (clock_adjtime ADJ_FREQUENCY)
> efx_ptp_enable() completes
>                                 efx_phc_adjfine()
>                                   mutex_lock(&ptp_data->phc_lock)
>                                   MCDI ADJUST with FREQ=X
>                                   current_adjfreq = X
>                                   mutex_unlock(&ptp_data->phc_lock)
> mutex_lock(&ptp->phc_lock)
> current_adjfreq = 0
> mutex_unlock(&ptp->phc_lock)
>
> The next adjtime or settime goes through _efx_phc_adjtime(), which
> programs the frequency from the cached value:
>
>         MCDI_SET_QWORD(inbuf, PTP_IN_ADJUST_FREQ, ptp_data->current_adjfreq);
>
> That would put the MC back at frequency 0 and drop a correction that
> adjfine already reported as applied.
>
> The commit message says current_adjfreq "cannot be torn by the reset in
> efx_ptp_start()". The patch prevents tearing, but the cached value can
> still get out of sync with the hardware. Would phc_lock need to be held
> across both efx_ptp_enable() and the reset?
>
> Also, efx_ptp_enable() treats -EALREADY as success. In that case the
> firmware probably did not reset its frequency, but the driver still sets
> current_adjfreq to 0. Is that intended?


This looks like a real issue. I don't think we need to reset
current_adjfreq at all - will respin.

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


pw-bot: cr