[PATCH net v2 0/2] sfc: fix PTP synchronization races

[PATCH net v2 0/2] sfc: fix PTP synchronization races

[Alex Austin]

This series fixes PTP synchronization races found by code inspection.

efx_ptp_pps_worker() and the SIOCSHWTSTAMP path through
efx_ptp_change_mode() both call efx_ptp_synchronize(). Concurrent calls
can interfere with the shared DMA handshake and the shared timeset
results, because the MCDI interface is released inside
efx_mcdi_rpc_finish() and acquired after the handshake flag is cleared.
Patch 1 serializes a function's synchronization transactions and clock
operations under a new mutex. Patch 2 returns the PPS timestamp in
caller-owned storage to prevent concurrent synchronization from
overwriting it while the PPS worker reads it.

Tested on two hosts with X2522-25G NICs cabled back to back at 25G.
Both PCI functions share one clock.

Clock operations on the function that registers the clock were run
against repeated efx_ptp_change_mode() -> efx_ptp_synchronize() on the
other function:

  hwstamp_ctl -i ens41f0np0 -r 1 -t 1
  hwstamp_ctl -i ens41f1np1 -r 1 -t 1

  for i in $(seq 150); do
          phc_ctl /dev/ptp1 freq 1000
          phc_ctl /dev/ptp1 adj 0.0001
          phc_ctl /dev/ptp1 get
  done &
  for i in $(seq 40); do
          hwstamp_ctl -i ens41f1np1 -r 0 -t 0
          hwstamp_ctl -i ens41f1np1 -r 1 -t 1
  done &
  wait

  ethtool -S ens41f1np1 | grep ptp_

                                without    with
  ptp_good_syncs                     400     400
  ptp_bad_syncs                        0       0
  ptp_sync_timeouts                    0       0
  ptp_no_time_syncs                    0       0
  ptp_invalid_sync_windows             0       0
  ptp_undersize_sync_windows           0       8
  ptp_oversize_sync_windows            0       0

The undersize counter rejects readings as implausibly fast rather than
inaccurate. FW reports a 40ns minimum sync window; without the series
the fastest readings sit exactly on that limit, and with it they sit a
few nanoseconds below. The shift tracks the barrier in the mutex
acquisition rather than the serialization it provides: an smp_mb()
alone where the mutex is taken reproduces the counter on an otherwise
unmodified driver, while a plain memory access there does not. No
synchronization failed: each takes eight readings and needs one to
succeed.

End-to-end PTP sync quality is unchanged (tested with ptp4l and
phc2sys).

Changes since v1:
 - document in efx_ptp_synchronize() why a secondary function needs no
   cross-function locking.
 - take phc_lock in efx_ptp_start() around the current_adjfreq reset
 - do not take phc_lock in efx_phc_gettime(): it reads no shared state
   and MCDI already serializes the command, so _efx_phc_gettime() is
   gone and efx_phc_settime() calls efx_phc_gettime() under the lock
 - keep the comment above efx_phc_settime() (Edward Cree)
 - add the test results above
 - return the PPS timestamp through the caller's own pps_event_time
   rather than adding a struct.

Alex Austin (2):
  sfc: ptp: serialize host and MC synchronization transactions
  sfc: ptp: avoid racing PPS timestamp updates

 drivers/net/ethernet/sfc/ptp.c | 109 ++++++++++++++++++++++-----------
 1 file changed, 73 insertions(+), 36 deletions(-)

-- 
2.34.1

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

[Alex Austin]

efx_ptp_synchronize() clears the MC-ready DMA flag before
efx_mcdi_rpc_start() acquires the MCDI interface, and processes results
into ptp->timeset[] after efx_mcdi_rpc_finish() has released it. The PPS
worker and the hwtstamp enable path can therefore clobber each other's
handshake flag and timesets.

Hold a PHC mutex across the whole transaction and over the clock
operations, so efx_phc_settime() reads and adjusts atomically and
current_adjfreq cannot be torn by the reset in efx_ptp_start(). Also
bail out when efx_mcdi_rpc_start() fails rather than finishing an RPC
that was never acquired.

Fixes: 7c236c43b838 ("sfc: Add support for IEEE-1588 PTP")
Signed-off-by: Alex Austin <alex.austin@amd.com>
Reviewed-by: Alejandro Lucero <alucerop@amd.com>
Reviewed-by: Pieter Jansen van Vuuren <pieter.jansen-van-vuuren@amd.com>
Acked-by: Edward Cree <ecree.xilinx@gmail.com>
---
 drivers/net/ethernet/sfc/ptp.c | 83 +++++++++++++++++++++++-----------
 1 file changed, 57 insertions(+), 26 deletions(-)

diff --git a/drivers/net/ethernet/sfc/ptp.c b/drivers/net/ethernet/sfc/ptp.c
index 6ca9a75..c2f8b94 100644
--- a/drivers/net/ethernet/sfc/ptp.c
+++ b/drivers/net/ethernet/sfc/ptp.c
@@ -36,6 +36,7 @@
 #include <linux/errno.h>
 #include <linux/ktime.h>
 #include <linux/module.h>
+#include <linux/mutex.h>
 #include <linux/pps_kernel.h>
 #include <linux/ptp_clock_kernel.h>
 #include "net_driver.h"
@@ -272,6 +273,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
  * @phc_clock_info: Registration structure for phc device
  * @pps_work: pps work task for handling pps events
  * @pps_workwq: pps work queue
@@ -331,6 +334,7 @@ struct efx_ptp_data {
 	unsigned int adjfreq_ppb_shift;
 	s64 current_adjfreq;
 	struct ptp_clock *phc_clock;
+	struct mutex phc_lock;
 	struct ptp_clock_info phc_clock_info;
 	struct work_struct pps_work;
 	struct workqueue_struct *pps_workwq;
@@ -1008,7 +1012,14 @@ efx_ptp_process_times(struct efx_nic *efx, MCDI_DECLARE_STRUCT_PTR(synch_buf),
 	return 0;
 }
 
-/* Synchronize times between the host and the MC */
+/* Synchronize times between the host and the MC.
+ *
+ * The MC serializes PTP requests, so MC_CMD_PTP_OP_ADJUST cannot be applied
+ * while MC_CMD_PTP_OP_SYNCHRONIZE is collecting timesets. This holds across
+ * functions of a controller, which share the clock but adjust it only through
+ * the function that registered it, so a secondary function can capture here
+ * without serializing against the primary.
+ */
 static int efx_ptp_synchronize(struct efx_nic *efx, unsigned int num_readings)
 {
 	struct efx_ptp_data *ptp = efx->ptp_data;
@@ -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;
 
 	/* Wait for start from MCDI (or timeout) */
 	timeout = jiffies + msecs_to_jiffies(MAX_SYNCHRONISE_WAIT_MS);
@@ -1062,11 +1076,13 @@ static int efx_ptp_synchronize(struct efx_nic *efx, unsigned int num_readings)
 			++ptp->no_time_syncs;
 	}
 
+out:
 	/* Increment the bad syncs counter if the synchronize fails, whatever
 	 * the reason.
 	 */
 	if (rc != 0)
 		++ptp->bad_syncs;
+	mutex_unlock(&ptp->phc_lock);
 
 	return rc;
 }
@@ -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);
 
 	return 0;
 
@@ -1570,6 +1588,7 @@ int efx_ptp_probe(struct efx_nic *efx, struct efx_channel *channel)
 	if (!efx->ptp_data)
 		return -ENOMEM;
 
+	mutex_init(&ptp->phc_lock);
 	ptp->efx = efx;
 	ptp->channel = channel;
 
@@ -1641,6 +1660,7 @@ fail2:
 	efx_nic_free_buffer(efx, &ptp->start);
 
 fail1:
+	mutex_destroy(&ptp->phc_lock);
 	kfree(efx->ptp_data);
 	efx->ptp_data = NULL;
 
@@ -1695,6 +1715,7 @@ void efx_ptp_remove(struct efx_nic *efx)
 	destroy_workqueue(efx->ptp_data->workwq);
 
 	efx_nic_free_buffer(efx, &efx->ptp_data->start);
+	mutex_destroy(&efx->ptp_data->phc_lock);
 	kfree(efx->ptp_data);
 	efx->ptp_data = NULL;
 }
@@ -2106,23 +2127,23 @@ static int efx_phc_adjfine(struct ptp_clock_info *ptp, long scaled_ppm)
 	MCDI_SET_QWORD(inadj, PTP_IN_ADJUST_FREQ, adjustment_ns);
 	MCDI_SET_DWORD(inadj, PTP_IN_ADJUST_SECONDS, 0);
 	MCDI_SET_DWORD(inadj, PTP_IN_ADJUST_NANOSECONDS, 0);
+	mutex_lock(&ptp_data->phc_lock);
 	rc = efx_mcdi_rpc(efx, MC_CMD_PTP, inadj, sizeof(inadj),
 			  NULL, 0, NULL);
-	if (rc != 0)
-		return rc;
-
-	ptp_data->current_adjfreq = adjustment_ns;
-	return 0;
+	if (!rc)
+		ptp_data->current_adjfreq = adjustment_ns;
+	mutex_unlock(&ptp_data->phc_lock);
+	return rc;
 }
 
-static int efx_phc_adjtime(struct ptp_clock_info *ptp, s64 delta)
+static int _efx_phc_adjtime(struct efx_ptp_data *ptp_data, s64 delta)
+	__must_hold(&ptp_data->phc_lock)
 {
-	u32 nic_major, nic_minor;
-	struct efx_ptp_data *ptp_data = container_of(ptp,
-						     struct efx_ptp_data,
-						     phc_clock_info);
-	struct efx_nic *efx = ptp_data->efx;
 	MCDI_DECLARE_BUF(inbuf, MC_CMD_PTP_IN_ADJUST_LEN);
+	struct efx_nic *efx = ptp_data->efx;
+	u32 nic_major, nic_minor;
+
+	lockdep_assert_held(&ptp_data->phc_lock);
 
 	efx->ptp_data->ns_to_nic_time(delta, &nic_major, &nic_minor);
 
@@ -2135,6 +2156,18 @@ static int efx_phc_adjtime(struct ptp_clock_info *ptp, s64 delta)
 			    NULL, 0, NULL);
 }
 
+static int efx_phc_adjtime(struct ptp_clock_info *ptp, s64 delta)
+{
+	struct efx_ptp_data *ptp_data = container_of(ptp, struct efx_ptp_data,
+						  phc_clock_info);
+	int rc;
+
+	mutex_lock(&ptp_data->phc_lock);
+	rc = _efx_phc_adjtime(ptp_data, delta);
+	mutex_unlock(&ptp_data->phc_lock);
+	return rc;
+}
+
 static int efx_phc_gettime(struct ptp_clock_info *ptp, struct timespec64 *ts)
 {
 	struct efx_ptp_data *ptp_data = container_of(ptp,
@@ -2166,23 +2199,21 @@ static int efx_phc_settime(struct ptp_clock_info *ptp,
 {
 	/* Get the current NIC time, efx_phc_gettime.
 	 * Subtract from the desired time to get the offset
-	 * call efx_phc_adjtime with the offset
+	 * call _efx_phc_adjtime with the offset
 	 */
+	struct efx_ptp_data *ptp_data = container_of(ptp, struct efx_ptp_data,
+						  phc_clock_info);
+	struct timespec64 time_now, delta;
 	int rc;
-	struct timespec64 time_now;
-	struct timespec64 delta;
 
+	mutex_lock(&ptp_data->phc_lock);
 	rc = efx_phc_gettime(ptp, &time_now);
-	if (rc != 0)
-		return rc;
-
-	delta = timespec64_sub(*e_ts, time_now);
-
-	rc = efx_phc_adjtime(ptp, timespec64_to_ns(&delta));
-	if (rc != 0)
-		return rc;
-
-	return 0;
+	if (!rc) {
+		delta = timespec64_sub(*e_ts, time_now);
+		rc = _efx_phc_adjtime(ptp_data, timespec64_to_ns(&delta));
+	}
+	mutex_unlock(&ptp_data->phc_lock);
+	return rc;
 }
 
 static int efx_phc_enable(struct ptp_clock_info *ptp,
-- 
2.34.1

[PATCH net v2 2/2] sfc: ptp: avoid racing PPS timestamp updates

[Alex Austin]

efx_ptp_pps_worker() can race with SIOCHWTSTAMP via
efx_ptp_change_mode(). After the worker completes synchronization, the
ioctl can start another synchronization and update host_time_pps while
the worker copies it into the PPS event. This can report a partially
updated timestamp. Serializing synchronization transactions alone does
not protect this copy, which happens after phc_lock is released.

Return the timestamp through the caller's own pps_event_time rather
than the shared field in efx_ptp_data, so the worker reads storage no
one else can write.

Fixes: 7c236c43b838 ("sfc: Add support for IEEE-1588 PTP")
Signed-off-by: Alex Austin <alex.austin@amd.com>
Reviewed-by: Alejandro Lucero <alucerop@amd.com>
Reviewed-by: Pieter Jansen van Vuuren <pieter.jansen-van-vuuren@amd.com>
---
 drivers/net/ethernet/sfc/ptp.c | 26 ++++++++++++++++----------
 1 file changed, 16 insertions(+), 10 deletions(-)

diff --git a/drivers/net/ethernet/sfc/ptp.c b/drivers/net/ethernet/sfc/ptp.c
index c2f8b94..116b380 100644
--- a/drivers/net/ethernet/sfc/ptp.c
+++ b/drivers/net/ethernet/sfc/ptp.c
@@ -268,7 +268,6 @@ struct efx_ptp_rxfilter {
  * @evt_frag_idx: Current fragment number
  * @evt_code: Last event code
  * @start: Address at which MC indicates ready for synchronisation
- * @host_time_pps: Host time at last PPS
  * @adjfreq_ppb_shift: Shift required to convert scaled parts-per-billion
  * frequency adjustment into a fixed point fractional nanosecond format.
  * @current_adjfreq: Current ppb adjustment.
@@ -330,7 +329,6 @@ struct efx_ptp_data {
 	int evt_frag_idx;
 	int evt_code;
 	struct efx_buffer start;
-	struct pps_event_time host_time_pps;
 	unsigned int adjfreq_ppb_shift;
 	s64 current_adjfreq;
 	struct ptp_clock *phc_clock;
@@ -912,7 +910,8 @@ static void efx_ptp_read_timeset(MCDI_DECLARE_STRUCT_PTR(data),
 static int
 efx_ptp_process_times(struct efx_nic *efx, MCDI_DECLARE_STRUCT_PTR(synch_buf),
 		      size_t response_length,
-		      const struct pps_event_time *last_time)
+		      const struct pps_event_time *last_time,
+		      struct pps_event_time *host_time_pps)
 {
 	unsigned number_readings =
 		MCDI_VAR_ARRAY_LEN(response_length,
@@ -990,6 +989,10 @@ efx_ptp_process_times(struct efx_nic *efx, MCDI_DECLARE_STRUCT_PTR(synch_buf),
 			   "PTP bad synchronisation seconds\n");
 		return -EAGAIN;
 	}
+
+	if (!host_time_pps)
+		return 0;
+
 	delta.tv_sec = (last_sec - start_sec) & 1;
 	delta.tv_nsec =
 		last_time->ts_real.tv_nsec -
@@ -1006,8 +1009,8 @@ efx_ptp_process_times(struct efx_nic *efx, MCDI_DECLARE_STRUCT_PTR(synch_buf),
 	delta.tv_nsec += ktime_to_timespec64(mc_time).tv_nsec;
 
 	/* Set PPS timestamp to match NIC top of second */
-	ptp->host_time_pps = *last_time;
-	pps_sub_ts(&ptp->host_time_pps, delta);
+	*host_time_pps = *last_time;
+	pps_sub_ts(host_time_pps, delta);
 
 	return 0;
 }
@@ -1020,7 +1023,8 @@ efx_ptp_process_times(struct efx_nic *efx, MCDI_DECLARE_STRUCT_PTR(synch_buf),
  * the function that registered it, so a secondary function can capture here
  * without serializing against the primary.
  */
-static int efx_ptp_synchronize(struct efx_nic *efx, unsigned int num_readings)
+static int efx_ptp_synchronize(struct efx_nic *efx, unsigned int num_readings,
+			       struct pps_event_time *host_time_pps)
 {
 	struct efx_ptp_data *ptp = efx->ptp_data;
 	MCDI_DECLARE_BUF(synch_buf, MC_CMD_PTP_OUT_SYNCHRONIZE_LENMAX);
@@ -1069,7 +1073,7 @@ static int efx_ptp_synchronize(struct efx_nic *efx, unsigned int num_readings)
 				 &response_length);
 	if (rc == 0) {
 		rc = efx_ptp_process_times(efx, synch_buf, response_length,
-					   &last_time);
+					   &last_time, host_time_pps);
 		if (rc == 0)
 			++ptp->good_syncs;
 		else
@@ -1504,14 +1508,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);
 }
 
@@ -1847,7 +1852,8 @@ int efx_ptp_change_mode(struct efx_nic *efx, bool enable_wanted,
 				rc = efx_ptp_start(efx);
 			if (rc == 0) {
 				rc = efx_ptp_synchronize(efx,
-							 PTP_SYNC_ATTEMPTS * 2);
+							 PTP_SYNC_ATTEMPTS * 2,
+							 NULL);
 				if (rc != 0)
 					efx_ptp_stop(efx);
 			}
-- 
2.34.1

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

[netdev-bot+sashiko]

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.

> diff --git a/drivers/net/ethernet/sfc/ptp.c b/drivers/net/ethernet/sfc/ptp.c
> index 6ca9a75af01d8..c2f8b94592718 100644
> --- a/drivers/net/ethernet/sfc/ptp.c
> +++ b/drivers/net/ethernet/sfc/ptp.c

[ ... ]

> @@ -272,6 +273,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 description cover more than the code does?

efx_phc_adjfine(), efx_phc_adjtime() and efx_phc_settime() take
phc_lock. efx_phc_gettime(), which is the gettime64 callback, still sends
MC_CMD_PTP_OP_READ_NIC_TIME without it when the PTP core calls it
directly:

drivers/net/ethernet/sfc/ptp.c:efx_phc_gettime() {
    ...
	rc = efx_mcdi_rpc(efx, MC_CMD_PTP, inbuf, sizeof(inbuf),
			  outbuf, sizeof(outbuf), NULL);
    ...
}

efx_phc_enable() also updates its state without the lock:

	ptp_data->nic_ts_enabled = !!enable;

Could the comment be limited to the operations that actually take
phc_lock?

[ ... ]

> @@ -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?

> @@ -1062,11 +1076,13 @@ static int efx_ptp_synchronize(struct efx_nic *efx, unsigned int num_readings)
>  			++ptp->no_time_syncs;
>  	}
>  
> +out:
>  	/* Increment the bad syncs counter if the synchronize fails, whatever
>  	 * the reason.
>  	 */
>  	if (rc != 0)
>  		++ptp->bad_syncs;
> +	mutex_unlock(&ptp->phc_lock);
>  
>  	return rc;
>  }

[Severity: Low]
After this unlock, efx_ptp_pps_worker() reads the shared synchronization
result without holding phc_lock:

drivers/net/ethernet/sfc/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);
}

Can a concurrent efx_ptp_change_mode()->efx_ptp_synchronize() rewrite
ptp->host_time_pps in efx_ptp_process_times() during that read?

	ptp->host_time_pps = *last_time;
	pps_sub_ts(&ptp->host_time_pps, delta);

If so, ptp_clock_event() would get a torn or foreign timestamp.

The next patch in the series, "sfc: ptp: avoid racing PPS timestamp
updates", removes host_time_pps. It copies the result into a stack-local
pps_event_time under phc_lock, so the unlocked read is gone by the end of
the series.

[ ... ]

> @@ -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?

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

Re: [PATCH net v2 2/2] sfc: ptp: avoid racing PPS timestamp updates

[netdev-bot+sashiko]

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] The fix only covers drivers/net/ethernet/sfc/ptp.c.

--

sfc: ptp: avoid racing PPS timestamp updates

This commit removes the shared host_time_pps field from efx_ptp_data.
efx_ptp_process_times() now writes the PPS timestamp into storage that
the caller provides. efx_ptp_pps_worker() passes a stack-local
pps_event_time, and efx_ptp_change_mode() passes NULL.

> diff --git a/drivers/net/ethernet/sfc/ptp.c b/drivers/net/ethernet/sfc/ptp.c
> index c2f8b94592718..116b38079d8e6 100644
> --- a/drivers/net/ethernet/sfc/ptp.c
> +++ b/drivers/net/ethernet/sfc/ptp.c

[ ... ]

> @@ -1504,14 +1508,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]
Should the same change also go into drivers/net/ethernet/sfc/siena/ptp.c?

That file was split off from the code that commit 7c236c43b838 added,
and it still has the shared field. In the Siena copy of
efx_ptp_process_times():

	ptp->host_time_pps = *last_time;
	pps_sub_ts(&ptp->host_time_pps, delta);

The Siena efx_ptp_pps_worker() then copies it after synchronize returns:

	if (efx_ptp_synchronize(efx, PTP_SYNC_ATTEMPTS))
		return;

	ptp_evt.type = PTP_CLOCK_PPSUSR;
	ptp_evt.pps_times = ptp->host_time_pps;

Meanwhile, the timestamping config path can still rewrite that field:

siena_ptp_set_ts_config()
  efx_siena_ptp_change_mode()
    efx_ptp_synchronize(efx, PTP_SYNC_ATTEMPTS * 2)
      efx_ptp_process_times()

This is the same partially updated PPS timestamp race that the commit
message describes for efx_ptp_pps_worker() and change_mode.

The Siena efx_ptp_synchronize() also takes no driver lock around the
transaction, while the sfc version holds phc_lock. Only the MCDI request
itself is serialized. Two concurrent synchronizations could therefore
overlap on these:

	WRITE_ONCE(*start, 0) on the shared DMA start flag
	the ptp->timeset[] writes in efx_ptp_process_times()
	the sync stats counters

Since the patch carries the same Fixes tag, stable kernels that have the
Siena split would get this fix only for sfc. Could Siena get the same
treatment, meaning a caller-owned pps_event_time, NULL from
efx_siena_ptp_change_mode(), and serialization of efx_ptp_synchronize()?
If Siena is left out on purpose, could the commit message say so?

> @@ -1847,7 +1852,8 @@ int efx_ptp_change_mode(struct efx_nic *efx, bool enable_wanted,
>  				rc = efx_ptp_start(efx);
>  			if (rc == 0) {
>  				rc = efx_ptp_synchronize(efx,
> -							 PTP_SYNC_ATTEMPTS * 2);
> +							 PTP_SYNC_ATTEMPTS * 2,
> +							 NULL);
>  				if (rc != 0)
>  					efx_ptp_stop(efx);
>  			}

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

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@kernel.org 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