Re: [PATCH v10 06/14] dpll: sit9531x: implement input pin state on a DPLL

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] dpll: sit9531x: input receiver gets permanently disabled on partial I2C failure
- [Low] dpll: sit9531x: priority reversal when adding new inputs to empty slots
--

commit dcd68584c337184ceb7b89117cd05260207e1507
Author: Oleg Zadorozhnyi <Oleg.Zadorozhnyi@devoxsoftware.com>
    
dpll: sit9531x: implement input pin state on a DPLL
    
This commit implements input pin state reporting and configuration for the
SIT9531x DPLL driver. It manages references using a priority table to
determine which input is connected and selectable.

> diff --git a/drivers/dpll/sit9531x/core.c b/drivers/dpll/sit9531x/core.c
> --- a/drivers/dpll/sit9531x/core.c
> +++ b/drivers/dpll/sit9531x/core.c

[ ... ]

> +int sit9531x_input_disable(struct sit9531x_dev *sitdev, u8 index)
> +{

[ ... ]

> +	rc = sit9531x_read_u8(sitdev, force_reg, &val);
> +	if (!rc)
> +		rc = sit9531x_write_u8(sitdev, force_reg, val | BIT(pair));
> +
> +	ret = sit9531x_read_u8(sitdev, state_reg, &val);
> +	if (!ret)
> +		ret = sit9531x_write_u8(sitdev, state_reg, val & ~BIT(pair));
> +	if (ret && !rc)
> +		rc = ret;
> +
> +	if (rc)
> +		return rc;
> +
> +	ref->enabled = false;

[Severity: Medium]
Can returning early on a bus error here desynchronize the software cache
from the physical hardware state?

If the I2C write to assert the FORCE bit succeeds, but the subsequent read
or write to the STATE bit fails, the function returns an error without
setting ref->enabled = false. If the hardware default for STATE was 0, the
receiver is now physically disabled.

This becomes problematic in sit9531x_dpll_input_pin_state_on_dpll_set():

> diff --git a/drivers/dpll/sit9531x/dpll.c b/drivers/dpll/sit9531x/dpll.c
> --- a/drivers/dpll/sit9531x/dpll.c
> +++ b/drivers/dpll/sit9531x/dpll.c

[ ... ]

> +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)
> +{

[ ... ]

> +	case DPLL_PIN_STATE_SELECTABLE:
> +		/*
> +		 * Gate the receiver on whenever it is off, not only when this
> +		 * DPLL holds no claim yet.  The two are tracked separately --
> +		 * the claim comes from the priority table, the receiver from
> +		 * the force bits -- so a PLL that already lists the input can
> +		 * still find it powered down, and skipping the enable would
> +		 * report success for a reference that cannot reach the loop.
> +		 */
> +		if (!ref->enabled) {
> +			rc = sit9531x_input_enable(sitdev, dpin->id);
> +			if (rc)
> +				break;
> +			enabled_here = true;
> +		}

[Severity: Medium]
When userspace later tries to connect or make the pin selectable, this block
will see the stale ref->enabled cache and skip re-enabling it.

Will this leave the physical input receiver permanently disabled, blocking
the signal to all PLLs until the module is reloaded?

> diff --git a/drivers/dpll/sit9531x/core.c b/drivers/dpll/sit9531x/core.c
> --- a/drivers/dpll/sit9531x/core.c
> +++ b/drivers/dpll/sit9531x/core.c

[ ... ]

> +int sit9531x_input_prio_add(struct sit9531x_dev *sitdev, u8 pll_idx,
> +			    u8 input_idx)
> +{

[ ... ]

> +	/*
> +	 * A slot whose code names no usable source -- a reserved value, or
> +	 * one past the inputs this variant has -- is free space, and taking
> +	 * it costs nothing.  Prefer it over displacing a real reference.
> +	 */
> +	for (slot = SIT9531X_PRIO_MAX_SLOTS; slot-- > 0;) {
> +		src = srcs[slot] & SIT9531X_PRIO_NIBBLE_MASK;
> +		if (!sit9531x_prio_src_usable(src)) {
> +			replace = slot;
> +			break;
> +		}
> +	}

[Severity: Low]
Does this backward loop assign new inputs to the lowest available priority
instead of the highest?

When the driver adds multiple sources to a priority table containing
multiple empty slots (e.g., after the table was cleared), the backward loop
selects the highest-index empty slot first.

This results in the first added input getting the lowest priority (highest
slot index), and subsequent inputs getting increasingly higher priorities,
which reverses the expected FIFO addition order.

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