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

[PATCH net 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 synchronization state.
PHC adjustments can also change the clock during a capture. Patch 1
serializes synchronization transactions and PHC operations. Patch 2
returns the PPS timestamp in caller-owned storage to prevent concurrent
synchronization from overwriting it while the PPS worker reads it.

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

 drivers/net/ethernet/sfc/ptp.c | 136 ++++++++++++++++++++++-----------
 1 file changed, 90 insertions(+), 46 deletions(-)

-- 
2.34.1

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

[Alex Austin]

Protect the DMA handshake and result processing against concurrent
synchronization callers. Serialize clock adjustments with the same mutex
so they cannot change the PHC during a capture.

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 | 102 +++++++++++++++++++++------------
 1 file changed, 66 insertions(+), 36 deletions(-)

diff --git a/drivers/net/ethernet/sfc/ptp.c b/drivers/net/ethernet/sfc/ptp.c
index 6ca9a75..ac9951f 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,7 @@ 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
  * @phc_clock_info: Registration structure for phc device
  * @pps_work: pps work task for handling pps events
  * @pps_workwq: pps work queue
@@ -331,6 +333,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;
@@ -1027,11 +1030,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 +1068,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;
 }
@@ -1570,6 +1578,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 +1650,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 +1705,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 +2117,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,16 +2146,29 @@ static int efx_phc_adjtime(struct ptp_clock_info *ptp, s64 delta)
 			    NULL, 0, NULL);
 }
 
-static int efx_phc_gettime(struct ptp_clock_info *ptp, struct timespec64 *ts)
+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);
-	struct efx_nic *efx = ptp_data->efx;
-	MCDI_DECLARE_BUF(inbuf, MC_CMD_PTP_IN_READ_NIC_TIME_LEN);
-	MCDI_DECLARE_BUF(outbuf, MC_CMD_PTP_OUT_READ_NIC_TIME_LEN);
+	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 efx_ptp_data *ptp_data,
+			    struct timespec64 *ts)
+	__must_hold(&ptp_data->phc_lock)
+{
+	MCDI_DECLARE_BUF(outbuf, MC_CMD_PTP_OUT_READ_NIC_TIME_LEN);
+	MCDI_DECLARE_BUF(inbuf, MC_CMD_PTP_IN_READ_NIC_TIME_LEN);
+	struct efx_nic *efx = ptp_data->efx;
 	ktime_t kt;
+	int rc;
+
+	lockdep_assert_held(&ptp_data->phc_lock);
 
 	MCDI_SET_DWORD(inbuf, PTP_IN_OP, MC_CMD_PTP_OP_READ_NIC_TIME);
 	MCDI_SET_DWORD(inbuf, PTP_IN_PERIPH_ID, 0);
@@ -2161,28 +2185,34 @@ static int efx_phc_gettime(struct ptp_clock_info *ptp, struct timespec64 *ts)
 	return 0;
 }
 
+static int efx_phc_gettime(struct ptp_clock_info *ptp, struct timespec64 *ts)
+{
+	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_gettime(ptp_data, ts);
+	mutex_unlock(&ptp_data->phc_lock);
+	return rc;
+}
+
 static int efx_phc_settime(struct ptp_clock_info *ptp,
 			   const struct timespec64 *e_ts)
 {
-	/* Get the current NIC time, efx_phc_gettime.
-	 * Subtract from the desired time to get 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;
 
-	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;
+	mutex_lock(&ptp_data->phc_lock);
+	rc = _efx_phc_gettime(ptp_data, &time_now);
+	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 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.

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 | 34 ++++++++++++++++++++++++----------
 1 file changed, 24 insertions(+), 10 deletions(-)

diff --git a/drivers/net/ethernet/sfc/ptp.c b/drivers/net/ethernet/sfc/ptp.c
index ac9951f..103df1f 100644
--- a/drivers/net/ethernet/sfc/ptp.c
+++ b/drivers/net/ethernet/sfc/ptp.c
@@ -205,6 +205,14 @@ struct efx_ptp_timeset {
 	u32 window;	/* Derived: end - start, allowing for wrap */
 };
 
+/**
+ * struct efx_ptp_sync_result - Result of host and MC synchronisation
+ * @host_time_pps: Host time at the NIC top of second
+ */
+struct efx_ptp_sync_result {
+	struct pps_event_time host_time_pps;
+};
+
 /**
  * struct efx_ptp_rxfilter - Filter for PTP packets
  * @list: Node of the list where the filter is added
@@ -268,7 +276,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.
@@ -329,7 +336,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;
@@ -911,7 +917,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 efx_ptp_sync_result *result)
 {
 	unsigned number_readings =
 		MCDI_VAR_ARRAY_LEN(response_length,
@@ -989,6 +996,10 @@ efx_ptp_process_times(struct efx_nic *efx, MCDI_DECLARE_STRUCT_PTR(synch_buf),
 			   "PTP bad synchronisation seconds\n");
 		return -EAGAIN;
 	}
+
+	if (!result)
+		return 0;
+
 	delta.tv_sec = (last_sec - start_sec) & 1;
 	delta.tv_nsec =
 		last_time->ts_real.tv_nsec -
@@ -1005,14 +1016,15 @@ 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);
+	result->host_time_pps = *last_time;
+	pps_sub_ts(&result->host_time_pps, delta);
 
 	return 0;
 }
 
 /* Synchronize times between the host and the MC */
-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 efx_ptp_sync_result *result)
 {
 	struct efx_ptp_data *ptp = efx->ptp_data;
 	MCDI_DECLARE_BUF(synch_buf, MC_CMD_PTP_OUT_SYNCHRONIZE_LENMAX);
@@ -1061,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, result);
 		if (rc == 0)
 			++ptp->good_syncs;
 		else
@@ -1494,14 +1506,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 efx_ptp_sync_result result = {};
 	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, &result))
 		return;
 
 	ptp_evt.type = PTP_CLOCK_PPSUSR;
-	ptp_evt.pps_times = ptp->host_time_pps;
+	ptp_evt.pps_times = result.host_time_pps;
 	ptp_clock_event(ptp->phc_clock, &ptp_evt);
 }
 
@@ -1837,7 +1850,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 0/2] sfc: fix PTP synchronization races

[netdev-bot+sinfo]

Hi!

This is an automated message. This series looks like a fix, but its
commit messages seem to be missing some information:

 - What hardware the change was tested on. For driver fixes please
   mention the device (and if relevant firmware version) used for
   testing, or say that the change was not tested on real hardware.

Please do not repost the series just to address the above. Instead,
reply to this email with the missing information, so that reviewers
can take it into account. If the series needs another revision for
other reasons, please include the information in the commit messages
then.

The evaluation is done by an LLM so it may be wrong, if you think
that is the case please reply and explain.

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

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 5 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 3 (1) · 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] At this commit, ptp->host_time_pps is written by
  efx_ptp_process_times() while phc_lock is held.
- [Medium] phc_lock is created separately in each PCI function's
  efx_ptp_data: efx_ptp_probe() calls mutex_init(&ptp->phc_lock) for…
- [Medium] sfc: data race on current_adjfreq in efx_ptp_start
- [Low] The patch changes how efx_ptp_synchronize()
  (drivers/net/ethernet/sfc/ptp.c) handles a failed…
- [Low] The commit message says clock adjustments are serialized with
  phc_lock "so they cannot change the PHC during a capture".

Pre-existing issues:
- [Medium] efx_ptp_start() resets ptp->current_adjfreq = 0 (line 1457)
  without phc_lock.

--

sfc: ptp: serialize host and MC synchronization transactions

This commit adds a phc_lock mutex to struct efx_ptp_data and holds it
across the SYNCHRONIZE DMA handshake in efx_ptp_synchronize(). The PHC
adjfine, adjtime, gettime and settime callbacks are serialized with the
same mutex, and efx_phc_settime() now reads and adjusts under one lock
hold.

> Protect the DMA handshake and result processing against concurrent
> synchronization callers. Serialize clock adjustments with the same mutex
> so they cannot change the PHC during a capture.

[Severity: Low]
Is this rationale accurate for adjustments made on the same function?

efx_ptp_synchronize() acquires the MCDI interface in
efx_mcdi_rpc_start()->efx_mcdi_acquire_sync(). It only releases it at
the end of efx_mcdi_rpc_finish():

drivers/net/ethernet/sfc/mcdi.c:efx_mcdi_acquire_sync() {
	wait_event(mcdi->wq,
		   cmpxchg(&mcdi->state,
			   MCDI_STATE_QUIESCENT, MCDI_STATE_RUNNING_SYNC) ==
		   MCDI_STATE_QUIESCENT);
}

efx_phc_adjfine(), efx_phc_adjtime() and efx_phc_gettime() all go
through efx_mcdi_rpc()->_efx_mcdi_rpc()->efx_mcdi_rpc_start(). So even
before this patch, they could not send an MC command while SYNCHRONIZE
was in flight.

After rpc_finish, efx_ptp_process_times() only uses the response buffer
and nic_to_kernel_time(). It does not use current_adjfreq.

The commit message doesn't mention what the new PHC locking actually
adds:

- efx_phc_settime() now does its gettime and adjtime atomically.
- The current_adjfreq cached by efx_phc_adjfine() now stays consistent
  with the PTP_IN_ADJUST_FREQ value that _efx_phc_adjtime() sends.

Could the commit message describe these changes instead?

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

[ ... ]

> @@ -1027,11 +1030,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);
> +

[Severity: Medium]
Can a per-function phc_lock protect a PHC that is shared across PCI
functions?

efx_ptp_probe() calls mutex_init(&ptp->phc_lock) for every function.
Only the primary function registers the clock with ptp_clock_register().
efx_ptp_get_ts_info() reports the primary's clock for secondaries too:

drivers/net/ethernet/sfc/ptp.c:efx_ptp_get_ts_info() {
    ...
	if (primary && primary->ptp_data && primary->ptp_data->phc_clock)
		ts_info->phc_index =
			ptp_clock_index(primary->ptp_data->phc_clock);
    ...
}

A secondary function still captures against that clock through
efx_ptp_change_mode()->efx_ptp_synchronize(). While it does so it holds
only secondary->ptp_data->phc_lock.

On the primary, efx_phc_adjtime(), efx_phc_settime() and
efx_phc_adjfine() take primary->ptp_data->phc_lock, which is a different
mutex. They send MC_CMD_PTP_OP_ADJUST over the primary's own MCDI
interface, so per-function MCDI serialization doesn't cover this case
either.

If ptp4l on the primary steps the PHC while SIOCSHWTSTAMP runs on a
secondary, could the secondary's timesets become inconsistent?

In that case efx_ptp_process_times() would return -EAGAIN. The baseline
synchronisation in efx_ptp_change_mode() would then fail, and PTP would
be stopped on that function:

			if (rc == 0) {
				rc = efx_ptp_synchronize(efx,
							 PTP_SYNC_ATTEMPTS * 2);
				if (rc != 0)
					efx_ptp_stop(efx);
			}

Whether this actually goes wrong depends on whether the MC firmware
processes the primary's ADJUST while it is servicing the secondary's
SYNCHRONIZE. That can't be confirmed from the driver.

Would a lock scoped to the clock be needed to fully cover the case the
commit message describes? One option is a lock shared through
efx->primary->ptp_data.

>  	/* 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: Low]
This change looks correct, but it fixes a separate bug that the commit
message doesn't mention.

efx_mcdi_rpc_start() can fail before it calls efx_mcdi_acquire_sync().
This happens when efx_mcdi_check_supported() fails, when
mc_bist_for_other_fn is set, or when mcdi->mode is MCDI_MODE_FAIL:

drivers/net/ethernet/sfc/mcdi.c:efx_mcdi_rpc_start() {
    ...
	if (efx->mc_bist_for_other_fn)
		return -ENETDOWN;

	if (mcdi->mode == MCDI_MODE_FAIL)
		return -ENETDOWN;

	efx_mcdi_acquire_sync(mcdi);
    ...
}

Without paranoid checks, EFX_WARN_ON_ONCE_PARANOID() compiled to
nothing. The old code then busy-waited for up to MAX_SYNCHRONISE_WAIT_MS
and called efx_mcdi_rpc_finish() anyway.

That path waited for a completion that never came and called
efx_mcdi_abandon(). It then called efx_mcdi_release() on an interface
this caller had never acquired.

Could this be described in the commit message, or split into its own
patch with its own justification?

[ ... ]

> @@ -1062,11 +1068,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: Medium]
Is ptp->host_time_pps still racy after this unlock?

efx_ptp_process_times() writes ptp->host_time_pps while phc_lock is
held. efx_ptp_pps_worker() reads it only after efx_ptp_synchronize() has
dropped the mutex:

drivers/net/ethernet/sfc/ptp.c:efx_ptp_pps_worker() {
	...
	if (efx_ptp_synchronize(efx, PTP_SYNC_ATTEMPTS))
		return;

	ptp_evt.type = PTP_CLOCK_PPSUSR;
	ptp_evt.pps_times = ptp->host_time_pps;
	ptp_clock_event(ptp->phc_clock, &ptp_evt);
}

A concurrent SIOCSHWTSTAMP can go through
efx_ptp_change_mode()->efx_ptp_synchronize() and overwrite host_time_pps
while the worker is copying it. The PPS event passed to
ptp_clock_event() could then carry a torn timestamp.

This race existed before this commit.

The next patch in this series, "sfc: ptp: avoid racing PPS timestamp
updates", removes host_time_pps from struct efx_ptp_data. That patch
returns the result through a caller-owned struct efx_ptp_sync_result
filled in under phc_lock, and efx_ptp_change_mode() passes NULL. The
race is gone by the end of the series.

[ ... ]

> @@ -2106,23 +2117,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;
>  }

[Severity: Medium]
efx_ptp_start() still writes ptp->current_adjfreq without taking
phc_lock:

drivers/net/ethernet/sfc/ptp.c:efx_ptp_start() {
    ...
	ptp->evt_frag_idx = 0;
	ptp->current_adjfreq = 0;
    ...
}

current_adjfreq is an s64. _efx_phc_adjtime() reads it under phc_lock
and passes it to MCDI_SET_QWORD(). That macro evaluates its value
argument twice, once for the low dword and once for the high dword.
Even on 64-bit the compiler is free to emit two separate loads. On
32-bit, the store in efx_ptp_start() is also two separate 32-bit
writes.

efx_ptp_start() is reached in two ways that don't hold phc_lock:

- SIOCSHWTSTAMP via efx_ptp_change_mode()
- efx_ptp_worker(), when reset_required is set

So a concurrent clock_adjtime() on the PHC can run _efx_phc_adjtime()
at the same moment.

Could the MC then receive a PTP_IN_ADJUST_FREQ built from the low half
of one value and the high half of another? Suppose current_adjfreq
held a small negative value. The low dword could read the new 0 while
the high dword still reads 0xffffffff. The result is a frequency
adjustment of roughly -4.3e9, which is neither the old value nor 0.
How the firmware reacts to such a value can't be determined from the
driver.

This is the same unlocked write raised above for efx_phc_adjfine(),
but the consequence here is a corrupted value rather than just a lost
one. This patch makes phc_lock the lock for current_adjfreq. Should
efx_ptp_start() also take phc_lock around the reset?

efx_ptp_start() runs in process context. It doesn't appear to be
called with phc_lock already held: efx_ptp_change_mode() calls
efx_ptp_synchronize() only after efx_ptp_start() returns. So taking
the mutex there looks straightforward.

[Severity: Medium]
This is a pre-existing issue and was not introduced by this patch.
efx_ptp_start() still resets current_adjfreq without taking phc_lock:

drivers/net/ethernet/sfc/ptp.c:efx_ptp_start() {
    ...
	ptp->evt_frag_idx = 0;
	ptp->current_adjfreq = 0;
    ...
}

efx_ptp_start() is reached from three places:

- efx_ptp_worker(), when reset_required is set
- efx_ptp_start_datapath()->efx_ptp_restart()
- efx_ptp_change_mode()

None of them take phc_lock, so efx_ptp_start() can run alongside the
PHC clock ioctls.

Could this interleaving drop the servo's frequency correction?

efx_phc_adjfine()
  sends frequency X to the MC
  current_adjfreq = X
                              efx_ptp_start()
                                current_adjfreq = 0
efx_phc_adjtime()
  _efx_phc_adjtime()
    sends PTP_IN_ADJUST_FREQ = 0

The next adjfine would correct this, so the frequency error is
transient.

Now that phc_lock protects the other accesses to current_adjfreq,
should efx_ptp_start() take it as well?

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

Re: [PATCH net 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 (1)

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 Siena PTP driver still has the PPS timestamp race that
  this patch fixes in sfc.

Pre-existing issues:
- [Low] The comment above efx_ptp_process_times() in
  drivers/net/ethernet/sfc/ptp.c (lines 909-916) says the function will…

--

sfc: ptp: avoid racing PPS timestamp updates

This patch removes the shared host_time_pps field from struct
efx_ptp_data in the sfc driver. The computed PPS timestamp is now returned
through a caller-owned struct efx_ptp_sync_result. efx_ptp_pps_worker()
uses a stack-local result, and efx_ptp_change_mode() passes NULL.

> diff --git a/drivers/net/ethernet/sfc/ptp.c b/drivers/net/ethernet/sfc/ptp.c
> index ac9951fee5ddd..103df1f8fa92d 100644
> --- a/drivers/net/ethernet/sfc/ptp.c
> +++ b/drivers/net/ethernet/sfc/ptp.c

[ ... ]

> @@ -911,7 +917,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 efx_ptp_sync_result *result)
>  {

[Severity: Low]
This isn't a bug introduced by this patch, but the comment above
efx_ptp_process_times() doesn't match what the function does:

 * Extract times from returned results, and establish the minimum value
 * seen.  The minimum value represents the "best" possible time and events
 * too much greater than this are rejected - the machine is, perhaps, too
 * busy.

The loop never computes an observed minimum. It checks each reading
against SYNCHRONISATION_GRANULARITY_NS, MAX_SYNCHRONISATION_NS and
ptp->min_synchronisation_ns, and every reading that passes updates
last_good:

		} else if (corrected < ptp->min_synchronisation_ns) {
			++ptp->undersize_sync_windows;
		} else {
			ngood++;
			last_good = i;
		}

When several readings pass, the function uses the last one, not the one
with the smallest window.

Could the comment be updated to describe the fixed thresholds and the
last-good selection?

[ ... ]

> @@ -1494,14 +1506,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 efx_ptp_sync_result result = {};
>  	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, &result))
>  		return;
>  
>  	ptp_evt.type = PTP_CLOCK_PPSUSR;
> -	ptp_evt.pps_times = ptp->host_time_pps;
> +	ptp_evt.pps_times = result.host_time_pps;
>  	ptp_clock_event(ptp->phc_clock, &ptp_evt);
>  }

[Severity: Medium]
Does the Siena driver still have the same race?

drivers/net/ethernet/sfc/siena/ptp.c is a near copy of this file. It still
keeps host_time_pps as a shared field in its struct efx_ptp_data.

Its efx_ptp_process_times() writes that field on every successful
synchronization:

drivers/net/ethernet/sfc/siena/ptp.c:efx_ptp_process_times() {
    ...
	/* Set PPS timestamp to match NIC top of second */
	ptp->host_time_pps = *last_time;
	pps_sub_ts(&ptp->host_time_pps, delta);
    ...
}

Its efx_ptp_pps_worker() copies the field after the synchronization
returns:

drivers/net/ethernet/sfc/siena/ptp.c:efx_ptp_pps_worker() {
    ...
	if (efx_ptp_synchronize(efx, PTP_SYNC_ATTEMPTS))
		return;

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

The SIOCSHWTSTAMP path can rewrite the field at the same time:

efx_siena_ptp_change_mode()
    efx_ptp_synchronize(efx, PTP_SYNC_ATTEMPTS * 2)
        efx_ptp_process_times()
            ptp->host_time_pps = *last_time;

The Siena efx_ptp_synchronize() also takes no lock, because siena/ptp.c
has no phc_lock. So Siena has neither the serialization from "sfc: ptp:
serialize host and MC synchronization transactions" nor the caller-owned
result added here. Concurrent synchronizations can overwrite ptp->timeset[]
and host_time_pps while the worker reads them.

Both copies come from the Fixes: commit 7c236c43b838, and the commit
message does not mention Siena.

Should siena/ptp.c get the same fix, either in this patch or as a
follow-up?

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