* [PATCH net v2 1/2] sfc: ptp: serialize host and MC synchronization transactions
2026-10-15 11:41 [PATCH net v2 0/2] sfc: fix PTP synchronization races Alex Austin
@ 2026-10-15 11:41 ` Alex Austin
2026-10-15 11:41 ` [PATCH net v2 2/2] sfc: ptp: avoid racing PPS timestamp updates Alex Austin
1 sibling, 0 replies; 3+ messages in thread
From: Alex Austin @ 2026-10-15 11:41 UTC (permalink / raw)
To: Edward Cree, Andrew Lunn, David S . Miller, Eric Dumazet,
Jakub Kicinski, Paolo Abeni
Cc: Richard Cochran, Alejandro Lucero, Pieter Jansen van Vuuren,
netdev, linux-kernel, 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
^ permalink raw reply related [flat|nested] 3+ messages in thread* [PATCH net v2 2/2] sfc: ptp: avoid racing PPS timestamp updates
2026-10-15 11:41 [PATCH net v2 0/2] sfc: fix PTP synchronization races Alex Austin
2026-10-15 11:41 ` [PATCH net v2 1/2] sfc: ptp: serialize host and MC synchronization transactions Alex Austin
@ 2026-10-15 11:41 ` Alex Austin
1 sibling, 0 replies; 3+ messages in thread
From: Alex Austin @ 2026-10-15 11:41 UTC (permalink / raw)
To: Edward Cree, Andrew Lunn, David S . Miller, Eric Dumazet,
Jakub Kicinski, Paolo Abeni
Cc: Richard Cochran, Alejandro Lucero, Pieter Jansen van Vuuren,
netdev, linux-kernel, 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
^ permalink raw reply related [flat|nested] 3+ messages in thread