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

[Paolo Abeni <pabeni@redhat.com>]

On 10/30/26 13:53, netdev-bot+sashiko@kernel.org wrote:
> 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.

Note you can usually ignore pre-existing issues. Exceptions are
e.g. when the solution for the pre-existing issue will require
a significant rework of the patch at hand or impact is drammatic.

BTW I agree comments WRT problems still existing on siena are
unrelated and should be addressed separately.

> 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.
> 
>> diff --git a/drivers/net/ethernet/sfc/ptp.c b/drivers/net/ethernet/sfc/ptp.c
>> index 957ca08092e7f..e26269fae38d6 100644
>> --- a/drivers/net/ethernet/sfc/ptp.c
>> +++ b/drivers/net/ethernet/sfc/ptp.c
> 
> [ ... ]
> 
>> @@ -271,6 +272,8 @@ struct efx_ptp_rxfilter {
>>    * frequency adjustment into a fixed point fractional nanosecond format.
>>    * @current_adjfreq: Current ppb adjustment.
>>    * @phc_clock: Pointer to registered phc device (if primary function)
>> + * @phc_lock: Serializes PHC commands and synchronization, and protects
>> + *	      @current_adjfreq
> 
> [Severity: Low]
> Does this comment claim more coverage than the code gives? Two of the
> callbacks in the PHC ops table don't take phc_lock. efx_phc_gettime()
> (.gettime64) sends MC_CMD_PTP_OP_READ_NIC_TIME, and efx_phc_enable()
> (.enable) flips nic_ts_enabled. A standalone clock_gettime() on the PHC
> therefore runs without phc_lock.
> 
> Making the comment true by adding the lock to efx_phc_gettime() would
> self-deadlock, because efx_phc_settime() now calls it with phc_lock
> already held:
> 
> 	mutex_lock(&ptp_data->phc_lock);
> 	rc = efx_phc_gettime(ptp, &time_now);
> 
> Nothing breaks today, since the MCDI interface lock already serializes
> the RPCs. Could the comment instead list what phc_lock actually covers
> (synchronize, adjfine, adjtime and settime)?
You forgot to comment on this one. My reading is that this is just a
slightly misleading comment.

/P