Re: [PATCH net-next v8 08/15] dpll: sit9531x: add support to get and set frequency on pins

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Committing a partially written output divider (DIVO) to active hardware state on I2C error.
- [Medium] Unprotected read of `freq` outside of multiop_lock in `sit9531x_dpll_output_pin_frequency_get`.
- [Medium] Truncation of computed output frequency to `u32` without bounds checking.
--

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

    dpll: sit9531x: add support to get and set frequency on pins
    
    Both directions in one patch, since they share everything that matters.
    
    An input's frequency is what the board presents, so it is reported from
    the firmware description rather than read back: the chip has no divider on
    an input whose rate it merely qualifies.

> diff --git a/drivers/dpll/sit9531x/core.c b/drivers/dpll/sit9531x/core.c
> index e706f6942f654..721b2c451d599 100644
> --- a/drivers/dpll/sit9531x/core.c
> +++ b/drivers/dpll/sit9531x/core.c
[ ... ]
> @@ -735,6 +769,414 @@ int sit9531x_input_prio_add(struct sit9531x_dev *sitdev, u8 pll_idx,
[ ... ]
> +int sit9531x_output_freq_set(struct sit9531x_dev *sitdev, u8 out_idx,
> +			     u8 pll_idx, u64 frequency)
> +{
[ ... ]
> +	for (j = 0; j < 5; j++) {
> +		rc = sit9531x_write_u8(sitdev,
> +				       SIT9531X_REG(page, base_reg - j),
> +				       divo_bytes[j]);
> +		if (rc)
> +			goto commit;
> +	}
> +
> +commit:
> +	/*
> +	 * Step 4: NVM update + loop lock.  Always run prg_commit() so the chip
> +	 * leaves the PRG_CMD state with the output loops re-locked, even when a
> +	 * write above failed; keep the first error to return.  It also carries
> +	 * the required post-lock settling sleep.
> +	 */
> +	ret = sit9531x_prg_commit(sitdev);

[Severity: High]
Does this code commit a partially written hardware state to the active NVM?

If any of the byte-by-byte writes to the 5-byte DIVO register fail, the goto
commit path unconditionally calls sit9531x_prg_commit(). That helper issues
a SIT9531X_UPDATE_NVM command, which transfers the corrupted shadow registers
into the active hardware state, making the output frequency unpredictable.

Can this safely abort the update instead?

> +	if (ret && !rc)
> +		rc = ret;
> +	if (rc)
> +		return rc;
> +
> +	/*
> +	 * Step 5: flush the PLL's output phase so the new DIVO starts
> +	 * aligned instead of keeping the arbitrary phase the divider
> +	 * happened to be at.
> +	 */
> +	rc = sit9531x_output_phase_flush(sitdev, pll_idx);
> +	if (rc)
> +		return rc;
> +
> +	sitdev->out[out_idx].freq = (u32)div64_u64(fvco, divo);

[Severity: Medium]
Could this assignment truncate the actual output frequency?

The function div64_u64(fvco, divo) returns a u64, but the result is cast to a
u32 without any maximum frequency bounds checking. If an extremely high
frequency is requested and divo is evaluated as small, the calculated result
might exceed U32_MAX, resulting in an incorrectly truncated cached frequency.

> +
> +	return 0;
> +}
[ ... ]
> diff --git a/drivers/dpll/sit9531x/dpll.c b/drivers/dpll/sit9531x/dpll.c
> index 67b9fbba9f2e8..a7510ca721b61 100644
> --- a/drivers/dpll/sit9531x/dpll.c
> +++ b/drivers/dpll/sit9531x/dpll.c
[ ... ]
> @@ -576,8 +600,79 @@ sit9531x_dpll_output_pin_direction_get(const struct dpll_pin *pin,
[ ... ]
> +static int
> +sit9531x_dpll_output_pin_frequency_get(const struct dpll_pin *pin,
> +				       void *pin_priv,
> +				       const struct dpll_device *dpll,
> +				       void *dpll_priv, u64 *frequency,
> +				       struct netlink_ext_ack *extack)
> +{
> +	struct sit9531x_dpll_pin *dpin = pin_priv;
> +	struct sit9531x_dpll *sitdpll = dpll_priv;
> +	struct sit9531x_dev *sitdev = sitdpll->dev;
> +	int rc;
> +
> +	mutex_lock(&sitdev->multiop_lock);
> +	rc = sit9531x_output_freq_get(sitdev, dpin->id, frequency);
> +	mutex_unlock(&sitdev->multiop_lock);
> +
> +	if (rc)
> +		*frequency = sit9531x_out_state_get(sitdev, dpin->id)->freq;

[Severity: Medium]
Is it safe to perform this fallback read outside of the multiop_lock?

If sit9531x_output_freq_get() fails, the code drops the lock and reads the
cached freq value. This read can race with concurrent writers updating the
cache under the multiop_lock (e.g., sit9531x_output_freq_set()), potentially
leading to load tearing on 32-bit architectures.

> +
> +	return 0;
> +}

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