Re: [PATCH net-next v2 1/2] dpll: zl3073x: add channel ToD, phase step and TIE operations

[Jakub Kicinski]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
--- Patch [1]: [PATCH net-next v2 1/2] dpll: zl3073x: add channel ToD, phase step and TIE operations ---
commit 4829f83428b4a90e412f8bc573a84b3869b68edb
Author: Ivan Vecera <ivecera@xxxxxxxxxx>

dpll: zl3073x: add channel ToD, phase step and TIE operations

Add low-level DPLL channel operations for ToD read/write/adjust, output phase
step, delta frequency offset write and TIE write. These serve as building
blocks for the PTP clock callbacks.

> diff --git a/drivers/dpll/zl3073x/chan.c b/drivers/dpll/zl3073x/chan.c
> index 4ec2cf53dad46..53032e782fd4a 100644
> --- a/drivers/dpll/zl3073x/chan.c
> +++ b/drivers/dpll/zl3073x/chan.c
[ ... ]
> @@ -234,6 +240,307 @@ const struct zl3073x_chan *zl3073x_chan_state_get(struct zl3073x_dev *zldev,
[ ... ]
> +int zl3073x_chan_tod_read(struct zl3073x_dev *zldev, u8 ch,
> +			  bool next_hz, struct timespec64 *ts,
> +			  struct ptp_system_timestamp *sts)
> +{
[ ... ]
> +	ptp_read_system_prets(sts);
> +	rc = zl3073x_chan_tod_ctrl(zldev, ch, cmd);
> +	if (rc)
> +		return rc;
> +
> +	rc = zl3073x_chan_tod_ready_wait(zldev, ch);
> +	if (rc)
> +		return rc;
> +	ptp_read_system_postts(sts);

[Severity: High]
Does this artificially inflate the cross-timestamp window?

For accurate PTP cross-timestamping, the prets and postts must closely bound
the exact moment the hardware latches the timestamp. The postts is captured
after zl3073x_chan_tod_ready_wait(), which can poll the device over I2C/SPI
for up to 30ms.

Including this transaction and polling delay inside the timestamping bracket
introduces significant jitter, destroying the precision of the
cross-timestamp.

[ ... ]
> +int zl3073x_chan_tod_adjust(struct zl3073x_dev *zldev, u8 ch,
> +			    struct timespec64 delta)
> +{
[ ... ]
> +	/* Read current ToD to determine remaining margin */
> +	rc = zl3073x_chan_tod_read(zldev, ch, false, &ts_cur, NULL);
> +	if (rc)
> +		return rc;
> +
> +	/* If too close to (or past) the next rollover, wait it out */
> +	diff = timespec64_sub(ts_next, ts_cur);
> +	if (diff.tv_sec < 0 || (!diff.tv_sec && diff.tv_nsec < threshold_ns)) {
> +		if (!diff.tv_sec && diff.tv_nsec)
> +			fsleep((unsigned long)diff.tv_nsec / NSEC_PER_USEC + 1);
> +		ts_next.tv_sec++;
> +	}

[Severity: High]
Is this adjustment vulnerable to thread preemption?

The code assumes exactly one 1Hz rollover occurred between reading ts_next and
ts_cur, and simply increments ts_next.tv_sec by 1. If the thread is preempted
for multiple seconds between the reads or before writing, multiple rollovers
will have occurred.

Incrementing by just 1 would leave the target ToD in the past, and writing it
back could cause the DPLL ToD to abruptly jump backwards, breaking PTP
synchronization.

[ ... ]
> +int zl3073x_chan_df_offset_set(struct zl3073x_dev *zldev, u8 ch, s64 offset)
> +{
> +	int rc;
> +
> +	rc = zl3073x_write_u48(zldev, ZL_REG_DPLL_DF_OFFSET(ch), offset);
> +	if (!rc)
> +		zldev->chan[ch].df_offset = offset;

[Severity: Medium]
Can this lead to torn reads on 32-bit platforms?

The 64-bit chan->df_offset is updated locklessly here, and can be read
concurrently by the driver's background monitor thread via
zl3073x_chan_df_offset_get().

Since 64-bit reads and writes are not atomic on 32-bit architectures, a torn
read could result in incorrect frequency offset calculations and spurious
netlink notifications.

[ ... ]
> +int zl3073x_chan_tie_write(struct zl3073x_dev *zldev, u8 ch, s64 delta_ns)
> +{
[ ... ]
> +	/* Convert ns to TIE units (0.01 ps = 10^-14 s) */
> +	tie_data = delta_ns * 100000LL;
> +
> +	rc = zl3073x_write_u48(zldev, ZL_REG_DPLL_TIE_DATA(ch), tie_data);

[Severity: High]
Does this calculation overflow the 48-bit signed hardware register for large
positive phase adjustments?

There is no programmatic bounds check enforcing the +-1s limit on delta_ns.
A positive phase adjustment exceeding ~1.4 seconds will produce a tie_data
value that exceeds the maximum positive 48-bit signed integer.

This sets bit 47 to 1. Because the value still fits within an unsigned 48-bit
integer, it bypasses the bounds check in zl3073x_write_u48(). The hardware
treats bit 47 as a sign bit, which would cause a massive negative phase jump
instead of a positive adjustment.

[ ... ]
> diff --git a/drivers/dpll/zl3073x/core.h b/drivers/dpll/zl3073x/core.h
> index 78dc208f3eea2..2dc836d0c8f6b 100644
> --- a/drivers/dpll/zl3073x/core.h
> +++ b/drivers/dpll/zl3073x/core.h
> @@ -26,6 +26,10 @@ struct zl3073x_dpll;
>  #define ZL_POLL_HWREG_TIMEOUT_US	(50 * USEC_PER_MSEC)
>  #define ZL_POLL_MB_TIMEOUT_US		(30 * USEC_PER_MSEC)
>  #define ZL_POLL_PHASE_ERR_TIMEOUT_US	(50 * USEC_PER_MSEC)
> +#define ZL_POLL_PHASE_STEP_TIMEOUT_US	(3000 * USEC_PER_MSEC)
> +#define ZL_POLL_TIE_WR_TIMEOUT_US	(1000 * USEC_PER_MSEC)
> +#define ZL_POLL_TOD_RD_TIMEOUT_US	(30 * USEC_PER_MSEC)
> +#define ZL_POLL_TOD_WR_TIMEOUT_US	(1000 * USEC_PER_MSEC)

[Severity: High]
Will these multi-second timeouts flood the communication bus and stall the CPU?

These timeout values are passed to zl3073x_poll_zero_u8(), which uses
regmap_read_poll_timeout() with a hardcoded sleep interval of 10 microseconds.

A sleep of 10us typically translates to a busy-wait via udelay(10) or triggers
extremely rapid back-to-back I2C/SPI transactions. Looping like this for up to
3 seconds will completely flood the communication bus and cause a massive CPU
stall for the duration of the timeout.
-- 
pw-bot: cr