Re: [PATCH v10 13/14] dpll: sit9531x: model the inter-PLL sync net as a pair of pins

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] dpll: sit9531x: intsync_disable unconditionally re-enables sync on success
- [Medium] dpll: sit9531x: error paths in intsync_disable bypass global bit rollback
--

commit 95c79d6f4b1eae6316f594ced6eed52e8dcec752
Author: Oleg Zadorozhnyi <Oleg.Zadorozhnyi@devoxsoftware.com>

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

This commit updates the sit9531x driver to model the inter-PLL sync net
as a pair of pins (one output, multiple inputs) to better track state.
It introduces functions to enable, disable, and detect the sync net source
across the phase-locked loops.

> diff --git a/drivers/dpll/sit9531x/core.c b/drivers/dpll/sit9531x/core.c
> index 1cb691f040194..e8d47999f1a97 100644
> --- a/drivers/dpll/sit9531x/core.c
> +++ b/drivers/dpll/sit9531x/core.c

[ ... ]

> +/*
> + * sit9531x_intsync_disable - disable inter-PLL synchronization
> + * @src_pll_idx: source (frequency) PLL index (0-3)
> + *
> + * Clears INTSYNC global bit, writes disable values to the source
> + * PLL's EXT page, and triggers a small update.
> + *
> + * Caller must hold sitdev->multiop_lock.
> + */
> +int sit9531x_intsync_disable(struct sit9531x_dev *sitdev, u8 src_pll_idx)
> +{
> +	u8 ext_page, val;
> +	int rc, lock_rc, i;

[ ... ]

> +	rc = sit9531x_read_u8(sitdev, SIT9531X_REG_INTSYNC_GLOBAL, &val);
> +	if (rc)
> +		return rc;
> +	rc = sit9531x_write_u8(sitdev, SIT9531X_REG_INTSYNC_GLOBAL,
> +			       val & ~BIT(SIT9531X_INTSYNC_EN_BIT));
> +	if (rc)
> +		return rc;
> +
> +	/* Small update on Page 0 */
> +	rc = sit9531x_write_u8(sitdev, SIT9531X_REG_GLOBAL_UPDATE,
> +			       SIT9531X_SMALL_UPDATE_CMD);
> +	usleep_range(1000, 2000);
> +	if (rc)
> +		return rc;

[Severity: Medium]
If this I2C write fails, returning directly bypasses the restore_global
recovery block below. Does this leave the global bit cleared while the EXT
page still holds the enable pattern? This could cause the detector to report
the net as unowned, causing any retry of the disable operation to short
circuit.

> +
> +	/* 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 relock;

[Severity: Medium]
Jumping directly to relock on a debug unlock failure abandons the global bit
rollback. Should this jump to restore_global instead to ensure the global
state is properly reverted?

> +
> +	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)
> +			goto restore_global;
> +	}
> +
> +	/* 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 relock;

[Severity: Medium]
Similar to the debug unlock failure, jumping to relock here on a small update
failure also fails to revert the global state. 

> +
> +	rc = 0;
> +
> +restore_global:

[Severity: High]
Is a goto relock missing right before the restore_global label? Without it,
a completely successful disable sequence will unconditionally fall through
into this error recovery block, which writes the INTSYNC_EN_BIT back to 1.
This could leave the hardware in a corrupted state where the global enable
is set but the EXT page is disabled.

> +	/*
> +	 * The global enable was cleared first, so a failure here leaves the
> +	 * EXT page still holding the enable pattern with nothing pointing
> +	 * at it: the source detector keys on the global bit, would report
> +	 * the net as unowned, and a retry of the disable would then
> +	 * short-circuit.  Put the bit back so the state stays one the
> +	 * driver can describe and the request can be repeated.
> +	 */
> +	if (!sit9531x_read_u8(sitdev, SIT9531X_REG_INTSYNC_GLOBAL, &val))
> +		sit9531x_write_u8(sitdev, SIT9531X_REG_INTSYNC_GLOBAL,
> +				  val | BIT(SIT9531X_INTSYNC_EN_BIT));
> +
> +relock:
> +	/* Close the EXT page debug window the sequence opened. */
> +	lock_rc = sit9531x_intsync_debug_lock(sitdev, ext_page);
> +	if (lock_rc && !rc)
> +		rc = lock_rc;
> +
> +	return rc;
> +}

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