From 21d4917cfc8b6837c44d8aa186941a6a16ec17a0 Mon Sep 17 00:00:00 2001 From: Alex Austin Date: Mon, 26 Oct 2026 12:30:21 +0000 Subject: [PATCH 1/3] sfc: ptp: drop the stale frequency reset in efx_ptp_start() FW does not touch the clock frequency state on MC_CMD_PTP_OP_DISABLE or MC_CMD_PTP_OP_ENABLE, so the driver doesn't need to either. Doing so just desyncs the driver's cached current_adjfreq from what FW still has programmed. Resetting current_adjfreq to 0 here means the first efx_phc_adjtime() call after efx_ptp_start() reprograms the NIC's clock frequency to 0, since efx_phc_adjtime() always sends current_adjfreq to FW alongside the time step. efx_ptp_start() runs on every SIOCSHWTSTAMP mode change, on ifup, and on the reset_required path in efx_ptp_worker(), so each of those silently drops any frequency correction the PTP servo had already applied. efx_ptp_start() is also reached from reset recovery, but that is fine: on an FLR or PCIe reset FW does not touch the NIC clock, and on a full MC reboot the clock is reset to 1970, so a stale current_adjfreq from before the reset is hardly consequential - especially as it is bound to MAX_PPB, 1000000 ppb. Fixes: 7c236c43b838 ("sfc: Add support for IEEE-1588 PTP") Signed-off-by: Alex Austin Link: https://patch.msgid.link/20261026123023.3566549-2-alex.austin@amd.com Signed-off-by: Paolo Abeni --- drivers/net/ethernet/sfc/ptp.c | 1 - 1 file changed, 1 deletion(-) diff --git a/drivers/net/ethernet/sfc/ptp.c b/drivers/net/ethernet/sfc/ptp.c index 00cfd8d5b46e..957ca08092e7 100644 --- a/drivers/net/ethernet/sfc/ptp.c +++ b/drivers/net/ethernet/sfc/ptp.c @@ -1445,7 +1445,6 @@ static int efx_ptp_start(struct efx_nic *efx) goto fail; ptp->evt_frag_idx = 0; - ptp->current_adjfreq = 0; return 0; From f707347b33feebdd67633bccea52f96807c18d6e Mon Sep 17 00:00:00 2001 From: Alex Austin Date: Mon, 26 Oct 2026 12:30:22 +0000 Subject: [PATCH 2/3] sfc: ptp: serialize host and MC synchronization transactions 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 is consistently read and written under one lock. 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 Reviewed-by: Alejandro Lucero Reviewed-by: Pieter Jansen van Vuuren Acked-by: Edward Cree Link: https://patch.msgid.link/20261026123023.3566549-3-alex.austin@amd.com Signed-off-by: Paolo Abeni --- drivers/net/ethernet/sfc/ptp.c | 81 +++++++++++++++++++++++----------- 1 file changed, 55 insertions(+), 26 deletions(-) diff --git a/drivers/net/ethernet/sfc/ptp.c b/drivers/net/ethernet/sfc/ptp.c index 957ca08092e7..e26269fae38d 100644 --- a/drivers/net/ethernet/sfc/ptp.c +++ b/drivers/net/ethernet/sfc/ptp.c @@ -36,6 +36,7 @@ #include #include #include +#include #include #include #include "net_driver.h" @@ -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 * @phc_clock_info: Registration structure for phc device * @pps_work: pps work task for handling pps events * @pps_workwq: pps work queue @@ -330,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; @@ -1007,7 +1011,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; @@ -1026,11 +1037,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); @@ -1061,11 +1075,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; } @@ -1568,6 +1584,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; @@ -1639,6 +1656,7 @@ int efx_ptp_probe(struct efx_nic *efx, struct efx_channel *channel) efx_nic_free_buffer(efx, &ptp->start); fail1: + mutex_destroy(&ptp->phc_lock); kfree(efx->ptp_data); efx->ptp_data = NULL; @@ -1693,6 +1711,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; } @@ -2104,23 +2123,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); @@ -2133,6 +2152,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, @@ -2164,23 +2195,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, From 23d23e6c6517d0fea4f64340a23e33c89f82c03c Mon Sep 17 00:00:00 2001 From: Alex Austin Date: Mon, 26 Oct 2026 12:30:23 +0000 Subject: [PATCH 3/3] sfc: ptp: avoid racing PPS timestamp updates 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 Reviewed-by: Alejandro Lucero Reviewed-by: Pieter Jansen van Vuuren Link: https://patch.msgid.link/20261026123023.3566549-4-alex.austin@amd.com Signed-off-by: Paolo Abeni --- 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 e26269fae38d..37511f234d76 100644 --- a/drivers/net/ethernet/sfc/ptp.c +++ b/drivers/net/ethernet/sfc/ptp.c @@ -267,7 +267,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 +328,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 +909,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, @@ -989,6 +988,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 - @@ -1005,8 +1008,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; } @@ -1019,7 +1022,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); @@ -1068,7 +1072,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 @@ -1500,14 +1504,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); } @@ -1843,7 +1848,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); }