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

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 12 potential
issue(s) to consider:

Critical: 0 · High: 0 · Medium: 9 · Low: 3

- [Medium] sit9531x_get_fvco() returns 0 both for a register-read failure
  (DIVN registers on the PLL page, or the XO-doubler bit at page 0 reg…
- [Medium] A per-pin frequency set has PLL-wide and NVM side effects that
  the commit message does not disclose: sit9531x_prg_commit() writes…
- [Medium] sit9531x_dpll_output_pin_frequency_get() converts every
  failure of sit9531x_output_freq_get() into success with the stale…
- [Medium] The new input .frequency_get reports sitdev->ref[].freq, which
  prop.c seeds with the *first* entry of the DT 'supported-frequencies-…
- [Medium] prop.c appends an unconditional catch-all supported-frequency
  range of 1 Hz..1 GHz to every ordinary pin, so the freq_supported set…
- [Medium] The new output frequency setter is reachable with values it
  never advertised, because the DPLL core narrows the u64 netlink…
- [Medium] sit9531x_output_phase_flush() sets
  SIT9531X_PLL_PHFL_ON_DEMAND_EN (PLL page reg 0x3D bit 7) and latches…
- [Medium] An I2C failure partway through the five-byte DIVO write leaves
  a mixed old/new 34-bit divider staged, and the common `goto commit`…
- [Medium] Truncation of computed output frequency to `u32` without
  bounds checking.
- [Low] DISCARDED (fixed later in the series).
- [Low] The kernel-doc block of the newly added sit9531x_get_fvco()
  explains it as a fix for 'the previous split between free-run and sync…
- [Low] sit9531x_output_freq_get()'s kernel-doc documents only '0 on
  success, -ENODEV when the output divider or VCO rate is not…

--

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

This adds .frequency_get for input, XO and output pins and .frequency_set
for output pins.  Input frequencies are reported from the cached firmware
description, while output frequencies are computed from the DIVO divider
read back from the chip and programmed by writing a new divider inside the
chip's programming state.

A few questions about the new code below.

> diff --git a/drivers/dpll/sit9531x/core.c b/drivers/dpll/sit9531x/core.c
> index e706f6942f65..721b2c451d59 100644
> --- a/drivers/dpll/sit9531x/core.c
> +++ b/drivers/dpll/sit9531x/core.c
> @@ -339,6 +339,35 @@ static int sit9531x_output_forced_hiz(struct sit9531x_dev *sitdev, u8 slot,
>  	return 0;
>  }
>  
> +/*
> + * Commit a programming sequence started by sit9531x_prg_enter():
> + * update the NVM shadow and re-lock the loops.  The sleep gives the
> + * hardware its required settling time after the loop-lock command;
> + * it is intentional despite the caller holding multiop_lock, as the
> + * whole NVM + lock sequence must be atomic.
> + */
> +static int sit9531x_prg_commit(struct sit9531x_dev *sitdev)
> +{
> +	int rc, rc2;
> +
> +	rc = sit9531x_write_u8(sitdev, SIT9531X_REG_PRG_DIR_GEN,
> +			       SIT9531X_UPDATE_NVM);

[Severity: Medium]
The commit message describes an output frequency set only as "computed from
the divider read back from the chip and set by writing a new one".  Does a
single pin set also carry two side effects the message does not mention?

sit9531x_dpll_output_pin_frequency_set()
  sit9531x_output_freq_set()
    sit9531x_prg_commit()            /* SIT9531X_UPDATE_NVM + msleep(100) */
    sit9531x_output_phase_flush(pll_idx)

The NVM shadow update and the unconditional 100 ms sleep run on every
successful set, and the flush comment states that it makes "every output
divider of @pll_idx restart aligned to the PLL phase".

Does that mean setting the frequency of one output phase-steps every other
output driven by the same PLL, discarding whatever per-output phase adjust
was previously programmed on those siblings?  Could the changelog say that
a per-pin set is PLL-wide and touches the NVM shadow?

> +
> +	/*
> +	 * Issue the loop lock even if the update failed.  Callers reach
> +	 * this function through a goto so that the chip never stays in
> +	 * the PRG_CMD state with its loops open; returning early here
> +	 * would defeat that and leave the outputs unlocked until the
> +	 * next successful commit.
> +	 */
> +	rc2 = sit9531x_write_u8(sitdev, SIT9531X_REG_PRG_DIR_GEN,
> +				SIT9531X_LOOP_LOCK);
> +
> +	msleep(100);
> +
> +	return rc ? rc : rc2;
> +}
> +
>  /*
>   * Input priority selection
>   *

[ ... ]

> @@ -735,6 +769,414 @@ int sit9531x_input_prio_add(struct sit9531x_dev *sitdev, u8 pll_idx,
>  #define SIT9531X_FVCO_HIGHBAND_MIN		6875000000ULL
>  #define SIT9531X_FVCO_HIGHBAND_MAX		7812500000ULL
>  

[ ... ]

> +/*
> + * sit9531x_get_fvco - read VCO frequency from chip's DIVN registers
> + *
> + * Fvco = Fref * DIVN, where DIVN comes from sit9531x_divn_static() and
> + * Fref = xtal_freq << doubler.  DIVN is the
> + * steady-state Fvco/Fref target programmed by the NVM blob and is
> + * authoritative in both free-run and sync modes; the previous split
> + * between free-run and sync formulas returned 0 on chips that didn't
> + * have a sync input populated, which broke the TDC phase readback.
> + *
> + * Return: Fvco in Hz, or 0 on error
> + */

[Severity: Low]
This comment describes "the previous split between free-run and sync
formulas returned 0 on chips that didn't have a sync input populated, which
broke the TDC phase readback".  Which upstream code is that referring to?

This patch adds the first Fvco computation in the driver, so there is no
previous split to have been replaced.  There is also no TDC phase readback
at this point in the series: sit9531x_phase_offset_read() is only declared
in core.h, with no definition in any .c file yet.

Could the paragraph be dropped, or reworded so it does not read as a fix
for a regression that never existed in tree?

> +static u64 sit9531x_get_fvco(struct sit9531x_dev *sitdev, u8 pll_idx)
> +{
> +	int doubler, rc;
> +	s64 divn;
> +	u64 fref;
> +
> +	/*
> +	 * DT board-config override: some configs (e.g. an INTSYNC PLL)
> +	 * run a VCO that Fref*DIVN does not reproduce.  When the board
> +	 * supplies the measured VCO, use it verbatim.
> +	 */
> +	if (pll_idx < SIT9531X_NUM_PLLS && sitdev->pll_fvco[pll_idx])
> +		return sitdev->pll_fvco[pll_idx];
> +
> +	rc = sit9531x_divn_static(sitdev, pll_idx, &divn);
> +	if (rc || divn <= 0)
> +		return 0;
> +
> +	doubler = sit9531x_is_xo_doubler_enabled(sitdev);
> +	if (doubler < 0)
> +		return 0;
> +
> +	fref = (u64)sitdev->xtal_freq << doubler;
> +
> +	return mul_u64_u64_div_u64(fref, (u64)divn, SIT9531X_DIVN_SCALE);
> +}
> +
> +/*
> + * sit9531x_output_phase_flush - flush the output phase of a PLL

[ ... ]

> +static int sit9531x_output_phase_flush(struct sit9531x_dev *sitdev, u8 pll_idx)
> +{
> +	u8 ctrl, orig;
> +	int rc, ret;
> +
> +	/* Arm the on-demand phase-flush on the PLL page. */
> +	rc = sit9531x_update_pll_u8(sitdev, pll_idx,
> +				    SIT9531X_PLL_REG_PHFL_CTRL,
> +				    SIT9531X_PLL_PHFL_ON_DEMAND_EN,
> +				    SIT9531X_PLL_PHFL_ON_DEMAND_EN);
> +	if (rc)
> +		return rc;

[Severity: Medium]
Is SIT9531X_PLL_PHFL_ON_DEMAND_EN meant to stay set after the flush?

sit9531x_output_phase_flush() sets it on the PLL page and latches it with
the small-change update, but no path in the driver clears it again -- a grep
for PHFL in drivers/dpll/sit9531x/ finds only this write site.  Only the
Page 0 trigger-source select is restored to orig at the end.

That leaves the PLL permanently armed, so a later assertion of the restored
(possibly GPIO-sourced) DIVO phase trigger re-flushes every output divider
of that PLL, which is the persistent output phase offset the flush was added
to avoid.

The function's own comment argues the trigger source must not stay pinned
after a one-shot flush; should the other half of the same one-shot sequence
be unwound too, or is the enable intentionally sticky and just missing a
comment saying so?

> +
> +	/* Latch it with the PLL small-change update. */
> +	rc = sit9531x_update_pll_u8(sitdev, pll_idx,
> +				    SIT9531X_PLL_REG_SMALL_UPDATE,
> +				    SIT9531X_SMALL_UPDATE_CMD,
> +				    SIT9531X_SMALL_UPDATE_CMD);
> +	if (rc)
> +		return rc;

[ ... ]

> +	ret = sit9531x_write_u8(sitdev, SIT9531X_REG_GPIO_FUNC_CTRL1, orig);
> +	if (ret && !rc)
> +		rc = ret;
> +
> +	return rc;
> +}
> +
> +/*
> + * sit9531x_output_freq_set - set output clock frequency

[ ... ]

> +int sit9531x_output_freq_set(struct sit9531x_dev *sitdev, u8 out_idx,
> +			     u8 pll_idx, u64 frequency)
> +{

[ ... ]

> +	fvco = sit9531x_get_fvco(sitdev, pll_idx);
> +	if (!fvco) {
> +		fvco = fvco_min;
> +	} else if (!sitdev->pll_fvco[pll_idx]) {
> +		if (fvco < fvco_min)
> +			fvco = fvco_min;
> +		else if (fvco > fvco_max)
> +			fvco = fvco_max;
> +	}

[Severity: Medium]
Is it safe to program a real divider from a guessed VCO rate here?

sit9531x_get_fvco() returns 0 for two very different situations:

	rc = sit9531x_divn_static(sitdev, pll_idx, &divn);
	if (rc || divn <= 0)
		return 0;

	doubler = sit9531x_is_xo_doubler_enabled(sitdev);
	if (doubler < 0)
		return 0;

so an unprogrammed DIVN and a failed regmap read of the DIVN registers or
of the doubler bit are indistinguishable.

In either case this code substitutes the band-edge constant fvco_min, then
DIVO is computed from it, written to the chip, committed to the NVM shadow
and the loops re-locked, and 0 is returned.  For a low-band PLL whose real
VCO is near 5.89824 GHz but whose readback failed, does that not put the
output roughly 20% away from what user space asked for while reporting
success?

The clamp path has the same question: an out-of-band readback is silently
pulled to the band edge rather than reported.

Would returning an error when the VCO cannot be read be preferable, and
should sit9531x_get_fvco() distinguish a bus error from a dormant PLL?

> +
> +	divo = div64_u64(fvco, frequency);
> +	if (!divo)
> +		return -EINVAL;

[Severity: Medium]
Can a request that the driver advertised as supported be satisfied at a
materially different rate here?

sit9531x_pin_props_get() in prop.c appends a catch-all range to every
ordinary pin:

	/* Always append a wide catch-all range */
	ranges[j].min = 1;
	ranges[j].max = 1000000000ULL; /* 1 GHz */

dpll_pin_freq_set() accepts anything inside prop.freq_supported, and this
code only rejects a zero DIVO.  With a low-band Fvco of 4915200000 Hz, a
request for 1 GHz gives DIVO = 4, so the pin emits 1228800000 Hz -- neither
the requested value nor inside the advertised range -- and the call returns
success.  Smaller residues appear everywhere else, e.g. 156250000 Hz becomes
158554838 Hz on the same VCO.

Should either the advertised ranges describe rates the divider can actually
produce, or the setter reject requests it cannot hit exactly?

> +
> +	dev_dbg(sitdev->dev,
> +		"out%u: Fvco=%llu freq=%llu DIVO=%llu (effective %llu Hz)\n",
> +		out_idx, fvco, frequency, divo, div64_u64(fvco, divo));

[ ... ]

> +	divo_bytes[0] = (divo >>  0) & 0xFF;
> +	divo_bytes[1] = (divo >>  8) & 0xFF;
> +	divo_bytes[2] = (divo >> 16) & 0xFF;
> +	divo_bytes[3] = (divo >> 24) & 0xFF;
> +	divo_bytes[4] = (divo >> 32) & 0x03;  /* only bits [1:0] */
> +
> +	rc = sit9531x_read_u8(sitdev,
> +			      SIT9531X_REG(page, base_reg - 4), &msb_old);
> +	if (rc)
> +		goto commit;
> +	divo_bytes[4] |= msb_old & 0xFC;
> +
> +	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:

[Severity: Medium]
What happens to the 34-bit divider when one of these five writes fails
partway through?

The bytes are written one register at a time, so a failure at j > 0 leaves a
mixed old/new DIVO staged, and the shared exit then applies it:

commit:
	ret = sit9531x_prg_commit(sitdev);   /* UPDATE_NVM then LOOP_LOCK */

Does that leave the output running at a frequency nobody requested, with the
NVM shadow updated to match, while the function returns an error and
sitdev->out[out_idx].freq still describes the old rate?

The commit message covers the exit running on failure ("the exit runs even
when a write in the middle failed and the first error is the one returned"),
but not the half-written field.  Would saving and restoring the five
pre-write bytes on the failure path, or abandoning the staged update, be
possible here?

> +	/*
> +	 * 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);
> +	if (ret && !rc)
> +		rc = ret;
> +	if (rc)
> +		return rc;

[ ... ]

> +	sitdev->out[out_idx].freq = (u32)div64_u64(fvco, divo);
> +
> +	return 0;
> +}
> +
> +/*
> + * sit9531x_output_freq_get - read output clock frequency from hardware
> + * @out_idx:	output index (0-N for this chip variant)
> + * @frequency:	output frequency in Hz

[Severity: Medium]
Is a u32 wide enough for the effective rate this line caches?

struct sit9531x_out.freq is a u32, but the value stored here is
fvco / divo, and divo is only rejected when it is zero.  With divo == 1
the effective rate is the raw VCO, 4.9-5.9 GHz on a low-band PLL and
6.9-7.8 GHz on a high-band one, all above U32_MAX (~4.29 GHz), so the
cast silently wraps: a 4915200000 Hz output would be cached as
620232704 Hz.  sit9531x_output_freq_get() does the same thing one
function below with (u32)*frequency, so a read-back of such an output
overwrites the cache with the same wrapped value even though
*frequency itself is correct.

That matters because sit9531x_dpll_output_pin_frequency_get() falls
back to sit9531x_out_state_get(sitdev, dpin->id)->freq whenever the
register read path returns an error, so user space can be handed the
wrapped rate as a live frequency, and dpll_pin_freq_set() compares its
request against that same value.

Would widening sit9531x_out.freq to u64 be the intended fix, or should
sit9531x_output_freq_set() reject requests whose divider would come out
below the smallest DIVO the hardware supports, so a rate the field
cannot represent is never programmed in the first place?

[ ... ]

> + * Return: 0 on success, -ENODEV when the output divider or VCO rate
> + *	   is not resolvable, <0 on register access error
> + */
> +int sit9531x_output_freq_get(struct sit9531x_dev *sitdev, u8 out_idx,
> +			     u64 *frequency)
> +{
> +	const struct sit9531x_chip_info *info = sitdev->info;
> +	u8 slot, page, base_reg, pll_idx, v;
> +	u64 fvco, divo = 0;
> +	int rc, j;
> +
> +	lockdep_assert_held(&sitdev->multiop_lock);
> +
> +	if (out_idx >= info->num_outputs)
> +		return -EINVAL;

[Severity: Low]
The Return: block lists 0, -ENODEV and register access errors, but this
-EINVAL for an out-of-range out_idx is not among them.  Could it be added?

In the same area, the doc block of sit9531x_output_freq_set() numbers a
seven-step write sequence while the in-body comments label the same points
"Step 4" and "Step 5", so the two numberings do not line up.

> +
> +	pll_idx = sitdev->out[out_idx].pll_idx;
> +	if (pll_idx >= SIT9531X_NUM_PLLS)
> +		return -ENODEV;
> +
> +	fvco = sit9531x_get_fvco(sitdev, pll_idx);
> +	if (!fvco)
> +		return -ENODEV;

[ ... ]

> diff --git a/drivers/dpll/sit9531x/dpll.c b/drivers/dpll/sit9531x/dpll.c
> index 67b9fbba9f2e..a7510ca721b6 100644
> --- a/drivers/dpll/sit9531x/dpll.c
> +++ b/drivers/dpll/sit9531x/dpll.c
> @@ -302,6 +302,28 @@ sit9531x_dpll_input_pin_direction_get(const struct dpll_pin *pin,
>  	return 0;
>  }
>  
> +/*
> + * sit9531x_dpll_input_pin_frequency_get - read input pin frequency
> + *
> + * returns cached frequency from DT or last set.
> + */
> +static int
> +sit9531x_dpll_input_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;
> +	const struct sit9531x_ref *ref;
> +
> +	ref = sit9531x_ref_state_get(sitdpll->dev, dpin->id);
> +	*frequency = ref->freq;
> +
> +	return 0;
> +}

[Severity: Medium]
The changelog says "An input's frequency is what the board presents".  Is
ref->freq actually that value when a board lists several rates?

sit9531x_pin_props_get() in prop.c seeds it with the first list entry:

	if (num_freqs > 0) {
		if (dir == DPLL_PIN_DIRECTION_INPUT)
			sitdev->ref[index].freq = (u32)freqs[0];

The generic binding in Documentation/devicetree/bindings/dpll/dpll-pin.yaml
describes supported-frequencies-hz only as a "List of supported frequencies
for this pin, expressed in Hz", so list order carries no current-rate
meaning.

With no input .frequency_set and no hardware measurement, does this report
an arbitrary entry of the supported list as the pin's live frequency, with
no way for user space to correct it?

[Severity: Low]
Separately, does this read past ref[] for the INTSYNC pin at this point in
the series?

sit9531x_dpll_pin_ops_get() returns sit9531x_dpll_input_pin_ops for the
INTSYNC destination pin, whose dpin->id is SIT9531X_INTSYNC_PIN_ID (9),
while sitdev->ref[] is dimensioned SIT9531X_MAX_INPUTS + 1 (9 entries), so
ref[9] aliases out[0].  The error fallback in the output getter below has
the same shape with out[12] aliasing chan[0].

The later series patch "dpll: sit9531x: model the inter-PLL sync net as a
pair of pins" adds dedicated INTSYNC ops tables with no frequency callbacks
and a comment saying exactly this, so the path is gone by the end of the
series -- would it be worth ordering that patch before this one so no commit
in between exposes ref[9] and out[12] as pin frequencies?

>  /*
>   * sit9531x_dpll_input_pin_state_on_dpll_get - get input pin DPLL state
>   *

[ ... ]

> @@ -576,8 +600,79 @@ sit9531x_dpll_output_pin_direction_get(const struct dpll_pin *pin,
>  	return 0;
>  }
>  
> +/*
> + * sit9531x_dpll_output_pin_frequency_get - read output pin frequency
> + *
> + * Reads the DIVO divider back from the chip and computes the live
> + * frequency as Fvco / DIVO.  Falls back to the cached value when the
> + * output is not resolvable through the divider chain (e.g. not mapped
> + * to a PLL), so a netlink dump never fails on such pins.
> + */
> +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;
> +
> +	return 0;
> +}

[Severity: Medium]
The comment justifies the fallback for outputs "not resolvable through the
divider chain (e.g. not mapped to a PLL)", but does this also swallow I2C
and regmap errors?

sit9531x_output_freq_get() propagates the error of sit9531x_read_u8() for
each of the five DIVO byte reads, and sit9531x_get_fvco() reads the DIVN
registers and the doubler bit as well:

		rc = sit9531x_read_u8(sitdev,
				      SIT9531X_REG(page, base_reg - j), &v);
		if (rc)
			return rc;

With every non-zero rc turned into "return 0" plus the stale cached value,
a transient bus failure is reported to user space as a valid live frequency.

Does it also affect the set path?  dpll_pin_freq_set() calls
ops->frequency_get() first and then:

	if (freq == old_freq)
		return 0;

so if the stale cache happens to equal the requested frequency, the set is
reported as successful without frequency_set() ever running.

Would distinguishing -ENODEV (the unresolvable case the comment describes)
from register access errors work here?

> +
> +/*
> + * sit9531x_dpll_output_pin_frequency_set - set output pin frequency
> + *
> + * computes DIVO = Fvco / frequency and writes the
> + * 34-bit output divider to the output system registers via
> + * sit9531x_output_freq_set().
> + */
> +static int
> +sit9531x_dpll_output_pin_frequency_set(const struct dpll_pin *pin,
> +				       void *pin_priv,
> +				       const struct dpll_device *dpll,
> +				       void *dpll_priv, u64 frequency,
> +				       struct netlink_ext_ack *extack)
> +{

[ ... ]

> +	mutex_lock(&sitdev->multiop_lock);
> +	rc = sit9531x_output_freq_set(sitdev, dpin->id, actual_pll,
> +				      frequency);
> +	mutex_unlock(&sitdev->multiop_lock);

[Severity: Medium]
Should this setter sanity-check the upper bound of frequency itself?

The core validates the request narrowed to 32 bits before handing over the
full 64-bit value:

drivers/dpll/dpll_netlink.c:dpll_pin_freq_set() {
	u64 freq = nla_get_u64(a), old_freq;
	...
	if (!dpll_pin_is_freq_supported(pin, freq)) {
	...
	ret = ops->frequency_set(pin, ..., freq, extack);
}

with

static bool dpll_pin_is_freq_supported(struct dpll_pin *pin, u32 freq)

Combined with the 1 Hz..1 GHz catch-all range this driver advertises, does a
request of 4294967297 Hz validate as 1 Hz and then arrive here at full
width?  sit9531x_output_freq_set() only rejects a zero DIVO, so for a
documented band VCO it would compute DIVO = 1 and program the output at the
raw VCO rate of 4.9-7.8 GHz while returning success, and the u32 cache
update would wrap.

The narrowing helper predates this series, but this is the first driver
callback that can act on it.

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260902214030.20955-1-arouhi%40sitime.com