Re: [PATCH net-next v14 12/12] dpll: sit9531x: model the inter-PLL sync net as a pair of pins

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 0 · Medium: 5 · 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] In sit9531x_intsync_src_detect()
  (drivers/dpll/sit9531x/core.c), the global INTSYNC enable (page 0 reg…
- [Medium] sit9531x_resume() (drivers/dpll/sit9531x/core.c) re-reads
  INTSYNC ownership (intsync_src/intsync_partial) and every output's…
- [Medium] This patch defines an undriven INTSYNC net as having no
  signal: sit9531x_dpll_selection_no_signal() makes the destination pin…
- [Medium] sit9531x_intsync_src_detect() returns 'no owner'
  (intsync_src=-1, partial=false) as soon as the global INTSYNC bit…
- [Medium] sit9531x_dpll_intsync_src_state_on_dpll_set() forces a
  partial-owner record only after a failed CONNECTED.
- [Low] If sit9531x_intsync_src_detect() fails during sit9531x_resume(),
  intsync_src and intsync_partial keep their pre-suspend values and…
- [Low] The pin-state contract comment in drivers/dpll/sit9531x/dpll.c
  defines predicate N as 'the input lane's clock monitor reports loss of…

--

dpll: sit9531x: model the inter-PLL sync net as a pair of pins

This patch models the INTSYNC net as two pins: an output (source) pin on
the PLL that drives the net, and an input (destination) pin on each PLL
that can select it. It adds enable and disable sequences for the source
configuration and a scan that detects the current owner, including a
partial one. On resume it reads INTSYNC ownership and the output mutes
back from the part.

> diff --git a/drivers/dpll/sit9531x/core.c b/drivers/dpll/sit9531x/core.c
> index 10609d46a7e1a..7de6584acea96 100644
> --- a/drivers/dpll/sit9531x/core.c
> +++ b/drivers/dpll/sit9531x/core.c

[ ... ]

> +int sit9531x_intsync_src_detect(struct sit9531x_dev *sitdev)
> +{

[ ... ]

> +	rc = sit9531x_read_u8(sitdev, SIT9531X_REG_INTSYNC_GLOBAL, &global);
> +	if (rc)
> +		return rc;
> +
> +	if (!(global & BIT(SIT9531X_INTSYNC_EN_BIT))) {
> +		sitdev->intsync_src = -1;
> +		sitdev->intsync_partial = false;
> +		return 0;
> +	}

[Severity: Medium]
Can this early return hide a source configuration that the new error
paths leave behind?

Say sit9531x_intsync_enable(X) writes every en_val byte and then
sit9531x_pll_small_update() fails. The rollback runs
__sit9531x_intsync_disable(X, false), which clears and latches the global
bit. If the EXT unlock or the first dis_val write then fails, it jumps to
restore_global, which does nothing when restore is false. X's EXT page
still holds the enable pattern, but the global bit is clear.

A disconnect can end in the same state: its dis_val write fails, and then
the restore read or write fails too.

The re-scan in sit9531x_dpll_intsync_src_state_on_dpll_set() then
succeeds and returns intsync_src = -1, so the partial-owner fallback does
not fire:

	if (changed && rc && state == DPLL_PIN_STATE_CONNECTED &&
	    (detect_rc || sitdev->intsync_src == sitdpll->id)) {

A later CONNECTED on PLL Z passes the -EBUSY check. Its
sit9531x_intsync_enable(Z) sets the global bit, which also re-activates
the leftover pattern on X.

The next scan matches both PLLs and keeps the lower index, with only a
warning. If X < Z, intsync_src becomes X even though Z's request
succeeded.

After that, Z's source pin reads DISCONNECTED and a DISCONNECTED on Z is
a no-op. SELECTABLE on Z's own INTSYNC destination is no longer refused
either. Would that let Z select the net it drives?

This also seems to contradict the commit message:

  "A source that failed to take the net over ... is recorded as a
  partial owner"

A profile that leaves the global bit clear with an enable pattern in an
EXT page would hit the same case at probe or resume.

[ ... ]

> +	rc = sit9531x_pll_small_update(sitdev, src_pll_idx);
> +
> +	goto relock;
> +
> +restore_global:

[ ... ]

> +	if (restore &&
> +	    !sit9531x_read_u8(sitdev, SIT9531X_REG_INTSYNC_GLOBAL, &val) &&
> +	    !sit9531x_write_u8(sitdev, SIT9531X_REG_INTSYNC_GLOBAL,
> +			       val | BIT(SIT9531X_INTSYNC_EN_BIT))) {
> +		/*
> +		 * Every write of the global enable, here and in SiTime's
> +		 * sequences, is followed by the page-0 small update that
> +		 * applies it.  Without it the bit would sit pending until
> +		 * an unrelated commit through the same update register
> +		 * applied it, re-asserting the net over a half-written
> +		 * EXT page with nobody having asked.
> +		 */
> +		sit9531x_write_u8(sitdev, SIT9531X_REG_GLOBAL_UPDATE,
> +				  SIT9531X_SMALL_UPDATE_CMD);
> +		usleep_range(1000, 2000);
> +	}

[Severity: Medium]
What happens if this GLOBAL_UPDATE write fails? Its return value is
ignored, so the enable bit can stay pending. The comment above says that
state re-asserts the net when some unrelated commit goes through the same
update register.

In that case the re-scan in sit9531x_dpll_intsync_src_state_on_dpll_set()
reads the pending global bit and the untouched EXT pattern. It records a
complete owner even though the gate is off.

The commit message says "the restore is latched as every other write to
that register is", but a failure of that latch is not checked.

The final step has a similar problem. Suppose the disable writes every
dis_val byte and only sit9531x_pll_small_update() fails, and the re-scan
then also hits a read error. The cache still holds intsync_src == this
PLL with intsync_partial == false, so the source pin reports CONNECTED
while the net is off.

The partial-owner handling in the setter only covers failed connects:

	if (changed && rc && state == DPLL_PIN_STATE_CONNECTED &&

In both cases, would a later CONNECTED on this PLL take this path and
return success without running the enable?

		if (was_connected)
			break;

The periodic poll does not refresh ownership either.

[ ... ]

> @@ -4894,8 +5278,35 @@ static int sit9531x_suspend(struct device *dev)
>  static int sit9531x_resume(struct device *dev)
>  {
>  	struct sit9531x_dev *sitdev = dev_get_drvdata(dev);
> +	int rc;
> +	u8 i;
>  
>  	sit9531x_page_cache_drop(sitdev);

[ ... ]

> +	mutex_lock(&sitdev->multiop_lock);
> +	rc = sit9531x_intsync_src_detect(sitdev);
> +	if (rc)
> +		dev_warn(sitdev->dev,
> +			 "INTSYNC source not re-detected on resume: %d\n", rc);
> +	sitdev->intsync_stale = rc != 0;
> +	for (i = 0; i < sitdev->info->num_outputs; i++) {
> +		rc = sit9531x_output_state_refresh(sitdev, i);
> +		if (rc) {
> +			dev_warn(sitdev->dev,
> +				 "Output %u mute state not read on resume: %d\n",
> +				 i, rc);
> +			sitdev->out[i].state_stale = true;
> +		}
> +	}
> +	mutex_unlock(&sitdev->multiop_lock);

[Severity: Medium]
Are pin-change notifications needed here? This rewrites intsync_src,
intsync_partial and out[].enabled from the hardware. It does not compare
them with the previous values and does not notify anyone.

The poll won't catch the change either, because
sit9531x_dpll_changes_check() skips all but the selection pins:

		if (!sit9531x_dpll_is_selection_pin(pin))
			continue;

The comment on sit9531x_dpll_is_selection_pin() says outputs, including
the INTSYNC source, "change only through their own set callback". The
new source setter also says "the poll does not watch this pin".

The part may lose power across suspend and come back with a different
INTSYNC owner or different output mutes. PIN_GET would then report the
new state, while netlink subscribers keep the pre-suspend state.

> +
>  	if (sitdev->irq > 0)
>  		enable_irq(sitdev->irq);
>  	kthread_queue_delayed_work(sitdev->kworker, &sitdev->work, 0);

[Severity: Low]
If sit9531x_intsync_src_detect() failed above, intsync_src and
intsync_partial keep their pre-suspend values and only intsync_stale is
set.

The poll queued here does not call sit9531x_dpll_intsync_refresh(), but
it still reads those fields:

sit9531x_dpll_changes_check()
  sit9531x_dpll_pin_snapshot()
    if (id == SIT9531X_INTSYNC_PIN_ID && sitdev->intsync_src == sitdpll->id)
    sit9531x_dpll_selection_operstate_get()
      sit9531x_dpll_selection_no_signal()

Would the poll then compute and store the destination pin state and
operstate from the stale owner?

If the stale result matches the baseline, no notification goes out. Once
a netlink GET refreshes the cache, the next tick would send a
notification that contradicts the earlier state.

> diff --git a/drivers/dpll/sit9531x/dpll.c b/drivers/dpll/sit9531x/dpll.c
> index dc8d84133b72b..9826adb353c95 100644
> --- a/drivers/dpll/sit9531x/dpll.c
> +++ b/drivers/dpll/sit9531x/dpll.c

[ ... ]

> +static bool
> +sit9531x_dpll_selection_no_signal(struct sit9531x_dev *sitdev, u8 pin_id)
> +{
> +	lockdep_assert_held(&sitdev->multiop_lock);
> +
> +	if (pin_id == SIT9531X_INTSYNC_PIN_ID) {
> +		/*
> +		 * The driver knows the net is driven when it enabled the
> +		 * source itself or found the configuration it would have
> +		 * written.  A profile can drive the net with a configuration
> +		 * of its own; then a PLL the device feeds from the net and
> +		 * holds locked is the evidence that the net carries a signal.
> +		 */
> +		if (sitdev->intsync_src >= 0 && !sitdev->intsync_partial)
> +			return false;
> +		return !sit9531x_dpll_intsync_tracked(sitdev);
> +	}

[Severity: Low]
This isn't a bug, but the pin-state contract comment earlier in dpll.c
still says:

 *   ...  The INTSYNC destination
 *   has no monitor, so N and Q never hold for it.

With this change N can hold for INTSYNC. In that case
sit9531x_dpll_selection_operstate_get() reports
DPLL_PIN_OPERSTATE_NO_SIGNAL where the contract predicts STANDBY. Should
the comment be updated?

[ ... ]

> +	switch (state) {
> +	case DPLL_PIN_STATE_CONNECTED:

[ ... ]

> +		if (was_connected)
> +			break;
> +		if (sitdev->intsync_src >= 0 &&
> +		    sitdev->intsync_src != sitdpll->id) {
> +			NL_SET_ERR_MSG(extack,
> +				       "INTSYNC is already sourced by another PLL");
> +			rc = -EBUSY;
> +			break;
> +		}

[ ... ]

> +		rc = sit9531x_intsync_enable(sitdev, sitdpll->id);
> +		changed = true;
> +		break;
> +	case DPLL_PIN_STATE_DISCONNECTED:
> +		if (sitdev->intsync_src != sitdpll->id)
> +			break;
> +		rc = sit9531x_intsync_disable(sitdev, sitdpll->id);
> +		changed = true;
> +		break;

[Severity: Medium]
How does this handle a net that a profile drives with its own EXT page
values? sit9531x_dpll_selection_no_signal() above says that can happen.

In that case sit9531x_intsync_src_detect() finds the global bit set but
no PLL matching intsync_config, and it ends with:

	sitdev->intsync_src = src;

where src is still -1. Every source pin then reports DISCONNECTED while
the net is driven.

A CONNECTED on any PLL passes the -EBUSY check above, because the check
only looks at intsync_src >= 0. sit9531x_intsync_enable() then writes a
second source configuration on top of the profile's.

A DISCONNECTED on any PLL takes the intsync_src != sitdpll->id break and
returns 0 without touching the hardware. dpll_pin_state_set() still
sends a change notification for the success.

If the PLL the driver enabled is disconnected later, the shared global
bit is cleared. That also cuts off the profile's source.

The commit message says setting the source pin "takes the net over or
gives it up", but for a profile-driven net it seems to do neither.
Should a set global bit with no recognised pattern be treated as busy, or
as an unknown owner?

[ ... ]

> +	case DPLL_PIN_STATE_SELECTABLE:
> +		if (sitdev->intsync_src == sitdpll->id) {
> +			NL_SET_ERR_MSG(extack,
> +				       "PLL cannot lock to the INTSYNC it drives");
> +			rc = -EINVAL;
> +			break;
> +		}
> +		rc = sit9531x_input_prio_add(sitdev, sitdpll->id, hw_src);
> +		break;

[Severity: Medium]
Can this move a PLL onto an undriven INTSYNC net? For reporting, this
patch treats an undriven net as having no signal. The selector that
writes the active reference still treats INTSYNC as always live:

drivers/dpll/sit9531x/core.c:sit9531x_prio_src_live() {
	u8 index = sit9531x_hw_src_input(src);

	if (index >= sitdev->info->num_inputs)
		return true;
	...
}

Nothing here checks that any PLL drives the net. With no source pin
connected, a user can give INTSYNC a higher priority than a healthy
external input and set it SELECTABLE. Then:

sit9531x_input_prio_add()
  sit9531x_prio_table_apply()
    sit9531x_prio_table_commit()
      sit9531x_prio_activesel_pick()
        sit9531x_prio_top_live()

picks INTSYNC as the active selection. The comment on
sit9531x_prio_activesel_pick() says that a selection naming a dead source
means the next table write "sends the PLL back to it and it unlocks".
Would the PLL lose lock here?

This path was not reachable before, because the INTSYNC destination pin
was not registrable.

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