Re: [PATCH net-next v14 10/12] dpll: sit9531x: add support to adjust output phase

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 0 · Medium: 9 · 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] The reported phase value for an advance depends on history, so
  the same register contents can give two different uAPI values and two…
- [Medium] The 'owed flush' recovery tracked by @flush_pending cannot be
  reached by an identical retry, because the DPLL core drops that retry…
- [Medium] A profile delay that cannot be represented is reported as
  exactly +1 ms.
- [Medium] sit9531x_dpll_output_pin_phase_adjust_set() returns an error
  without a pin change notification, even after…
- [Medium] The flush is PLL-wide, but the record of a committed-but-
  unapplied delay is kept per output.
- [Medium] When a profile leaves an output with a delay beyond +/-1 ms,
  sit9531x_dev_state_fetch (core.c:3539-3540) caches it at probe as the…
- [Medium] sit9531x_output_phase_adjust_set() converts picoseconds into
  VCO cycles using the Fvco from sit9531x_get_fvco(), without the in-…
- [Medium] sit9531x_output_phase_adjust_set() treats every error from
  sit9531x_output_phase_flush() as proof that the delay was not applied,…
- [Medium] sit9531x_output_freq_set() writes the new DIVO first and the
  re-timed PRG_RST_DELAY bytes second, in the same PRG_CMD window.
- [Low] The kernel-doc of sit9531x_dpll_output_pin_phase_adjust_get()
  says it 'Returns what the delay registers hold, i.e. the value…
- [Low] core.h documents @flush_pending as 'the programmed delay is
  committed but the flush that applies it failed'.

--

dpll: sit9531x: add support to adjust output phase

This commit adds phase adjust support for sit9531x outputs. It programs
the per-output PRG_RST_DELAY coarse and fine delay registers and
advertises a +/-1 ms range at 1 ps granularity. Requests are folded into
one output period and advances are held as complementary delays. A rate
change re-times a programmed delay in the same programming window, and
the failure paths track stale caches and owed phase flushes per output.

> diff --git a/drivers/dpll/sit9531x/core.c b/drivers/dpll/sit9531x/core.c
> index 9aa0d327b6bf6..ffed2aaf8a345 100644
> --- a/drivers/dpll/sit9531x/core.c
> +++ b/drivers/dpll/sit9531x/core.c
> @@ -2267,11 +2267,431 @@ static int sit9531x_output_divo_read(struct sit9531x_dev *sitdev, u8 out_idx,

[ ... ]

> +static int sit9531x_output_phase_encode(u64 fvco, u64 divo, s32 phase_ps,
> +					u64 *coarse_out, u8 *fine_out,
> +					s32 *phase_adj)
> +{

[ ... ]

> +	if (phase_norm_ps < 0)
> +		*phase_adj = abs_ps ?
> +			     -(s32)min_t(u64, t_out_ps - abs_ps,
> +					  SIT9531X_OUT_PHASE_ADJ_MAX_PS) : 0;
> +	else
> +		*phase_adj = (s32)min_t(u64, abs_ps,
> +					 SIT9531X_OUT_PHASE_ADJ_MAX_PS);

[Severity: Medium]
Does the reported phase now depend on history rather than on what the
registers hold?

A negative request always caches the advance form here, whatever the
period. sit9531x_output_phase_read() runs at probe and on a phase_stale
read-back, and it only reports the advance form when the period is longer
than 1 ms:

    if (ps > SIT9531X_OUT_PHASE_ADJ_MAX_PS &&
        t_out_ps - ps <= SIT9531X_OUT_PHASE_ADJ_MAX_PS) {

Take a 10 MHz output from a 5 GHz VCO (divo = 500, T_out = 100000 ps). A
request of -100 ps encodes coarse = 499, fine = 3 (99890 ps) and caches
-110. After a driver reload or a stale read-back, the same registers
report +99890.

On a later rate change to 5 MHz, sit9531x_output_freq_set() re-encodes
whichever form is cached. With -110 cached it writes a 199890 ps delay.
With +99890 cached the bytes match, nothing is written, and the delay
stays at 99890 ps, which is a different edge.

The commit message says:

  "An advance is held as the complementary delay and reads back as an
  advance only where the period exceeds the advertised window; elsewhere
  the two describe the same edge and the delay form is reported."

Is that accurate? The advance form is reported after every successful
set, and the two forms only describe the same edge while the rate stays
fixed.

[ ... ]

> +int sit9531x_output_phase_read(struct sit9531x_dev *sitdev, u8 out_idx,
> +			       s32 *phase_ps, bool *clamped)
> +{

[ ... ]

> +	if (!rc) {
> +		u64 t_out_ps = mul_u64_u64_div_u64(divo, 1000000000000ULL,
> +						   fvco);
> +
> +		if (ps >= t_out_ps)
> +			ps -= t_out_ps;
> +		if (ps > SIT9531X_OUT_PHASE_ADJ_MAX_PS &&
> +		    t_out_ps - ps <= SIT9531X_OUT_PHASE_ADJ_MAX_PS) {
> +			*phase_ps = -(s32)(t_out_ps - ps);
> +			return 0;
> +		}
> +	}
> +	*clamped = ps > SIT9531X_OUT_PHASE_ADJ_MAX_PS;
> +	*phase_ps = (s32)min_t(u64, ps, SIT9531X_OUT_PHASE_ADJ_MAX_PS);

[Severity: Medium]
Can a request for exactly +1 ms become a silent no-op here?

Take a profile delay that is outside the window and whose complement is
outside it too, such as a 5 ms delay on a 1 PPS output. This reports
+1000000000 ps, and sit9531x_dev_state_fetch() seeds that value into
out->phase_adj.

That value is inside the advertised range. A user request for +1 ms
passes the range check and then hits this in dpll_pin_phase_adj_set():

	if (phase_adj == old_phase_adj)
		return 0;

Userspace is told the output now has a +1 ms delay, but the registers
still hold the profile's 5 ms delay.

[ ... ]

> @@ -2280,11 +2700,45 @@ int sit9531x_output_freq_set(struct sit9531x_dev *sitdev, u8 out_idx,

[ ... ]

>  	rc = sit9531x_prg_enter(sitdev);
>  	if (rc)
>  		return rc;
>  
>  	rc = sit9531x_output_divo_write(sitdev, out_idx, divo);
> +	if (!rc && retime)
> +		rc = sit9531x_output_phase_bytes_write(sitdev, out_idx,
> +						       new_bytes, old_bytes);

[Severity: Medium]
What happens to the new divider if sit9531x_output_phase_bytes_write()
fails here?

The rollback in sit9531x_output_phase_bytes_write() only restores the
delay bytes. The fully written DIVO stays in place, sit9531x_prg_commit()
makes it live, and the function returns an error without running
sit9531x_output_phase_flush().

The output then runs at the new rate with no realignment. An advance
still encoded as T_old - |adv| now counts against T_new.

The frequency getter reads DIVO back from hardware and reports the new
rate, so dpll_pin_freq_set() drops an identical retry:

	if (freq == old_freq)
		return 0;

This happens even though userspace was told the request failed. Before
this patch, a fully written divider was always followed by a commit and a
flush.

[ ... ]

> @@ -2374,27 +2866,140 @@ int sit9531x_output_freq_get(struct sit9531x_dev *sitdev, u8 out_idx,

[ ... ]

> +int sit9531x_output_phase_adjust_set(struct sit9531x_dev *sitdev,
> +				     u8 out_idx, s32 phase_ps)
> +{

[ ... ]

> +	rc = sit9531x_get_fvco(sitdev, pll_idx, &fvco);
> +	if (rc)
> +		return rc == -ENODATA ? -ENODEV : rc;

[Severity: Medium]
Should this refuse an out-of-band Fvco, the way
sit9531x_output_divo_calc() does?

sit9531x_get_fvco() returns success for a Fref * DIVN outside the VCO
band, and its own comment says a set refuses to program against such a
rate. sit9531x_output_divo_calc() follows that:

	if (!sit9531x_fvco_in_band(pll_idx, fvco)) {
		...
		return -EINVAL;
	}

Here the estimate goes straight into sit9531x_output_phase_encode(). On
such a PLL (one fed from another PLL, for example), the delay is
programmed with the wrong scale and reported as exact.

The read-back uses the same estimate, so the error would not show up.

[ ... ]

> +	if (!memcmp(old_bytes, new_bytes, sizeof(new_bytes)) &&
> +	    !sitdev->out[out_idx].phase_stale) {
> +		if (!sitdev->out[out_idx].flush_pending)
> +			goto cache;
> +		goto flush;
> +	}

[Severity: Medium]
Can an identical retry ever reach this flush_pending path?

dpll_pin_phase_adj_set() calls phase_adjust_get() first, and returns
without calling the driver when the two values match:

	if (phase_adj == old_phase_adj)
		return 0;

dpll_pin_freq_set() does the same for the rate. Here are the paths that
set flush_pending.

When sit9531x_output_freq_set() hits a flush failure with retime set, it
stores the committed value in the cache:

		sitdev->out[out_idx].flush_pending = true;
		if (retime) {
			sitdev->out[out_idx].phase_adj = phase_adj;

The getter then publishes the committed value, and a retry of it is
dropped. The comment next to that code argues the opposite.

When that flush fails without retime, the cache already matches the
registers, so a retry is dropped as well.

A commit failure in either setter sets phase_stale and flush_pending. The
read-back in sit9531x_dpll_output_pin_phase_adjust_get() then publishes
whatever landed and clears phase_stale, so an identical retry is dropped
again.

The only request that seems to reach goto flush is one that differs from
the reported value but quantizes to the same bytes, say 1 ps off. Until
then the output runs unaligned while the pin reports success.

This seems to contradict the @flush_pending kernel-doc ("the next request
for that delay runs the flush although the registers already hold it").
It also contradicts the commit message:

  "a flush that failed after the delay was committed is retried by the
  next request"

[ ... ]

> +flush:
> +	/*
> +	 * Restart the output divider phase so the freshly programmed delay is
> +	 * applied against a known edge instead of the divider's arbitrary
> +	 * running phase.
> +	 */
> +	rc = sit9531x_output_phase_flush(sitdev, pll_idx);
> +	if (rc) {
> +		/*
> +		 * The delay is committed but the output keeps the phase it
> +		 * had, so the cache is left as it is: it still says what the
> +		 * output realizes.  Record the flush that is owed instead.

[ ... ]

> +		sitdev->out[out_idx].flush_pending = true;
> +		return rc;
> +	}

[Severity: Medium]
Does every error from sit9531x_output_phase_flush() mean the delay was not
applied?

The flush can write DIVO_PHASE_TRIG successfully, which latches the
realignment, and then fail while restoring GPIO_FUNC_CTRL1, while
disarming, or while unparking the other PLLs:

	ret = sit9531x_write_u8(sitdev, SIT9531X_REG_GPIO_FUNC_CTRL1, orig);
	if (ret && !rc)
		rc = ret;

In that case the output already runs the new delay X. The getter still
reports the old value Y, and phase_stale stays false.

A request for Y is then dropped by the core. A request for X takes the
flush_pending path and fires another PLL-wide restart.

> +	sitdev->out[out_idx].flush_pending = false;
> +	sitdev->out[out_idx].phase_stale = false;

[Severity: Medium]
The flush restarts every output divider of the PLL, but this bookkeeping,
and the bookkeeping in sit9531x_output_freq_set(), is per output.

What happens to a delay on output A that was committed but not applied,
when a later flush on the same PLL succeeds?

After the flush failure above, out[A] still holds Y with only
flush_pending set, while the registers hold X. Any later successful flush
of that PLL applies X. That flush can come from:

  a rate change on A with phase_armed false. Retime is false, so only
  flush_pending is cleared and the cache stays at Y.

  a rate or phase change on a sibling output B, which only updates out[B].

Either way the getter keeps reporting Y. A request to go back to Y,
including 0 to clear the delay, is dropped by the core, and the output
stays at X.

In the sibling case out[A].flush_pending is never cleared. A later
matching request on A then fires an unneeded PLL-wide restart. That is an
edge jump on every output of the PLL, or a loss of lock without PHFL_EN.

The mirror case looks similar. If the flush fails on a clear (X = 0),
phase_armed stays true with Y cached. The next rate change re-encodes Y
and undoes the committed clear.

[ ... ]

> @@ -2910,6 +3518,32 @@ static int sit9531x_dev_state_fetch(struct sit9531x_dev *sitdev)

[ ... ]

> +		mutex_lock(&sitdev->multiop_lock);
> +		rc = sit9531x_output_phase_read(sitdev, i, &phase_ps, &clamped);
> +		mutex_unlock(&sitdev->multiop_lock);
> +		if (!rc) {
> +			sitdev->out[i].phase_adj = phase_ps;
> +			sitdev->out[i].phase_armed = !clamped && phase_ps;

[Severity: Medium]
For a clamped profile delay, what keeps phase_adj in sync with the
registers after a rate change?

With phase_armed false, sit9531x_output_freq_set() skips the re-time
block. It refreshes the cache only when retime is set, and it never sets
phase_stale for this output.

sit9531x_output_phase_read() folds the raw coarse count modulo the current
DIVO. After a successful rate change the same registers can therefore
describe a different delay, possibly one inside the window. The getter
keeps returning the clamped 1 ms figure.

That breaks the @phase_adj description ("what the delay registers
actually realize") and the @phase_stale description ("the programmed delay
may differ from @phase_adj"). A user request equal to the stale value is
also dropped by the core.

[ ... ]

> diff --git a/drivers/dpll/sit9531x/core.h b/drivers/dpll/sit9531x/core.h
> index 1464ba0b66142..0c42bfc05b0b2 100644
> --- a/drivers/dpll/sit9531x/core.h
> +++ b/drivers/dpll/sit9531x/core.h
> @@ -104,6 +106,18 @@ struct sit9531x_ref {

[ ... ]

> + * @flush_pending:	the programmed delay is committed but the flush
> + *			that applies it failed, so the output still realizes
> + *			@phase_adj; the next request for that delay runs the
> + *			flush although the registers already hold it

[Severity: Low]
This may only be a documentation issue, but is this description narrower
than how the flag is used?

sit9531x_output_freq_set() sets flush_pending on both its commit-failure
and flush-failure paths, whatever phase_armed and retime are. That
includes plain rate changes on outputs with no delay at all:

		if (sitdev->out[out_idx].phase_armed)
			sitdev->out[out_idx].phase_stale = true;
		sitdev->out[out_idx].flush_pending = true;

sit9531x_output_phase_adjust_set() then treats the flag as meaning a flush
is owed, so a later phase request whose bytes already match fires a
PLL-wide flush.

Should the field description also cover an owed divider realignment?

[ ... ]

> diff --git a/drivers/dpll/sit9531x/dpll.c b/drivers/dpll/sit9531x/dpll.c
> index f6e76318a567a..b3719370901eb 100644
> --- a/drivers/dpll/sit9531x/dpll.c
> +++ b/drivers/dpll/sit9531x/dpll.c
> @@ -959,12 +959,92 @@ sit9531x_dpll_output_pin_state_on_dpll_set(const struct dpll_pin *pin,
>  	return rc;
>  }
>  
> +/*
> + * sit9531x_dpll_output_pin_phase_adjust_get - read output phase adjustment
> + *
> + * Returns what the delay registers hold, i.e. the value
> + * sit9531x_output_phase_adjust_set() programmed after quantization,
> + * read from the cache unless a failed request left it unconfirmed.
> + */

[Severity: Low]
Is this accurate after a flush failure?

In that case sit9531x_output_phase_adjust_set() deliberately keeps
phase_adj at the previous value ("the cache is left as it is"), while the
registers hold the new delay. The getter then returns a value the
registers do not hold.

[ ... ]

> +	mutex_lock(&sitdev->multiop_lock);
> +	rc = sit9531x_output_phase_adjust_set(sitdev, dpin->id, phase_adjust);
> +	mutex_unlock(&sitdev->multiop_lock);
> +
> +	if (rc) {
> +		NL_SET_ERR_MSG(extack, "Phase adjust failed");
> +		return rc;
> +	}

[Severity: Medium]
Should this path send a pin change notification when the reported value
may have moved?

When sit9531x_output_phase_adjust_set() fails after its writes reached
the device, it sets phase_stale, and the next getter call can report a
different value.

The core only notifies on success, and the poll does not watch the delay
registers, so subscribers keep the old value.

sit9531x_dpll_intsync_src_state_on_dpll_set() and the output state
setter handle the same situation with a notification:

		dpll_pin = READ_ONCE(dpin->dpll_pin);
		if (moved && dpll_pin)
			__dpll_pin_change_ntf(dpll_pin);

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261016005823.13690-1-arouhi%40sitime.com