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

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

Critical: 0 · High: 0 · Medium: 2 · 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] Incomplete fix: the Siena sibling driver is left with the same
  bugs.
- [Medium] efx_ptp_start() sends MC_CMD_PTP_OP_ENABLE through
  efx_ptp_enable() (line 1460) with no lock held.
- [Low] Resolved later in the series (baseline decision: false).
- [Low] The new kernel-doc for @phc_lock in struct efx_ptp_data (lines
  276-277) says the lock 'Serializes PHC commands and synchronization,…

--

sfc: ptp: serialize host and MC synchronization transactions

This patch adds a phc_lock mutex to struct efx_ptp_data. The lock is held
across efx_ptp_synchronize(), the PHC adjfine/adjtime/settime callbacks,
and the current_adjfreq reset in efx_ptp_start(). efx_ptp_synchronize()
also now bails out when efx_mcdi_rpc_start() fails, instead of finishing
an RPC that was never acquired.

[ ... ]

[Severity: Low]
Does this description cover more than the code does?

efx_phc_adjfine(), efx_phc_adjtime() and efx_phc_settime() take
phc_lock. efx_phc_gettime(), which is the gettime64 callback, still sends
MC_CMD_PTP_OP_READ_NIC_TIME without it when the PTP core calls it
directly:

drivers/net/ethernet/sfc/ptp.c:efx_phc_gettime() {
    ...
	rc = efx_mcdi_rpc(efx, MC_CMD_PTP, inbuf, sizeof(inbuf),
			  outbuf, sizeof(outbuf), NULL);
    ...
}

efx_phc_enable() also updates its state without the lock:

	ptp_data->nic_ts_enabled = !!enable;

Could the comment be limited to the operations that actually take
phc_lock?

[ ... ]

[Severity: Medium]
Does drivers/net/ethernet/sfc/siena/ptp.c need the same fix? It has the
same code, and every problem fixed here is still present there.

In siena, efx_ptp_synchronize() clears the start flag without a lock. If
the start fails, it still goes on to efx_siena_mcdi_rpc_finish():

drivers/net/ethernet/sfc/siena/ptp.c:efx_ptp_synchronize() {
    ...
	/* Clear flag that signals MC ready */
	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);
    ...
}

efx_siena_mcdi_rpc_start() can return before efx_mcdi_acquire_sync()
through efx_mcdi_check_supported(), mc_bist_for_other_fn or
MCDI_MODE_FAIL.

The siena driver also has the same two concurrent callers,
efx_ptp_pps_worker() and efx_ptp_change_mode(). Its efx_phc_adjfine() and
efx_ptp_start() write current_adjfreq without a lock. Its
efx_phc_settime() still does gettime and adjtime as two separate steps.

The Fixes: tag points at 7c236c43b838 ("sfc: Add support for IEEE-1588
PTP"), the original code that siena/ptp.c was forked from. The commit
message does not mention Siena. Should there be a companion patch for
sfc-siena, or a note on why Siena is not affected?

[Severity: Low]
After this unlock, efx_ptp_pps_worker() reads the shared synchronization
result without holding phc_lock:

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

Can a concurrent efx_ptp_change_mode()->efx_ptp_synchronize() rewrite
ptp->host_time_pps in efx_ptp_process_times() during that read?

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

If so, ptp_clock_event() would get a torn or foreign timestamp.

The next patch in the series, "sfc: ptp: avoid racing PPS timestamp
updates", removes host_time_pps. It copies the result into a stack-local
pps_event_time under phc_lock, so the unlocked read is gone by the end of
the series.

[ ... ]

[Severity: Medium]
Can a frequency adjustment still be lost here?

Just above this, efx_ptp_start() sends MC_CMD_PTP_OP_ENABLE without
holding any lock:

	rc = efx_ptp_enable(efx);
	if (rc != 0)
		goto fail;

The firmware enable and the cache reset are therefore not one critical
section, and efx_phc_adjfine() can run completely between them:

CPU0 (efx_ptp_start)            CPU1 (clock_adjtime ADJ_FREQUENCY)
efx_ptp_enable() completes
                                efx_phc_adjfine()
                                  mutex_lock(&ptp_data->phc_lock)
                                  MCDI ADJUST with FREQ=X
                                  current_adjfreq = X
                                  mutex_unlock(&ptp_data->phc_lock)
mutex_lock(&ptp->phc_lock)
current_adjfreq = 0
mutex_unlock(&ptp->phc_lock)

The next adjtime or settime goes through _efx_phc_adjtime(), which
programs the frequency from the cached value:

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

That would put the MC back at frequency 0 and drop a correction that
adjfine already reported as applied.

The commit message says current_adjfreq "cannot be torn by the reset in
efx_ptp_start()". The patch prevents tearing, but the cached value can
still get out of sync with the hardware. Would phc_lock need to be held
across both efx_ptp_enable() and the reset?

Also, efx_ptp_enable() treats -EALREADY as success. In that case the
firmware probably did not reset its frequency, but the driver still sets
current_adjfreq to 0. Is that intended?

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