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

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

[Alex Austin]

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).

Changes since v3:
 - fix the Fixes: tag in patch 1 to 7c236c43b838, matching patches 2
   and 3 (it previously pointed at a non-existent hash, ffe9554c17bf)
 - expand patch 1's commit message to describe the user-visible effect
Changes since v2:
 - add patch 1, dropping the stale current_adjfreq reset in
   efx_ptp_start() found while re-reviewing the locking this series
   adds.
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 (3):
  sfc: ptp: drop the stale frequency reset in efx_ptp_start()
  sfc: ptp: serialize host and MC synchronization transactions
  sfc: ptp: avoid racing PPS timestamp updates

 drivers/net/ethernet/sfc/ptp.c | 108 ++++++++++++++++++++++-----------
 1 file changed, 71 insertions(+), 37 deletions(-)

-- 
2.34.1

[PATCH net v4 1/3] sfc: ptp: drop the stale frequency reset in efx_ptp_start()

[Alex Austin]

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 <alex.austin@amd.com>
---
 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 6ca9a75..2247e5e 100644
--- a/drivers/net/ethernet/sfc/ptp.c
+++ b/drivers/net/ethernet/sfc/ptp.c
@@ -1446,7 +1446,6 @@ static int efx_ptp_start(struct efx_nic *efx)
 		goto fail;
 
 	ptp->evt_frag_idx = 0;
-	ptp->current_adjfreq = 0;
 
 	return 0;
 
-- 
2.34.1

Re: [PATCH net v4 1/3] sfc: ptp: drop the stale frequency reset in efx_ptp_start()

[netdev-bot+sashiko]

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

Critical: 0 · High: 0 · Medium: 1 (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 fix only changes the EF10/EF100 copy of efx_ptp_start() in
  drivers/net/ethernet/sfc/ptp.c.

Pre-existing issues:
- [Medium] At this commit, efx_phc_adjtime() reads
  ptp_data->current_adjfreq (drivers/net/ethernet/sfc/ptp.c:2129) and…
- [Low] The kernel-doc for struct efx_ptp_data says `@current_adjfreq:
  Current ppb adjustment.` (drivers/net/ethernet/sfc/ptp.c:272, and…

--

sfc: ptp: drop the stale frequency reset in efx_ptp_start()

This commit stops efx_ptp_start() from clearing the cached
current_adjfreq. Firmware keeps the clock frequency across PTP disable
and enable. Clearing the cache made the next efx_phc_adjtime() set the
NIC's frequency trim back to 0.

> diff --git a/drivers/net/ethernet/sfc/ptp.c b/drivers/net/ethernet/sfc/ptp.c
> index 00cfd8d5b46ea..957ca08092e7f 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;

[Severity: Medium]
Should the Siena copy of efx_ptp_start() get the same change? The Siena
version still clears the cache right after efx_ptp_enable() succeeds:

drivers/net/ethernet/sfc/siena/ptp.c:efx_ptp_start() {
    ...
	rc = efx_ptp_enable(efx);
	if (rc != 0)
		goto fail;

	ptp->evt_frag_idx = 0;
	ptp->current_adjfreq = 0;
    ...
}

Siena's efx_phc_adjtime() also sends the cached value with every time
step:

	MCDI_SET_QWORD(inbuf, PTP_IN_ADJUST_FREQ, ptp_data->current_adjfreq);

On Siena, all the paths named in the commit message still reach that
reset:

  efx_siena_ptp_change_mode() -> efx_ptp_start()    (SIOCSHWTSTAMP)
  efx_ptp_restart() -> efx_ptp_start()              (ifup, reset recovery)
  efx_ptp_worker() -> efx_ptp_start()               (reset_required)

So the next adjtime or settime would set the Siena NIC's frequency trim to
0. Meanwhile the servo and the PTP core still hold the old value.

The Fixes: tag points at 7c236c43b838, which added this code for Siena
hardware. The later patches in the series ("sfc: ptp: serialize host and
MC synchronization transactions" and "sfc: ptp: avoid racing PPS
timestamp updates") don't touch siena/ptp.c either.

This assumes Siena MC firmware also keeps the frequency across
MC_CMD_PTP_OP_DISABLE and MC_CMD_PTP_OP_ENABLE. The commit message only
says this for the firmware it targets. Does Siena firmware behave the
same way? If so, should drivers/net/ethernet/sfc/siena/ptp.c be fixed as
well?

[Severity: Low]
This isn't a bug introduced by this patch, but the kernel-doc for struct
efx_ptp_data, in both drivers/net/ethernet/sfc/ptp.c and
drivers/net/ethernet/sfc/siena/ptp.c, says:

 * @current_adjfreq: Current ppb adjustment.

The field doesn't actually hold ppb. efx_phc_adjfine() stores the
fixed-point fractional ns value that firmware takes:

	adjustment_ns = ((s64)delta * PPB_SCALE_WORD +
			 (1 << (ptp_data->adjfreq_ppb_shift - 1))) >>
			ptp_data->adjfreq_ppb_shift;
	...
	ptp_data->current_adjfreq = adjustment_ns;

Could the comment describe the unit correctly?

[Severity: Medium]
This is a pre-existing issue, but now that efx_phc_adjtime() relies only
on the cached value, can a concurrent adjtime and adjfine leave the cache
out of sync with firmware?

At this commit, nothing serializes these two accesses:

efx_phc_adjtime():
	MCDI_SET_QWORD(inbuf, PTP_IN_ADJUST_FREQ, ptp_data->current_adjfreq);

efx_phc_adjfine():
	rc = efx_mcdi_rpc(efx, MC_CMD_PTP, inadj, sizeof(inadj),
			  NULL, 0, NULL);
	...
	ptp_data->current_adjfreq = adjustment_ns;

posix_clock_adjtime() only takes the clock rwsem for read. So an adjtime
can read the old frequency F0 before an adjfine programs F1. Its RPC then
writes F0 back into firmware while the cache holds F1.

The next patch in the series, "sfc: ptp: serialize host and MC
synchronization transactions", appears to fix this. It adds
ptp_data->phc_lock, holds it around the RPC and the cache update in
efx_phc_adjfine(), and runs _efx_phc_adjtime() under the same lock.

>  
>  	return 0;
>  

To recap, these questions concern the Siena copy of efx_ptp_start(), the
@current_adjfreq kernel-doc, and the adjtime/adjfine race.

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

Re: [PATCH net v4 1/3] sfc: ptp: drop the stale frequency reset in efx_ptp_start()

[Austin, Alex (DCCG)]

On 30/10/2026 12:53, 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 1 potential
> issue(s) to consider.
>
> Critical: 0 · High: 0 · Medium: 1 (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 fix only changes the EF10/EF100 copy of efx_ptp_start() in
>   drivers/net/ethernet/sfc/ptp.c.
>
> Pre-existing issues:
> - [Medium] At this commit, efx_phc_adjtime() reads
>   ptp_data->current_adjfreq (drivers/net/ethernet/sfc/ptp.c:2129) and…
> - [Low] The kernel-doc for struct efx_ptp_data says `@current_adjfreq:
>   Current ppb adjustment.` (drivers/net/ethernet/sfc/ptp.c:272, and…
>
> --
>
> sfc: ptp: drop the stale frequency reset in efx_ptp_start()
>
> This commit stops efx_ptp_start() from clearing the cached
> current_adjfreq. Firmware keeps the clock frequency across PTP disable
> and enable. Clearing the cache made the next efx_phc_adjtime() set the
> NIC's frequency trim back to 0.
>
>> diff --git a/drivers/net/ethernet/sfc/ptp.c b/drivers/net/ethernet/sfc/ptp.c
>> index 00cfd8d5b46ea..957ca08092e7f 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;
> [Severity: Medium]
> Should the Siena copy of efx_ptp_start() get the same change? The Siena
> version still clears the cache right after efx_ptp_enable() succeeds:
>
> drivers/net/ethernet/sfc/siena/ptp.c:efx_ptp_start() {
>     ...
>         rc = efx_ptp_enable(efx);
>         if (rc != 0)
>                 goto fail;
>
>         ptp->evt_frag_idx = 0;
>         ptp->current_adjfreq = 0;
>     ...
> }
>
> Siena's efx_phc_adjtime() also sends the cached value with every time
> step:
>
>         MCDI_SET_QWORD(inbuf, PTP_IN_ADJUST_FREQ, ptp_data->current_adjfreq);
>
> On Siena, all the paths named in the commit message still reach that
> reset:
>
>   efx_siena_ptp_change_mode() -> efx_ptp_start()    (SIOCSHWTSTAMP)
>   efx_ptp_restart() -> efx_ptp_start()              (ifup, reset recovery)
>   efx_ptp_worker() -> efx_ptp_start()               (reset_required)
>
> So the next adjtime or settime would set the Siena NIC's frequency trim to
> 0. Meanwhile the servo and the PTP core still hold the old value.
>
> The Fixes: tag points at 7c236c43b838, which added this code for Siena
> hardware. The later patches in the series ("sfc: ptp: serialize host and
> MC synchronization transactions" and "sfc: ptp: avoid racing PPS
> timestamp updates") don't touch siena/ptp.c either.
>
> This assumes Siena MC firmware also keeps the frequency across
> MC_CMD_PTP_OP_DISABLE and MC_CMD_PTP_OP_ENABLE. The commit message only
> says this for the firmware it targets. Does Siena firmware behave the
> same way? If so, should drivers/net/ethernet/sfc/siena/ptp.c be fixed as
> well?
>
From inspection, it looks like the same issue exists 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.

[...]
> [Severity: Medium]
> This is a pre-existing issue, but now that efx_phc_adjtime() relies only
> on the cached value, can a concurrent adjtime and adjfine leave the cache
> out of sync with firmware?
>
> At this commit, nothing serializes these two accesses:
>
> efx_phc_adjtime():
>         MCDI_SET_QWORD(inbuf, PTP_IN_ADJUST_FREQ, ptp_data->current_adjfreq);
>
> efx_phc_adjfine():
>         rc = efx_mcdi_rpc(efx, MC_CMD_PTP, inadj, sizeof(inadj),
>                           NULL, 0, NULL);
>         ...
>         ptp_data->current_adjfreq = adjustment_ns;
>
> posix_clock_adjtime() only takes the clock rwsem for read. So an adjtime
> can read the old frequency F0 before an adjfine programs F1. Its RPC then
> writes F0 back into firmware while the cache holds F1.
>
> The next patch in the series, "sfc: ptp: serialize host and MC
> synchronization transactions", appears to fix this. It adds
> ptp_data->phc_lock, holds it around the RPC and the cache update in
> efx_phc_adjfine(), and runs _efx_phc_adjtime() under the same lock.


Yes, this is fixed in the next patch.

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

[PATCH net v4 2/3] 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 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 <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 | 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 2247e5e..eedab53 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;
 }
@@ -1569,6 +1585,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 +1657,7 @@ fail2:
 	efx_nic_free_buffer(efx, &ptp->start);
 
 fail1:
+	mutex_destroy(&ptp->phc_lock);
 	kfree(efx->ptp_data);
 	efx->ptp_data = NULL;
 
@@ -1694,6 +1712,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;
 }
@@ -2105,23 +2124,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 +2153,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 +2196,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

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

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] The new kernel-doc for `@phc_lock` in struct efx_ptp_data
  (drivers/net/ethernet/sfc/ptp.c:275-276) says the lock "Serializes PHC…

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but
  `_efx_phc_adjtime()` in drivers/net/ethernet/sfc/ptp.c (line 2144)…
- [Medium] Baseline decision: not a defect of this patch (pre-existing,
  Medium, in a driver the patch does not touch).
- [Low] Baseline decision: false positive, fixed later in the series.

--

sfc: ptp: serialize host and MC synchronization transactions

This adds a phc_lock mutex to struct efx_ptp_data. The mutex is held
across the whole efx_ptp_synchronize() transaction and around the PHC
adjfine/adjtime/settime operations, so the PPS worker and the hwtstamp
enable path no longer clobber each other's handshake flag and timesets.
It also bails out of efx_ptp_synchronize() when efx_mcdi_rpc_start()
fails.

> diff --git a/drivers/net/ethernet/sfc/ptp.c b/drivers/net/ethernet/sfc/ptp.c
> index 957ca08092e7f..e26269fae38d6 100644
> --- a/drivers/net/ethernet/sfc/ptp.c
> +++ b/drivers/net/ethernet/sfc/ptp.c

[ ... ]

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

[Severity: Low]
Does this comment claim more coverage than the code gives? Two of the
callbacks in the PHC ops table don't take phc_lock. efx_phc_gettime()
(.gettime64) sends MC_CMD_PTP_OP_READ_NIC_TIME, and efx_phc_enable()
(.enable) flips nic_ts_enabled. A standalone clock_gettime() on the PHC
therefore runs without phc_lock.

Making the comment true by adding the lock to efx_phc_gettime() would
self-deadlock, because efx_phc_settime() now calls it with phc_lock
already held:

	mutex_lock(&ptp_data->phc_lock);
	rc = efx_phc_gettime(ptp, &time_now);

Nothing breaks today, since the MCDI interface lock already serializes
the RPCs. Could the comment instead list what phc_lock actually covers
(synchronize, adjfine, adjtime and settime)?

>   * @phc_clock_info: Registration structure for phc device
>   * @pps_work: pps work task for handling pps events
>   * @pps_workwq: pps work queue

[ ... ]

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

[Severity: Medium]
This is a pre-existing issue, and it is not in a file this patch
touches. The Siena copy of this function still has the same pattern:

drivers/net/ethernet/sfc/siena/ptp.c:efx_ptp_synchronize() {
    ...
	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);
    ...
}

It takes no host lock around the handshake flag. When rpc_start fails
it still goes on to efx_siena_mcdi_rpc_finish(), and it processes
results into ptp->timeset[] after the MCDI interface has been released.
Both racing callers exist there as well: efx_ptp_pps_worker() and
efx_ptp_change_mode().

The Siena efx_phc_settime() also still does an unserialized gettime
followed by adjtime. current_adjfreq is written in efx_phc_adjfine() and
read in efx_phc_adjtime() with no lock.

Would it make sense to send a separate sfc/siena patch with the same
fix?

>  
>  	/* 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);

[Severity: Low]
This isn't a bug introduced by this patch, but ptp->host_time_pps is
still read outside phc_lock. efx_ptp_process_times() writes it under
the lock in several steps:

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

efx_ptp_pps_worker() then copies it after efx_ptp_synchronize() has
dropped the lock:

	ptp_evt.pps_times = ptp->host_time_pps;

Can a concurrent efx_ptp_synchronize() from efx_ptp_change_mode()
overwrite host_time_pps before the PPS worker reads it?

The next patch in the series, "sfc: ptp: avoid racing PPS timestamp
updates", appears to address this. It removes the shared host_time_pps
field and has efx_ptp_synchronize() return the timestamp through a
caller-provided struct pps_event_time. The PPS worker passes a stack
variable and efx_ptp_change_mode() passes NULL.

>  
>  	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);

[Severity: High]
This is a pre-existing issue, but should this be
ptp_data->ns_to_nic_time() rather than going back through
efx->ptp_data? The ptp_data argument comes from container_of() on the
registered phc_clock_info, so it is always valid here.

efx_realloc_channels() clears efx->ptp_data while it tears down the old
channels, and the PHC stays registered for that whole time:

drivers/net/ethernet/sfc/efx_channels.c:efx_realloc_channels() {
    ...
out:
	efx->ptp_data = NULL;
	/* Destroy unused channel structures */
	for (i = 0; i < efx->n_channels; i++) {
    ...
	efx->ptp_data = ptp_data;
    ...
}

A clock_settime() or clock_adjtime(ADJ_SETOFFSET) on the PHC that runs
at the same time as ethtool -G could then do:

efx_phc_settime() or efx_phc_adjtime()
  mutex_lock(&ptp_data->phc_lock)
  _efx_phc_adjtime()
    efx->ptp_data->ns_to_nic_time()    <-- efx->ptp_data is NULL

The PHC ioctl path does not take RTNL, and efx_realloc_channels() does
not take phc_lock, so nothing seems to stop the two from overlapping.

With this patch the oops now happens while phc_lock is held. Would that
leave the mutex locked for good? Every later efx_ptp_synchronize() call
would then block, both from the PPS worker and from
efx_ptp_change_mode(), which runs under RTNL for SIOCSHWTSTAMP.

Since this function is already being reworked here, using the ptp_data
argument would avoid the NULL dereference.

>  
> @@ -2133,6 +2152,18 @@ static int efx_phc_adjtime(struct ptp_clock_info *ptp, s64 delta)
>  			    NULL, 0, NULL);
>  }

[ ... ]

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

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

[Austin, Alex (DCCG)]

On 30/10/2026 12:53, 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 more severe
> issue(s) in other patches of this series. This mail is only an FYI, in case
> you want to address the nit picks below when you respin.
>
> Low priority / nit picks for your consideration:
>
> - [Low] The new kernel-doc for `@phc_lock` in struct efx_ptp_data
>   (drivers/net/ethernet/sfc/ptp.c:275-276) says the lock "Serializes PHC…
>
> Pre-existing issues:
> - [High] This problem wasn't introduced by this patch, but
>   `_efx_phc_adjtime()` in drivers/net/ethernet/sfc/ptp.c (line 2144)…
> - [Medium] Baseline decision: not a defect of this patch (pre-existing,
>   Medium, in a driver the patch does not touch).
> - [Low] Baseline decision: false positive, fixed later in the series.
>
> --
>
> sfc: ptp: serialize host and MC synchronization transactions
>
> This adds a phc_lock mutex to struct efx_ptp_data. The mutex is held
> across the whole efx_ptp_synchronize() transaction and around the PHC
> adjfine/adjtime/settime operations, so the PPS worker and the hwtstamp
> enable path no longer clobber each other's handshake flag and timesets.
> It also bails out of efx_ptp_synchronize() when efx_mcdi_rpc_start()
> fails.
>

[...]

> [Severity: Medium]
> This is a pre-existing issue, and it is not in a file this patch
> touches. The Siena copy of this function still has the same pattern:
>
> drivers/net/ethernet/sfc/siena/ptp.c:efx_ptp_synchronize() {
>     ...
>         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);
>     ...
> }
>
> It takes no host lock around the handshake flag. When rpc_start fails
> it still goes on to efx_siena_mcdi_rpc_finish(), and it processes
> results into ptp->timeset[] after the MCDI interface has been released.
> Both racing callers exist there as well: efx_ptp_pps_worker() and
> efx_ptp_change_mode().
>
> The Siena efx_phc_settime() also still does an unserialized gettime
> followed by adjtime. current_adjfreq is written in efx_phc_adjfine() and
> read in efx_phc_adjtime() with no lock.
>
> Would it make sense to send a separate sfc/siena patch with the same
> fix?


As before, I think we should tackle Siena issues in a different patchset.

[...]

> [Severity: High]
> This is a pre-existing issue, but should this be
> ptp_data->ns_to_nic_time() rather than going back through
> efx->ptp_data? The ptp_data argument comes from container_of() on the
> registered phc_clock_info, so it is always valid here.
>
> efx_realloc_channels() clears efx->ptp_data while it tears down the old
> channels, and the PHC stays registered for that whole time:
>
> drivers/net/ethernet/sfc/efx_channels.c:efx_realloc_channels() {
>     ...
> out:
>         efx->ptp_data = NULL;
>         /* Destroy unused channel structures */
>         for (i = 0; i < efx->n_channels; i++) {
>     ...
>         efx->ptp_data = ptp_data;
>     ...
> }
>
> A clock_settime() or clock_adjtime(ADJ_SETOFFSET) on the PHC that runs
> at the same time as ethtool -G could then do:
>
> efx_phc_settime() or efx_phc_adjtime()
>   mutex_lock(&ptp_data->phc_lock)
>   _efx_phc_adjtime()
>     efx->ptp_data->ns_to_nic_time()    <-- efx->ptp_data is NULL
>
> The PHC ioctl path does not take RTNL, and efx_realloc_channels() does
> not take phc_lock, so nothing seems to stop the two from overlapping.
>
> With this patch the oops now happens while phc_lock is held. Would that
> leave the mutex locked for good? Every later efx_ptp_synchronize() call
> would then block, both from the PPS worker and from
> efx_ptp_change_mode(), which runs under RTNL for SIOCSHWTSTAMP.
>
> Since this function is already being reworked here, using the ptp_data
> argument would avoid the NULL dereference.
>

Ed, are you happy if we tackle this in a follow up patch since it is a
pre-existing issue?

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

[PATCH net v4 3/3] 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 eedab53..3c6a85b 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
@@ -1501,14 +1505,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);
 }
 
@@ -1844,7 +1849,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 v4 3/3] 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] This patch removes the shared ptp->host_time_pps race from
  drivers/net/ethernet/sfc/ptp.c, but the same race is still present in…

--

sfc: ptp: avoid racing PPS timestamp updates

This patch removes the shared host_time_pps field from struct
efx_ptp_data in sfc/ptp.c. efx_ptp_synchronize() now returns the PPS
timestamp through storage owned by the caller. The PPS worker therefore
no longer copies a field that a concurrent SIOCSHWTSTAMP-driven
synchronization could be rewriting.

> diff --git a/drivers/net/ethernet/sfc/ptp.c b/drivers/net/ethernet/sfc/ptp.c
> index e26269fae38d6..37511f234d76d 100644
> --- a/drivers/net/ethernet/sfc/ptp.c
> +++ b/drivers/net/ethernet/sfc/ptp.c

[ ... ]

> @@ -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);
>  }

[Severity: Medium]
This fixes the race in sfc/ptp.c. Is the same race still present in the
Siena copy of this code, drivers/net/ethernet/sfc/siena/ptp.c? That file
also descends from the Fixes target 7c236c43b838, and this patch doesn't
touch it.

In siena/ptp.c, efx_ptp_process_times() still writes the shared field:

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);
    ...
}

The Siena efx_ptp_pps_worker() copies it after synchronization returns,
with no lock held:

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

Meanwhile, the SIOCSHWTSTAMP path can run another synchronization:

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

Can this give the partially updated PPS timestamp that the commit message
describes, but for Siena NICs?

Siena also looks more exposed than sfc was before this series. The Siena
efx_ptp_synchronize() takes no phc_lock or any other mutex. The
WRITE_ONCE(*start, 0) handshake runs before efx_siena_mcdi_rpc_start(),
and the ptp->timeset[] processing runs after efx_siena_mcdi_rpc_finish().
Could two concurrent synchronizations clobber each other's state there
too?

The earlier patch in this series, "sfc: ptp: serialize host and MC
synchronization transactions", seems to have handled that only for sfc.

Should the same caller-owned out-parameter change, along with the
serialization, also be applied to siena/ptp.c? If not, could the commit
message say why Siena is left out?

[ ... ]

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

Re: [PATCH net v4 3/3] sfc: ptp: avoid racing PPS timestamp updates

[Austin, Alex (DCCG)]

On 30/10/2026 12:53, 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 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] This patch removes the shared ptp->host_time_pps race from
>   drivers/net/ethernet/sfc/ptp.c, but the same race is still present in…
>
> --
>
> sfc: ptp: avoid racing PPS timestamp updates
>
> This patch removes the shared host_time_pps field from struct
> efx_ptp_data in sfc/ptp.c. efx_ptp_synchronize() now returns the PPS
> timestamp through storage owned by the caller. The PPS worker therefore
> no longer copies a field that a concurrent SIOCSHWTSTAMP-driven
> synchronization could be rewriting.
>
>> diff --git a/drivers/net/ethernet/sfc/ptp.c b/drivers/net/ethernet/sfc/ptp.c
>> index e26269fae38d6..37511f234d76d 100644
>> --- a/drivers/net/ethernet/sfc/ptp.c
>> +++ b/drivers/net/ethernet/sfc/ptp.c
> [ ... ]
>
>> @@ -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);
>>  }
> [Severity: Medium]
> This fixes the race in sfc/ptp.c. Is the same race still present in the
> Siena copy of this code, drivers/net/ethernet/sfc/siena/ptp.c? That file
> also descends from the Fixes target 7c236c43b838, and this patch doesn't
> touch it.
>
> In siena/ptp.c, efx_ptp_process_times() still writes the shared field:
>
> 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);
>     ...
> }
>
> The Siena efx_ptp_pps_worker() copies it after synchronization returns,
> with no lock held:
>
> drivers/net/ethernet/sfc/siena/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);
> }
>
> Meanwhile, the SIOCSHWTSTAMP path can run another synchronization:
>
> efx_siena_ptp_change_mode()
>     efx_ptp_synchronize(efx, PTP_SYNC_ATTEMPTS * 2)
>         efx_ptp_process_times()
>             ptp->host_time_pps = *last_time;
>
> Can this give the partially updated PPS timestamp that the commit message
> describes, but for Siena NICs?
>
> Siena also looks more exposed than sfc was before this series. The Siena
> efx_ptp_synchronize() takes no phc_lock or any other mutex. The
> WRITE_ONCE(*start, 0) handshake runs before efx_siena_mcdi_rpc_start(),
> and the ptp->timeset[] processing runs after efx_siena_mcdi_rpc_finish().
> Could two concurrent synchronizations clobber each other's state there
> too?
>
> The earlier patch in this series, "sfc: ptp: serialize host and MC
> synchronization transactions", seems to have handled that only for sfc.
>
> Should the same caller-owned out-parameter change, along with the
> serialization, also be applied to siena/ptp.c? If not, could the commit
> message say why Siena is left out?
>
> [ ... ]


As before, I think we should tackle Siena issues in a different patchset.

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