Re: [PATCH net v4 2/3] sfc: ptp: serialize host and MC synchronization transactions
From: Paolo Abeni
Date: Mon Nov 02 2026 - 07:37:46 EST
On 10/30/26 13:53, netdev-bot+sashiko@xxxxxxxxxx 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 transactionsYou forgot to comment on this one. My reading is that this is just a
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)?
slightly misleading comment.
/P