Re: [PATCH net-next v9 14/15] 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 8 potential
issue(s) to consider:

Critical: 0 · High: 2 · Medium: 3 · Low: 3

- [High] Unbalanced acquire/release of the exclusive INTSYNC net:
  sit9531x_intsync_enable()/disable() never record ownership; the only…
- [High] The new sit9531x_dpll_intsync_dst_pin_ops wires .prio_set =
  sit9531x_dpll_input_pin_prio_set, which ends with 'while…
- [Medium] sit9531x_intsync_enable() and sit9531x_intsync_disable() write
  the per-page debug key (offset SIT9531X_PLL_REG_DEBUG = 0xBD, value…
- [Medium] Source misattribution after a partially failed disable:
  sit9531x_intsync_disable() clears and commits the global enable bit…
- [Medium] The new source-side loopback check can be bypassed through a
  stale cache: a failed INTSYNC destination removal still rebuilds…
- [Low] The patch adds a second, identical prototype for
  sit9531x_input_prio_present() to core.h; the same declaration already…
- [Low] The patch contains a comment-only rewrite of the
  sit9531x_output_phase_adjust_set() kerneldoc (slot-vs-logical-index…
- [Low] sit9531x_dpll_intsync_dst_state_on_dpll_set() calls
  sit9531x_input_prio_add(), which returns -ENOSPC when the priority…

--

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

The patch exposes the chip's internal inter-PLL sync (INTSYNC) net as two
DPLL pins: an output pin on the PLL that drives the net and an input pin on
each PLL that can select it as a reference.  New core helpers program the
source PLL's EXT page and a detector re-reads which PLL currently drives the
net into sitdev->intsync_src.

A few questions below about the ownership bookkeeping, the debug-key
handling and the new destination pin ops.

> diff --git a/drivers/dpll/sit9531x/core.c b/drivers/dpll/sit9531x/core.c
> index 88bfa63b29755..b15859890caa7 100644
> --- a/drivers/dpll/sit9531x/core.c
> +++ b/drivers/dpll/sit9531x/core.c
> @@ -2024,12 +2024,13 @@ int sit9531x_output_freq_get(struct sit9531x_dev *sitdev, u8 out_idx,
>   *   base + 3  PROG3  PRG_RST_DELAY[15:8]
>   *   base + 4  PROG2  PRG_RST_DELAY[7:0]
>   *
> - * Outputs 0-5 live on Page 3, outputs 6-11 on Page 4, with each
> - * output's block at base = 0x15 + 16 * (out_idx % 6).
> + * Slots 0-5 live on Page 3, slots 6-11 on Page 4, with each slot's
> + * block at base = 0x15 + 16 * (slot % 6); the slot is the physical
> + * output position from clkout_map[], not the logical output index.
>   *
> - * The chip only supports unsigned positive delay.  A negative phase
> - * adjustment (advance) is wrapped to (T_out - |phase|) modulo one
> - * output period, which is identical for a periodic signal.
> + * The chip only supports unsigned positive delay.  Requests are folded
> + * modulo one output period: positive delays wrap naturally and a negative
> + * phase adjustment (advance) is rendered as (T_out - |phase|).
>   */

[Severity: Low]
This isn't a bug, but is this hunk meant to be part of this patch?  It only
rewrites the documentation block above sit9531x_output_phase_adjust_set()
(slot versus logical index, phase folding wording) and no code in that
function changes, while the commit message describes only the INTSYNC pin
model.  Could it move to its own patch so a reader does not have to check
whether phase-adjust behaviour changed here?

> @@ -2282,6 +2283,225 @@ int sit9531x_clear_notifications(struct sit9531x_dev *sitdev)
>  	return 0;
>  }
>  

[ ... ]

> +int sit9531x_intsync_src_detect(struct sit9531x_dev *sitdev)
> +{
> +	s8 src = -1;
> +	u8 global;
> +	u8 pll, ext_page;
> +	int rc, i;
> +
> +	lockdep_assert_held(&sitdev->multiop_lock);
> +
> +	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;
> +		return 0;
> +	}
> +
> +	for (pll = 0; pll < SIT9531X_NUM_PLLS; pll++) {
> +		ext_page = SIT9531X_PLL_EXT_PAGE(pll);
> +
> +		for (i = 0; i < ARRAY_SIZE(intsync_config); i++) {
> +			u16 reg;
> +			u8 val;
> +
> +			reg = SIT9531X_REG(ext_page, intsync_config[i].offset);
> +
> +			rc = sit9531x_read_u8(sitdev, reg, &val);
> +			if (rc)
> +				return rc;
> +			if (val != intsync_config[i].en_val)
> +				break;
> +		}
> +
> +		if (i == ARRAY_SIZE(intsync_config)) {
> +			/*
> +			 * Only one PLL can drive the net.  If a second
> +			 * one matches, the registers are not describing
> +			 * a state this driver put the device in, so say
> +			 * so rather than pick silently.
> +			 */
> +			if (src < 0)
> +				src = pll;
> +			else
> +				dev_warn(sitdev->dev,
> +					 "PLL%c also matches the INTSYNC source pattern; keeping PLL%c\n",
> +					 'A' + pll, 'A' + src);
> +		}
> +	}
> +
> +	sitdev->intsync_src = src;
> +
> +	return 0;
> +}

[Severity: Medium]
Can a partially failed disable make this detector attribute the net to the
wrong PLL?

sit9531x_intsync_disable() clears and commits the global enable bit first,
and only afterwards unlocks the EXT page and writes the eight dis_val
bytes, each with a bare "if (rc) return rc;".  If one of those writes
fails, that PLL's EXT page still holds the complete en_val pattern.

sit9531x_intsync_src_detect() then sees the cleared global bit:

	if (!(global & BIT(SIT9531X_INTSYNC_EN_BIT))) {
		sitdev->intsync_src = -1;
		return 0;
	}

so the leftover pattern is never noticed, and a retry of DISCONNECTED in
sit9531x_dpll_intsync_src_state_on_dpll_set() short-circuits on
"if (sitdev->intsync_src != sitdpll->id) break;".

When a later CONNECTED on another PLL re-asserts the global bit, the loop
here keeps the first matching PLL, so the stale lower-numbered PLL shadows
the PLL that was actually enabled: the newly connected pin reports
DISCONNECTED and cannot be disconnected, an unrelated PLL's source pin
reports CONNECTED, and two PLLs carry the source pattern for one net.
Would clearing the EXT page before the global bit, or recording an
"unknown/needs cleanup" state, be preferable to only warning here?

> +
> +/*
> + * sit9531x_intsync_enable - enable inter-PLL synchronization
> + * @src_pll_idx: source (frequency) PLL index (0-3)
> + *
> + * Enables INTSYNC global bit, unlocks the source PLL's EXT page
> + * debug registers, writes configuration, and triggers a small
> + * update on the source PLL.
> + *
> + * Caller must hold sitdev->multiop_lock.
> + */
> +int sit9531x_intsync_enable(struct sit9531x_dev *sitdev, u8 src_pll_idx)
> +{

[ ... ]

> +	/* Unlock debug on EXT page */
> +	rc = sit9531x_write_u8(sitdev, SIT9531X_REG(ext_page, SIT9531X_PLL_REG_DEBUG),
> +			       SIT9531X_PLL_DEBUG_UNLOCK);
> +	if (rc)
> +		goto err_disable;

[Severity: Medium]
Should the debug key be written back to SIT9531X_PLL_DEBUG_LOCK before
returning?  Both sit9531x_intsync_enable() and sit9531x_intsync_disable()
write SIT9531X_PLL_DEBUG_UNLOCK to SIT9531X_PLL_REG_DEBUG on the source
PLL's EXT page and never restore the lock value, on success or on any error
path (the enable error path calls disable(), which also only unlocks), so
that page's protected debug registers stay open for the lifetime of the
device after any INTSYNC source set or clear.

The existing sit9531x_phase_offset_read() in the same file always closes
the window again:

relock:
	/*
	 * Close the debug window again.  The key register opens every debug
	 * register on this PLL while it holds the unlock value ...
	 */
	lock_rc = sit9531x_write_pll_u8(sitdev, pll_idx,
					SIT9531X_PLL_REG_DEBUG,
					SIT9531X_PLL_DEBUG_LOCK);

Is there a reason the INTSYNC helpers do not follow the same unlock/relock
protocol?

> +
> +	for (i = 0; i < ARRAY_SIZE(intsync_config); i++) {
> +		rc = sit9531x_write_u8(sitdev,
> +				       SIT9531X_REG(ext_page,
> +						    intsync_config[i].offset),
> +				       intsync_config[i].en_val);
> +		if (rc)
> +			goto err_disable;
> +	}
> +
> +	/* Small update on source PLL */
> +	rc = sit9531x_write_pll_u8(sitdev, src_pll_idx,
> +				   SIT9531X_PLL_REG_SMALL_UPDATE,
> +				   SIT9531X_SMALL_UPDATE_CMD);
> +	if (rc)
> +		goto err_disable;
> +
> +	return 0;
> +
> +err_disable:
> +	/*
> +	 * The global enable is already set at this point.  The caller only
> +	 * records the source PLL when this function succeeds, so nothing
> +	 * else will ever clear the bit: undo it here rather than leave the
> +	 * net asserted with a half-written EXT page.
> +	 */

[Severity: High]
Is the caller contract described in this comment actually implemented?
"The caller only records the source PLL when this function succeeds"
suggests the caller assigns sitdev->intsync_src on success, but
sit9531x_dpll_intsync_src_state_on_dpll_set() never assigns it; the only
writer of the field is sit9531x_intsync_src_detect(), whose failure is
downgraded to a dev_warn() there.  See the comment on that call site below.

[ ... ]

> +int sit9531x_intsync_disable(struct sit9531x_dev *sitdev, u8 src_pll_idx)
> +{

[ ... ]

> +	for (i = 0; i < ARRAY_SIZE(intsync_config); i++) {
> +		rc = sit9531x_write_u8(sitdev,
> +				       SIT9531X_REG(ext_page,
> +						    intsync_config[i].offset),
> +				       intsync_config[i].dis_val);
> +		if (rc)
> +			return rc;
> +	}
> +
> +	/* Small update on source PLL */
> +	rc = sit9531x_write_pll_u8(sitdev, src_pll_idx,
> +				   SIT9531X_PLL_REG_SMALL_UPDATE,
> +				   SIT9531X_SMALL_UPDATE_CMD);
> +	if (rc)
> +		return rc;
> +
> +	return 0;
> +}

[ ... ]

> diff --git a/drivers/dpll/sit9531x/core.h b/drivers/dpll/sit9531x/core.h
> index 1f77ff28ec814..5275733a3b4a9 100644
> --- a/drivers/dpll/sit9531x/core.h
> +++ b/drivers/dpll/sit9531x/core.h
> @@ -252,6 +252,8 @@ int sit9531x_input_prio_set(struct sit9531x_dev *sitdev, u8 pll_idx,
>  			    u8 input_idx, u8 prio);
>  int sit9531x_input_prio_get(struct sit9531x_dev *sitdev, u8 pll_idx,
>  			    u8 input_idx, u8 *prio);
> +bool sit9531x_input_prio_present(struct sit9531x_dev *sitdev,
> +				 u8 pll_idx, u8 input_idx);
>  int sit9531x_input_prio_remove(struct sit9531x_dev *sitdev, u8 pll_idx,
>  			       u8 input_idx);

[Severity: Low]
This isn't a bug, but core.h already declares sit9531x_input_prio_present()
with exactly this prototype a few lines above, under the input priority
comment, before this patch.  Is this second declaration left over from a
rebase?  It can be dropped.

[ ... ]

> diff --git a/drivers/dpll/sit9531x/dpll.c b/drivers/dpll/sit9531x/dpll.c
> index 2a973364a9cd2..3efe6fc90701d 100644
> --- a/drivers/dpll/sit9531x/dpll.c
> +++ b/drivers/dpll/sit9531x/dpll.c
> @@ -792,8 +806,222 @@ sit9531x_dpll_output_pin_direction_get(const struct dpll_pin *pin,

[ ... ]

> +	switch (state) {
> +	case DPLL_PIN_STATE_CONNECTED:
> +		if (sitdev->intsync_src == sitdpll->id)
> +			break;
> +		if (sitdev->intsync_src >= 0) {
> +			NL_SET_ERR_MSG(extack,
> +				       "INTSYNC is already sourced by another PLL");
> +			rc = -EBUSY;
> +			break;
> +		}
> +		/*
> +		 * A PLL that already lists INTSYNC among its references must
> +		 * not also drive it: the destination side refuses the mirror
> +		 * of this, and without the check here the net could be routed
> +		 * back into the PLL feeding it.
> +		 */
> +		if (sit9531x_input_prio_present(sitdev, sitdpll->id,
> +						sit9531x_input_hw_src(SIT9531X_INTSYNC_PIN_ID))) {
> +			NL_SET_ERR_MSG(extack,
> +				       "PLL selects INTSYNC as a reference; it cannot drive it");
> +			rc = -EBUSY;
> +			break;
> +		}
> +		rc = sit9531x_intsync_enable(sitdev, sitdpll->id);
> +		break;

[Severity: Medium]
Can this loopback check be bypassed by a stale priority mask?

sit9531x_input_prio_present() reads only chan->prio_mask:

	return !!(sitdev->chan[pll_idx].prio_mask & BIT(input_idx));

and sit9531x_prio_table_commit() rebuilds that mask from the *intended*
table even when a slot write failed:

	sit9531x_prio_mask_build(sitdev, pll_idx, srcs);

So after a failed DISCONNECTED on the INTSYNC destination pin of a PLL
(sit9531x_dpll_intsync_dst_state_on_dpll_set() -> sit9531x_input_prio_remove()
-> sit9531x_prio_table_commit()), the mask says INTSYNC is absent while the
hardware slot still holds it.  A CONNECTED request on the same PLL's source
pin before the next periodic sit9531x_chan_state_fetch() re-read then passes
this check and calls sit9531x_intsync_enable(), which is the self-reference
the check exists to prevent.  Would reading the table back here, or not
updating the mask on a partial commit, close that window?

> +	case DPLL_PIN_STATE_DISCONNECTED:
> +		if (sitdev->intsync_src != sitdpll->id)
> +			break;
> +		rc = sit9531x_intsync_disable(sitdev, sitdpll->id);
> +		break;
> +	default:
> +		rc = -EINVAL;
> +		break;
> +	}
> +
> +	/*
> +	 * Re-scan hardware after source state transitions so cache follows
> +	 * partially failed enable/disable paths as closely as possible.
> +	 */
> +	if (state == DPLL_PIN_STATE_CONNECTED ||
> +	    state == DPLL_PIN_STATE_DISCONNECTED)
> +		detect_rc = sit9531x_intsync_src_detect(sitdev);
> +	/*
> +	 * The refresh only re-reads what the device now shows.  Failing
> +	 * the request because that read hit a bus error would tell
> +	 * userspace the enable did not happen when it did.
> +	 */
> +	if (detect_rc)
> +		dev_warn(sitdev->dev,
> +			 "INTSYNC source cache not refreshed: %d\n",
> +			 detect_rc);

[Severity: High]
Can the net become impossible to release when this refresh fails?

sit9531x_intsync_enable() and sit9531x_intsync_disable() never touch
sitdev->intsync_src, so this detect call is the only thing that records
ownership.  In sit9531x_intsync_src_detect() any read error returns before
the assignment:

			rc = sit9531x_read_u8(sitdev, reg, &val);
			if (rc)
				return rc;
	...
	sitdev->intsync_src = src;

So one i2c read error after a successful enable leaves intsync_src == -1
while the hardware drives the net.  From then on:

  - the source pin's state_on_dpll_get reports DISCONNECTED,
  - a DISCONNECTED request hits "if (sitdev->intsync_src != sitdpll->id)
    break;" and never calls sit9531x_intsync_disable(),
  - the "already sourced by another PLL" check passes, so a second PLL can
    be programmed onto the same single net,
  - the destination-side "PLL cannot lock to the INTSYNC it drives" guard
    stops matching.

sit9531x_dev_periodic_work() refreshes only ref[] and chan[] and never
re-runs the INTSYNC detect, so the divergence is not repaired later.  The
mirror case (successful disable, failed detect) keeps a stale positive
value and refuses every other PLL with -EBUSY.

Would recording ownership directly in the enable/disable helpers on success
(and clearing it on success of disable), instead of relying on a fallible
readback whose error is only warned about, avoid this?

>  /* ---- INTSYNC destination (input) pin ---- */
>  

[ ... ]

> +	switch (state) {
> +	case DPLL_PIN_STATE_DISCONNECTED:
> +		rc = sit9531x_input_prio_remove(sitdev, sitdpll->id, hw_src);
> +		break;
> +	case DPLL_PIN_STATE_CONNECTED:
> +		NL_SET_ERR_MSG(extack,
> +			       "Device selects its reference by priority; use selectable");
> +		rc = -EOPNOTSUPP;
> +		break;
> +	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;
> +	default:
> +		rc = -EINVAL;
> +		break;
> +	}
> +
> +	mutex_unlock(&sitdev->multiop_lock);
> +
> +	if (rc == -EBUSY)
> +		NL_SET_ERR_MSG(extack,
> +			       "Only source left in the priority table; it cannot be emptied");
> +	else if (rc && rc != -EINVAL && rc != -EOPNOTSUPP)
> +		NL_SET_ERR_MSG(extack, "Failed to set INTSYNC input state");

[Severity: Low]
Should -ENOSPC get its own message here?  sit9531x_input_prio_add()
returns -ENOSPC when the priority table is full of unique sources, and this
tail reports that as "Failed to set INTSYNC input state", which reads like
a bus failure.

The physical-input peer, sit9531x_dpll_input_pin_state_on_dpll_set(), calls
the same helper and maps it explicitly:

	else if (rc == -ENOSPC)
		NL_SET_ERR_MSG(extack,
			       "Priority table is full of unique sources on this PLL");

The comment above this function says it "accepts and refuses what a
physical input does", so is the divergence intentional?

> +
> +	return rc;
> +}
> +
> +/*
> + * Do not add .frequency_get / the generic input state getter here: the
> + * destination pin id is SIT9531X_INTSYNC_PIN_ID, one past the end of the
> + * ref[] array (INTSYNC is an internal net with no ref[] entry).  The ops
> + * below only ever key on chan[] and the priority table, never ref[id].
> + */
> +static const struct dpll_pin_ops sit9531x_dpll_intsync_dst_pin_ops = {
> +	.direction_get		= sit9531x_dpll_input_pin_direction_get,
> +	.state_on_dpll_get	= sit9531x_dpll_intsync_dst_state_on_dpll_get,
> +	.state_on_dpll_set	= sit9531x_dpll_intsync_dst_state_on_dpll_set,
> +	.prio_get		= sit9531x_dpll_input_pin_prio_get,
> +	.prio_set		= sit9531x_dpll_input_pin_prio_set,
> +};

[Severity: High]
Does wiring .prio_set here make a recursive dpll_lock acquisition reachable
on four more pins?

sit9531x_dpll_input_pin_prio_set() ends with:

	while (changed_cnt--)
		dpll_pin_change_ntf(changed[changed_cnt]);

and dpll_pin_change_ntf() takes the lock itself:

int dpll_pin_change_ntf(struct dpll_pin *pin)
{
	int ret;

	mutex_lock(&dpll_lock);

but every dpll_pin_ops callback already runs with dpll_lock held:
DPLL_CMD_PIN_SET uses .pre_doit = dpll_pin_pre_doit, which does
mutex_lock(&dpll_lock) and returns with it held until dpll_pin_post_doit(),
and dpll_pin_set_from_nlattr() -> dpll_pin_prio_set() invokes ops->prio_set
under it.  A DPLL_CMD_PIN_SET carrying DPLL_A_PIN_PRIO would then
re-acquire a non-recursive mutex the task already holds.

The path was introduced earlier in this series by "dpll: sit9531x: add
support to get and set priority on input pins", and this patch attaches the
same prio_set to the INTSYNC destination pin of each PLL, and makes that
pin a sibling candidate in the loop that fills changed[], so it can also
raise changed_cnt for the physical input pins.  Note the driver's own
comment in sit9531x_dev_periodic_work() states that "Those notification
helpers take DPLL-subsystem locks that are already held when our callbacks
are invoked from netlink context".

Would __dpll_pin_change_ntf(), which has lockdep_assert_held(&dpll_lock),
be the right helper here, or should the notification not be emitted from
inside the callback at all?

[ ... ]

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