Re: [PATCH net-next v9 14/15] 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:
- [Medium] dpll: missing debug block relock in INTSYNC enable/disable paths
- [Medium] dpll: cache desynchronization on I/O error allows multiple INTSYNC drivers
--

commit 218b2f034db2d78ade260886ea4d74abbdcc7ca8
Author: Oleg Zadorozhnyi <Oleg.Zadorozhnyi@devoxsoftware.com>

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

The device has an internal net by which one PLL can drive the others.
This commit models the source and destination behavior separately, allowing
each pin to faithfully report its own state.

> 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

[ ... ]

> +int sit9531x_intsync_enable(struct sit9531x_dev *sitdev, u8 src_pll_idx)
> +{
> +	u8 ext_page, val;
> +	int rc, i;
> +
> +	lockdep_assert_held(&sitdev->multiop_lock);

[ ... ]

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

[Severity: Medium]
Does this leave the EXT debug page permanently unlocked on success?

The debug registers are unlocked with SIT9531X_PLL_DEBUG_UNLOCK, but
SIT9531X_PLL_DEBUG_LOCK is never written before returning. Does this
bypass the hardware protection against spurious writes?

> +
> +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.
> +	 */

[ ... ]

> +int sit9531x_intsync_disable(struct sit9531x_dev *sitdev, u8 src_pll_idx)
> +{
> +	u8 ext_page, val;
> +	int rc, i;

[ ... ]

> +	/* Unlock debug on EXT page */
> +	rc = sit9531x_write_u8(sitdev, SIT9531X_REG(ext_page, SIT9531X_PLL_REG_DEBUG),
> +			       SIT9531X_PLL_DEBUG_UNLOCK);
> +	if (rc)
> +		return rc;
> +
> +	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;
> +}

[Severity: Medium]
Similarly, does the disable path also leave the debug page unlocked?

> 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

[ ... ]

> +static int
> +sit9531x_dpll_intsync_src_state_on_dpll_set(const struct dpll_pin *pin,
> +					    void *pin_priv,
> +					    const struct dpll_device *dpll,
> +					    void *dpll_priv,
> +					    enum dpll_pin_state state,
> +					    struct netlink_ext_ack *extack)
> +{
> +	struct sit9531x_dpll *sitdpll = dpll_priv;
> +	struct sit9531x_dev *sitdev = sitdpll->dev;
> +	int rc = 0, detect_rc = 0;
> +
> +	mutex_lock(&sitdev->multiop_lock);
> +
> +	switch (state) {
> +	case DPLL_PIN_STATE_CONNECTED:

[ ... ]

> +		rc = sit9531x_intsync_enable(sitdev, sitdpll->id);
> +		break;
> +	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);
> +
> +	mutex_unlock(&sitdev->multiop_lock);
> +
> +	if (rc && rc != -EBUSY && rc != -EINVAL && rc != -EOPNOTSUPP)
> +		NL_SET_ERR_MSG(extack, "Failed to set INTSYNC source state");
> +
> +	return rc;
> +}

[Severity: Medium]
Can a transient I2C error here lead to a hardware collision?

If sit9531x_intsync_enable() succeeds (rc == 0) but the subsequent
sit9531x_intsync_src_detect() fails due to a bus error, detect_rc is
ignored and the function returns success.

Since sit9531x_intsync_src_detect() bails out on read failures without
updating sitdev->intsync_src, won't the software cache still claim no
PLL is driving INTSYNC (-1) while the hardware is actively driving it?

Could a subsequent request then successfully connect a second PLL to
drive the net, causing a hardware collision?

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