Re: [PATCH net-next v15 11/13] dpll: sit9531x: add support to adjust output phase

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential
issues to consider:
- [Medium] dpll: sit9531x: phase flush into open loops in
  sit9531x_output_phase_adjust_set()
- [Medium] dpll: sit9531x: broken beyond and retime_pending state
  tracking on freq change
- [Medium] dpll: sit9531x: misordered phase flush undo and loop commit
  in sit9531x_dev_stop()
--

commit b81ddf969266a9670cf4d33bb4abc4443cce505f
Author: Oleg Zadorozhnyi <Oleg.Zadorozhnyi@devoxsoftware.com>

dpll: sit9531x: add support to adjust output phase

This commit adds output pin phase adjustment support to the sit9531x DPLL
driver, programming coarse VCO-cycle and fine 30 ps delay registers,
triggering PLL output phase flushes, and re-timing delays on rate changes.

> diff --git a/drivers/dpll/sit9531x/core.c b/drivers/dpll/sit9531x/core.c
> index 458de502bc8f7..9896d5aa40f56 100644
> --- a/drivers/dpll/sit9531x/core.c
> +++ b/drivers/dpll/sit9531x/core.c
[ ... ]
> @@ -1829,28 +2241,117 @@ int sit9531x_output_freq_set(struct sit9531x_dev *sitdev, u8 out_idx,
>  	if (rc)
>  		return rc;
>  
> +	if (out->phase_stale && !out->retime_pending) {
> +		s64 phase_ps;
> +
> +		rc = sit9531x_output_phase_read(sitdev, out_idx, &phase_ps,
> +						&beyond);
> +		if (rc)
> +			return rc;
> +		out->phase_adj = phase_ps;
> +		out->phase_armed = !beyond && phase_ps;
> +		out->phase_stale = false;
> +	}
> +
> +	/*
> +	 * The reset delay counts VCO cycles; an advance is stored as T_out -
> +	 * |advance| and must be re-encoded for the new period. Write it in the
> +	 * divider's programming window; a second flush would restart the PLL's
> +	 * dividers.
> +	 */
> +	rc = sit9531x_output_phase_bytes_read(sitdev, out_idx, old_bytes);
> +	if (rc)
> +		return rc;
> +	sit9531x_output_phase_bytes_decode(old_bytes, &coarse, &fine);
> +	beyond = !out->phase_armed && out->phase_adj;
> +	if (out->retime_pending || out->phase_armed ||
> +	    ((coarse || fine) && !beyond)) {
> +		held = out->retime_pending ? out->retime_ps : out->phase_adj;
> +
> +		rc = sit9531x_output_phase_encode(fvco, divo, held, &coarse,
> +						  &fine, &phase_adj);
> +		if (rc)
> +			return rc;
> +		encoded = true;
> +		sit9531x_output_phase_bytes_build(old_bytes, coarse, fine,
> +						  new_bytes);
> +		retime = memcmp(old_bytes, new_bytes, sizeof(new_bytes)) != 0;
> +	}
[ ... ]
>  	ret = sit9531x_prg_commit(sitdev);
>  	if (ret && !rc)
>  		rc = ret;
> -	if (rc)
> +	if (rc) {
> +		if (bytes_in || (encoded && !retime && divo_in)) {
> +			out->phase_adj = phase_adj;
> +			out->phase_armed = phase_adj != 0;
> +			out->phase_stale = false;
> +			out->retime_pending = false;
> +		} else if (retime && landed) {
> +			out->retime_ps = held;
> +			out->retime_pending = true;
> +			out->phase_stale = true;
> +		} else if (encoded && landed) {
> +			out->phase_stale = true;
> +		} else if (beyond) {
> +			out->phase_stale = true;
> +		}
> +		if (landed)
> +			chan->flush_pending = true;
>  		return rc;
> +	}
[ ... ]
> +	if (encoded) {
> +		out->phase_adj = phase_adj;
> +		out->phase_armed = phase_adj != 0;
> +		out->phase_stale = false;
>  	}
>  
> -	sitdev->out[out_idx].freq = div64_u64(fvco, divo);
> +	/*
> +	 * A delay beyond the range was not re-timed and now decodes against the
> +	 * new divider.
> +	 */
> +	if (beyond)
> +		out->phase_stale = true;

[Severity: Medium]
Can out->retime_pending and beyond both become true here, causing
sit9531x_output_freq_set() to mark a newly re-timed delay stale?

If DIVO lands in sit9531x_output_freq_set() but writing the re-timed delay
bytes fails, the error path sets out->retime_pending = true and
out->phase_stale = true, and sit9531x_dpll_output_pin_frequency_set() sends
a change notification. Because sit9531x_dpll_output_pin_phase_adjust_get()
does not check out->retime_pending:

drivers/dpll/sit9531x/dpll.c:sit9531x_dpll_output_pin_phase_adjust_get() {
	...
	if (sitdev->out[dpin->id].phase_stale) {
		bool beyond;
		s64 phase_ps;

		rc = sit9531x_output_phase_read(sitdev, dpin->id, &phase_ps,
						&beyond);
		...
		sitdev->out[dpin->id].phase_adj = phase_ps;
		sitdev->out[dpin->id].phase_armed = !beyond && phase_ps;
		sitdev->out[dpin->id].phase_stale = false;
	}
}

it reads back the un-retimed delay against the new divider while
out->retime_pending is still set. If that readback sets beyond = true (so
out->phase_armed is false and out->phase_adj > 0), a subsequent
sit9531x_output_freq_set() call computes beyond = true while also entering
the out->retime_pending branch (encoded = true). At the end of
sit9531x_output_freq_set(), "if (beyond)" overwrites out->phase_stale = true
after the re-timed delay was just programmed, so the next getter readback
decodes a negative advance from hardware as a positive delay whenever
T_out - |advance| exceeds 1 ms.

Also, because sit9531x_dpll_output_pin_phase_adjust_get() overwrites
out->phase_adj and clears out->phase_stale while out->retime_pending is
true, a user request for that reported phase_adj is treated as a no-op by
dpll_pin_phase_adj_set() and then overwritten with out->retime_ps when
sit9531x_retime_retry() runs on the next poll.

Finally, in the error path of sit9531x_output_freq_set(), "else if (beyond)"
sets out->phase_stale = true without checking landed, causing
sit9531x_dpll_output_pin_frequency_set() to see moved == true and send a
change notification even when the divider register was not modified.

[ ... ]
> @@ -1893,20 +2394,130 @@ int sit9531x_output_freq_get(struct sit9531x_dev *sitdev, u8 out_idx,
[ ... ]
> +	/*
> +	 * Skip the write if the quantized bytes are unchanged; a rewrite
> +	 * restarts the divider phase of every output on the PLL.
> +	 */
> +	same = !memcmp(old_bytes, new_bytes, sizeof(new_bytes)) &&
> +	       !out->phase_stale;
> +	if (same && !chan->flush_pending) {
> +		rc = 0;
> +		goto cache;
> +	}
> +
> +	if (!same) {
> +		/*
> +		 * Output-system registers take effect only inside PRG_CMD,
> +		 * committed to the NVM shadow.
> +		 */
> +		rc = sit9531x_prg_enter(sitdev);
> +		if (rc)
> +			return rc;
> +
> +		rc = sit9531x_output_phase_bytes_write(sitdev, out_idx,
> +						       new_bytes, old_bytes,
> +						       &bytes_back);
> +		bytes_in = !rc;
> +
> +		ret = sit9531x_prg_commit(sitdev);
> +		if (ret && !rc)
> +			rc = ret;
> +		if (rc) {
> +			if (bytes_in) {
> +				out->phase_adj = phase_adj;
> +				out->phase_armed = phase_adj != 0;
> +				out->phase_stale = false;
> +				out->retime_pending = false;
> +				chan->flush_pending = true;
> +			} else if (!bytes_back) {
> +				out->phase_stale = true;
> +				chan->flush_pending = true;
> +			}
> +			return rc;
> +		}
> +	}
> +
> +	/*
> +	 * Restart the divider phase so the delay applies against a known edge.
> +	 */
> +	rc = sit9531x_output_phase_flush(sitdev, pll_idx);

[Severity: Medium]
Can sit9531x_output_phase_adjust_set() fire sit9531x_output_phase_flush()
while sitdev->loops_open is still true?

If writing new_bytes succeeds (bytes_in is true) but sit9531x_prg_commit()
fails to lock the loops, sitdev->loops_open stays true, out->phase_stale is
set to false, and chan->flush_pending is set to true. If
sit9531x_output_phase_adjust_set() is called again before the poll worker
runs and the request quantizes to the same register bytes (or targets
another output on the same PLL whose bytes match), same is true and
chan->flush_pending is true. The "!same" block that calls
sit9531x_prg_commit() is skipped, and sit9531x_output_phase_flush() runs
into open loops and clears chan->flush_pending. As noted in
sit9531x_dev_chan_states_update(), a flush pulse sent into open loops may be
lost when they re-lock, and because chan->flush_pending was cleared, the
poll worker will not retry the flush after locking the loops.

[ ... ]
> @@ -2925,15 +3596,46 @@ static void sit9531x_dev_stop(struct sit9531x_dev *sitdev)
>  			dev_warn(sitdev->dev,
>  				 "PLL%c left in forced holdover\n", 'A' + i);
>  	}
> +
> +	sit9531x_phase_flush_undo_retry(sitdev);
> +	if (sitdev->flush_ctrl_pending)
> +		dev_warn(sitdev->dev,
> +			 "phase flush trigger source select not put back\n");
> +	for (i = 0; i < SIT9531X_NUM_PLLS; i++) {
> +		if (!(sitdev->flush_arm_pending & BIT(i)))
> +			continue;
> +		dev_warn(sitdev->dev, "PLL%c: on-demand phase flush left %s\n",
> +			 'A' + i,
> +			 (sitdev->flush_armed & BIT(i)) ? "parked" : "armed");
> +	}
>  	mutex_unlock(&sitdev->multiop_lock);
>  
> +	/*
> +	 * Re-time first, since it re-locks the loops; the flush runs only into
> +	 * locked loops.
> +	 */
>  	mutex_lock(&sitdev->multiop_lock);
> +	for (i = 0; i < sitdev->info->num_outputs; i++) {
> +		sit9531x_retime_retry(sitdev, i);
> +		if (sitdev->out[i].retime_pending)
> +			dev_warn(sitdev->dev,
> +				 "out%u: delay left not re-timed to its rate\n",
> +				 i);
> +	}
>  	if (sitdev->loops_open) {
>  		sit9531x_prg_commit(sitdev);
>  		if (sitdev->loops_open)
>  			dev_warn(sitdev->dev,
>  				 "output loops left unlocked at removal\n");
>  	}
> +	for (i = 0; i < SIT9531X_NUM_PLLS; i++) {
> +		if (!sitdev->loops_open)
> +			sit9531x_flush_retry(sitdev, i);
> +		if (sitdev->chan[i].flush_pending)
> +			dev_warn(sitdev->dev,
> +				 "PLL%c: output dividers left unaligned\n",
> +				 'A' + i);
> +	}
>  	mutex_unlock(&sitdev->multiop_lock);
>  }

[Severity: Medium]
Should sit9531x_phase_flush_undo_retry() and its warnings run after
sit9531x_retime_retry() and sit9531x_flush_retry(), and should
sit9531x_prg_commit() run before sit9531x_retime_retry()?

Both sit9531x_retime_retry() and sit9531x_flush_retry() call
sit9531x_output_phase_flush(), so if sit9531x_phase_flush_undo() fails
during either retry, flush_ctrl_pending or flush_arm_pending is set after
sit9531x_phase_flush_undo_retry() and the teardown warnings have already
executed. In addition, if sitdev->loops_open is true when
sit9531x_retime_retry() calls sit9531x_output_phase_adjust_set() and the
delay bytes already match (same is true), sit9531x_output_phase_flush() will
fire into unlocked loops before the "if (sitdev->loops_open)" block commits
and locks them.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261110202607.55744-1-arouhi@sitime.com?part=11