Re: [PATCH v10 08/14] dpll: sit9531x: add support to get and set frequency on pins

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] dpll: sit9531x: unconditional NVM commit on I2C write failure corrupts hardware state
--

commit 6cf2285c5820470a1c24c7b915f31d2eb06496d4
Author: Oleg Zadorozhnyi <Oleg.Zadorozhnyi@devoxsoftware.com>

dpll: sit9531x: add support to get and set frequency on pins

This commit introduces frequency getting and setting for the sit9531x
DPLL driver. It handles both input frequency reporting from firmware
and output frequency configuration by writing new dividers to the chip.

> 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_output_freq_set(struct sit9531x_dev *sitdev, u8 out_idx,
> +			     u8 pll_idx, u64 frequency)
> +{
> +	u64 fvco, divo;
> +	int rc, ret;
> +
> +	lockdep_assert_held(&sitdev->multiop_lock);
> +
> +	rc = sit9531x_output_divo_calc(sitdev, out_idx, pll_idx, frequency,
> +				       &fvco, &divo);
> +	if (rc)
> +		return rc;
> +
> +	rc = sit9531x_prg_enter(sitdev);
> +	if (rc)
> +		return rc;
> +
> +	rc = sit9531x_output_divo_write(sitdev, out_idx, divo);
> +	/*
> +	 * 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]
Will this persist corrupted values to non-volatile memory if an I2C bus error
occurs during the divider write?

If a multi-byte I2C write for the output divider fails in
sit9531x_output_divo_write() or its rollback sequence, the error path
unconditionally calls sit9531x_prg_commit() to re-lock the loops.

However, looking at sit9531x_prg_commit(), it unconditionally issues the
SIT9531X_UPDATE_NVM command:

static int sit9531x_prg_commit(struct sit9531x_dev *sitdev)
{
        int rc, rc2 = 0, rc3;
        u8 attempt;

        rc = sit9531x_write_u8(sitdev, SIT9531X_REG_PRG_DIR_GEN,
                               SIT9531X_UPDATE_NVM);
        ...
}

This persists the partially-written divider values into NVM, which
could permanently break the device's default boot configuration. Should this
path call sit9531x_prg_abort() instead when sit9531x_output_divo_write()
returns an error, so the loops can be safely re-locked without issuing an
NVM update?

> +	if (ret && !rc)
> +		rc = ret;
> +	if (rc)
> +		return rc;
> +

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