Re: [PATCH net-next v9 06/15] dpll: sit9531x: implement input pin state on a DPLL

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] dpll: sit9531x: software mask corruption on failed priority table writes
- [Medium] dpll: sit9531x: physical receiver leak on partial disconnect failure
- [Medium] dpll: sit9531x: shadowed priority register read-modify-write hazard
--

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

dpll: sit9531x: implement input pin state on a DPLL

This patch implements the reporting and setting of input references for a
DPLL. It introduces mechanisms to rewrite the selection priority table and
manages enabling or disabling the physical inputs based on membership.

> diff --git a/drivers/dpll/sit9531x/core.c b/drivers/dpll/sit9531x/core.c
> index 6dfb1b31d4913..25418485e9757 100644
> --- a/drivers/dpll/sit9531x/core.c
> +++ b/drivers/dpll/sit9531x/core.c
[ ... ]
> @@ -365,6 +538,90 @@ static void sit9531x_prio_mask_build(struct sit9531x_dev *sitdev, u8 pll_idx,
[ ... ]
> +static int sit9531x_prio_table_commit(struct sit9531x_dev *sitdev, u8 pll_idx,
> +				      const u8 *srcs)
> +{
> +	u8 val, slot, attempt;
> +	int rc, prg_rc, ho_rc = 0;
> +	u16 reg;
> +
> +	rc = sit9531x_update_pll_u8(sitdev, pll_idx, SIT9531X_PLL_REG_HO_CTRL,
> +				    BIT(SIT9531X_PLL_HO_FORCE_BIT),
> +				    BIT(SIT9531X_PLL_HO_FORCE_BIT));
> +	if (rc)
> +		return rc;
> +
> +	usleep_range(10000, 12000);
> +
> +	for (slot = 0; slot < SIT9531X_PRIO_MAX_SLOTS; slot++) {
> +		reg = sit9531x_prio_reg(pll_idx, slot);
> +
> +		rc = sit9531x_read_u8(sitdev, reg, &val);
> +		if (rc)
> +			goto commit;
> +
> +		val = sit9531x_prio_slot_set(val, slot, srcs[slot]);
> +
> +		rc = sit9531x_write_u8(sitdev, reg, val);
> +		if (rc)
> +			goto commit;
> +	}

[Severity: Medium]
Does this read-modify-write sequence run the risk of clobbering unlatched
shadow registers? Since two slots share one register, if the hardware
returns the latched active state on read rather than the uncommitted shadow
state, wouldn't the read for the next slot fetch the old value for the prior
slot and overwrite the uncommitted write?

> +
> +commit:
> +	/*
> +	 * Latch unconditionally: slots written before a failed write are in
> +	 * the table regardless, so the latch keeps hardware and the mask
> +	 * rebuild below consistent with what was actually written.
> +	 */
> +	prg_rc = sit9531x_prio_prg_commit(sitdev);
> +	if (prg_rc && !rc)
> +		rc = prg_rc;
> +
> +	/*
> +	 * Refresh the mask from the table just written so a get that follows
> +	 * a set does not have to wait for the next poll.  Slots written
> +	 * before a failed write are in the table too, so this is closer to
> +	 * the truth than the pre-write mask either way, and the poll read-back
> +	 * corrects whatever a partial write left behind.
> +	 */
> +	sit9531x_prio_mask_build(sitdev, pll_idx, srcs);

[Severity: High]
If the I2C writes fail and execution jumps to the commit label, is it safe
to unconditionally rebuild the software mask using the intended srcs array?
If the mask reflects a successful write but the hardware doesn't, this could
permanently desynchronize the software state from the hardware and break
rollback error handling on later pin operations.

[ ... ]
> diff --git a/drivers/dpll/sit9531x/dpll.c b/drivers/dpll/sit9531x/dpll.c
> index b4984335e6bfc..ab9aa7db1839c 100644
> --- a/drivers/dpll/sit9531x/dpll.c
> +++ b/drivers/dpll/sit9531x/dpll.c
[ ... ]
> @@ -300,8 +335,144 @@ sit9531x_dpll_input_pin_direction_get(const struct dpll_pin *pin,
> +static int
> +sit9531x_dpll_input_pin_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)
> +{
[ ... ]
> +	switch (state) {
> +	case DPLL_PIN_STATE_DISCONNECTED:
> +		rc = sit9531x_input_prio_remove(sitdev, sitdpll->id, hw_src);
> +		/*
> +		 * The table write, the latch and the holdover release are
> +		 * three steps behind one return code, so ask the table what
> +		 * actually happened rather than reading the errno as "no
> +		 * change".  A source that is gone from the table has been
> +		 * released whatever else failed.
> +		 */
> +		if (rc && sit9531x_input_prio_present(sitdev, sitdpll->id,
> +						      hw_src))
> +			break;
> +		ref->pll_mask &= ~pll_bit;
> +		if (rc)
> +			break;
> +		if (ref->pll_mask)
> +			rc = 0;	/* another DPLL still uses this input */
> +		else
> +			rc = sit9531x_input_disable(sitdev, dpin->id);
> +		break;

[Severity: Medium]
If sit9531x_input_prio_remove() succeeds in updating the hardware table but
fails a subsequent step (like releasing holdover), returning a non-zero rc,
doesn't this skip sit9531x_input_disable() and leave the physical receiver
powered on indefinitely while the software state considers it unused?

> +	case DPLL_PIN_STATE_CONNECTED:
[ ... ]
> +		break;
> +	case DPLL_PIN_STATE_SELECTABLE:
> +		if (!(ref->pll_mask & pll_bit)) {
> +			rc = sit9531x_input_enable(sitdev, dpin->id);
> +			if (rc)
> +				break;
> +		}
> +		rc = sit9531x_input_prio_add(sitdev, sitdpll->id, hw_src);
> +		if (rc && !sit9531x_input_prio_present(sitdev, sitdpll->id,
> +						       hw_src)) {
> +			if (!(ref->pll_mask & pll_bit) && !ref->pll_mask)
> +				sit9531x_input_disable(sitdev, dpin->id);
> +			break;
> +		}
> +		/*
> +		 * Claim the input for this DPLL only once it is both enabled
> +		 * and present in the priority table.  Setting the mask before
> +		 * prio_add would leak the claim if prio_add failed, keeping the
> +		 * shared input receiver powered even after every DPLL released
> +		 * it.
> +		 */
> +		ref->pll_mask |= pll_bit;
> +		break;

[Severity: High]
If sit9531x_input_prio_add() encounters an I2C error but the software mask
was corrupted in sit9531x_prio_table_commit() to look successful, won't
sit9531x_input_prio_present() incorrectly return true? This would bypass
the hardware rollback (sit9531x_input_disable) and incorrectly claim the
receiver in ref->pll_mask.

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