mirror of
https://github.com/torvalds/linux.git
synced 2026-11-07 06:17:02 +01:00
Merge branch 'sfc-fix-ptp-synchronization-races'
Alex Austin says:
====================
sfc: fix PTP synchronization races
This series fixes PTP synchronization races found by code inspection, and
a related frequency-adjustment bug found while reviewing the locking.
efx_ptp_start() resets the driver's cached current_adjfreq as though
MC_CMD_PTP_OP_ENABLE also resets the NIC's frequency adjustment on the
MC. It does not, so the next time-only adjustment reprograms the clock
from a stale cache. Patch 1 removes this reset.
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 2 serializes a function's synchronization transactions and clock
operations under a new mutex. Patch 3 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).
====================
Link: https://patch.msgid.link/20261026123023.3566549-1-alex.austin@amd.com
Signed-off-by: Paolo Abeni <pabeni@redhat.com>
This commit is contained in:
Paolo Abeni
commit
631c3b1859
|
|
@ -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"
|
||||
|
|
@ -266,11 +267,12 @@ 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.
|
||||
* @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
|
||||
|
|
@ -326,10 +328,10 @@ 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;
|
||||
struct mutex phc_lock;
|
||||
struct ptp_clock_info phc_clock_info;
|
||||
struct work_struct pps_work;
|
||||
struct workqueue_struct *pps_workwq;
|
||||
|
|
@ -907,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,
|
||||
|
|
@ -985,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 -
|
||||
|
|
@ -1001,14 +1008,22 @@ 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;
|
||||
}
|
||||
|
||||
/* Synchronize times between the host and the MC */
|
||||
static int efx_ptp_synchronize(struct efx_nic *efx, unsigned int num_readings)
|
||||
/* 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 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);
|
||||
|
|
@ -1026,11 +1041,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);
|
||||
|
|
@ -1054,18 +1072,20 @@ 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
|
||||
++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;
|
||||
}
|
||||
|
|
@ -1445,7 +1465,6 @@ static int efx_ptp_start(struct efx_nic *efx)
|
|||
goto fail;
|
||||
|
||||
ptp->evt_frag_idx = 0;
|
||||
ptp->current_adjfreq = 0;
|
||||
|
||||
return 0;
|
||||
|
||||
|
|
@ -1485,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);
|
||||
}
|
||||
|
||||
|
|
@ -1569,6 +1589,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;
|
||||
|
||||
|
|
@ -1640,6 +1661,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;
|
||||
|
||||
|
|
@ -1694,6 +1716,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;
|
||||
}
|
||||
|
|
@ -1825,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);
|
||||
}
|
||||
|
|
@ -2105,23 +2129,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);
|
||||
|
||||
|
|
@ -2134,6 +2158,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,
|
||||
|
|
@ -2165,23 +2201,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,
|
||||
|
|
|
|||
Loading…
Reference in New Issue
Block a user