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

[Jakub Kicinski <kuba@kernel.org>]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
dpll: zl3073x: add channel ToD, phase step and TIE operations

Adds low-level DPLL channel helpers for ToD read/write/adjust, output phase
step, delta frequency offset write and TIE write, plus a device-global
output step-time mask.  These are used by the PTP clock callbacks added in
the following patch.

> diff --git a/drivers/dpll/zl3073x/chan.c b/drivers/dpll/zl3073x/chan.c
> index 4ec2cf53dad46..ba4d303d41b47 100644
> --- a/drivers/dpll/zl3073x/chan.c
> +++ b/drivers/dpll/zl3073x/chan.c

[ ... ]

> @@ -234,6 +235,322 @@ const struct zl3073x_chan *zl3073x_chan_state_get(struct zl3073x_dev *zldev,
>  	return &zldev->chan[index];
>  }
>  
> +/**
> + * zl3073x_chan_tod_ready_wait - wait for ToD semaphore to clear
> + * @zldev: pointer to zl3073x device
> + * @ch: DPLL channel index
> + *
> + * Checks the ToD control register semaphore bit. If clear, returns
> + * immediately. Otherwise polls until the bit is cleared by the device.
> + *
> + * Return:
> + * * 0			- success
> + * * %-EBUSY		- timeout
> + * * %-EOPNOTSUPP	- unknown command detected
> + * * negative		- other error
> + */
> +int zl3073x_chan_tod_ready_wait(struct zl3073x_dev *zldev, u8 ch)
> +{
> +	unsigned int timeout;
> +	u8 tod_ctrl;
> +	int rc;
> +
> +	rc = zl3073x_read_u8(zldev, ZL_REG_DPLL_TOD_CTRL(ch), &tod_ctrl);
> +	if (rc)
> +		return rc;
> +
> +	if (!(tod_ctrl & ZL_DPLL_TOD_CTRL_SEM))
> +		return 0;

[ ... ]

> +int zl3073x_chan_tod_adjust(struct zl3073x_dev *zldev, u8 ch,
> +			    struct timespec64 delta)
> +{
> +#define ZL_TOD_MAX_RETRIES	20
> +	static const long threshold_ns = 20 * NSEC_PER_MSEC;
> +	struct timespec64 ts_next, ts_cur, diff;
> +	int rc, i;
> +
> +	for (i = 0; i < ZL_TOD_MAX_RETRIES; i++) {
> +		rc = zl3073x_chan_tod_read(zldev, ch, true, &ts_next, NULL);
> +		if (rc)
> +			return rc;
> +
> +		rc = zl3073x_chan_tod_read(zldev, ch, false, &ts_cur, NULL);
> +		if (rc)
> +			return rc;
> +
> +		/* Ensure the 1 Hz tick did not cross between the two reads
> +		 * and that enough margin remains to complete the write.
> +		 */
> +		diff = timespec64_sub(ts_next, ts_cur);
> +		if (diff.tv_sec > 0 ||
> +		    (!diff.tv_sec && diff.tv_nsec >= threshold_ns))
> +			break;
> +	}
> +	if (i == ZL_TOD_MAX_RETRIES) {
> +		dev_warn(zldev->dev,
> +			 "DPLL%u ToD adjust failed to get stable margin\n",
> +			 ch);
> +		return -EBUSY;
> +	}

[Severity: Medium]

Can this loop spin out its retry budget without ever giving the condition a
chance to become true?

The value being tested, diff = ts_next - ts_cur, is the time remaining until
the next 1 Hz edge.  When the call lands in the last 20 ms of a second, the
loop can only succeed once that residual has elapsed, but nothing in the
loop body waits.  zl3073x_chan_tod_ready_wait() returns straight away when
the semaphore is already clear:

	if (!(tod_ctrl & ZL_DPLL_TOD_CTRL_SEM))
		return 0;

and regmap_read_poll_timeout() inside zl3073x_poll_zero_u8() does its first
read before any usleep_range().  On a fast bus where the ToD command
completes before the semaphore is re-read, all 20 iterations can finish well
inside the 20 ms window, so i == ZL_TOD_MAX_RETRIES and the adjustment is
rejected with -EBUSY plus an unratelimited dev_warn() rather than being
retried a few milliseconds later.

In the next patch this propagates directly to userspace:

drivers/dpll/zl3073x/dpll.c:zl3073x_dpll_ptp_adjtime() {
	...
	return zl3073x_chan_tod_adjust(zldev, zldpll->id,
				       ns_to_timespec64(delta));
}

so clock_adjtime(ADJ_SETOFFSET) sees the -EBUSY.

Would it make sense to sleep for the remaining margin (or a fixed few
milliseconds) between iterations, so the loop actually waits out the
rollover window instead of burning the retries inside it?

[ ... ]

> +int zl3073x_chan_tie_write(struct zl3073x_dev *zldev, u8 ch, s64 delta_ns)
> +{
> +	s64 tie_data;
> +	int rc;
> +
> +	guard(mutex)(&zldev->tie_lock);
> +
> +	/* Wait for any previous TIE operation to complete */
> +	rc = zl3073x_poll_zero_u8(zldev, ZL_REG_DPLL_TIE_CTRL,
> +				  ZL_DPLL_TIE_CTRL_OP,
> +				  ZL_POLL_TIE_WR_TIMEOUT_US);
> +	if (rc)
> +		return rc;
> +
> +	/* 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);
> +	if (rc)
> +		return rc;

[Severity: Low]

This isn't a bug with the current callers, but the kerneldoc for
zl3073x_chan_tie_write() states delta_ns "must be in (-1s, 1s)" and nothing
in the function enforces it.  For |delta_ns| >= about 1.4e9 the product
overflows the signed 48-bit TIE field, and zl3073x_write_u48() accepts the
union of the signed and unsigned 48-bit ranges:

drivers/dpll/zl3073x/core.c:zl3073x_write_u48() {
	if (val > GENMASK_ULL(47, 0) && val < GENMASK_ULL(63, 47)) {
		dev_err(zldev->dev, "Value 0x%0llx out of range\n", val);
		return -EINVAL;
	}
	...
}

so a positive tie_data in [2^47, 2^48) would be programmed with the sign bit
set.

Both in-tree callers do bound the input: zl3073x_dpll_ptp_adjphase() is
gated by ptp_clock_adjtime(), which rejects offsets beyond
getmaxphase() == NSEC_PER_SEC - 1, and zl3073x_dpll_ptp_adjtime() passes
only the sub-second remainder from div_s64_rem(delta, NSEC_PER_SEC, ...).

Would an explicit range check on delta_ns be worth adding here so the
documented precondition is enforced by the helper itself?
-- 
pw-bot: cr