Re: [PATCH net-next v8 15/15] dpll: sit9531x: allow the device tree to override two board facts

[netdev-bot+sashiko]

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

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

- [Medium] sit9531x_parse_board_config()
  (drivers/dpll/sit9531x/core.c:3193-3195) copies the DT array…
- [Medium] Every failure path in the new sit9531x_parse_board_config()
  (drivers/dpll/sit9531x/core.c:3166-3196) is silent, so a board that…
- [Medium] The new parser accepts "sitime,pll-fvco" values with no
  plausibility check (drivers/dpll/sit9531x/core.c:3171-3172),…
- [Low] The commit message's justification for the new "sitime,pll-fvco"
  DT property does not match the code it activates.

--

dpll: sit9531x: allow the device tree to override two board facts

The patch adds sit9531x_parse_board_config(), which reads two optional
vendor properties at probe time: "sitime,pll-fvco" (per-PLL VCO frequency)
and "sitime,output-pll-map" (per-output source PLL).  Absent both
properties the driver keeps deriving Fvco from DIVN and reading the routing
from the OUT_MAP registers.

A few questions below, one about the changelog and three about the new
parser.

> dpll: sit9531x: allow the device tree to override two board facts
>
>     The VCO frequency is derived from the feedback divider, which is exact
>     while the loop runs but not while a PLL sits in free-run with a divider
>     the configuration never programmed; a board that knows its own VCO can
>     state it.

[Severity: Low]
Is the free-run rationale here consistent with the code the property feeds?

The kernel-doc above sit9531x_get_fvco() says the opposite about free-run:

  * 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 override's own comment in sit9531x_get_fvco() and the binding text name
INTSYNC, not free-run, as the case where the derivation does not reproduce
the VCO:

	/*
	 * 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.
	 */

Could the changelog name the configuration where Fref*DIVN actually fails?

The changelog also says "Absent the properties the driver behaves exactly as
before", which is accurate, but it does not mention that when the property
is present sit9531x_output_freq_set() stops clamping Fvco to the per-PLL
band:

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

Should that clamp bypass be called out, given this adds a permanent
vendor-specific DT ABI?

> diff --git a/drivers/dpll/sit9531x/core.c b/drivers/dpll/sit9531x/core.c
> index c3d7c4851549..d75ac02047b5 100644
> --- a/drivers/dpll/sit9531x/core.c
> +++ b/drivers/dpll/sit9531x/core.c
> @@ -3158,6 +3158,43 @@ static u64 sit9531x_derive_clock_id(struct sit9531x_dev *sitdev)
>  	return clkid;
>  }
>  
> +/*
> + * Board-config overrides for fixed efuse/blob routing the chip registers do
> + * not describe unambiguously.  Absent properties leave pll_fvco[] zeroed
> + * (derive from DIVN) and out_pll_map_valid false (use the OUT_MAP registers).
> + */
> +static void sit9531x_parse_board_config(struct sit9531x_dev *sitdev)
> +{
> +	u32 map[SIT9531X_MAX_OUTPUTS];
> +	int n, i;
> +
> +	device_property_read_u64_array(sitdev->dev, "sitime,pll-fvco",
> +				       sitdev->pll_fvco, SIT9531X_NUM_PLLS);

[Severity: Medium]
Should the return value of this read be checked?

device_property_read_u64_array() requires exactly SIT9531X_NUM_PLLS
elements, so a property with 1..3 entries returns -EOVERFLOW and writes
nothing.  pll_fvco[] then stays zero and sit9531x_get_fvco() silently falls
back to the derivation:

	if (pll_idx < SIT9531X_NUM_PLLS && sitdev->pll_fvco[pll_idx])
		return sitdev->pll_fvco[pll_idx];

A board that states its VCO incorrectly is therefore indistinguishable from
a board that states nothing, with nothing in dmesg.  Would a dev_warn() (or
dev_err_probe()) on a non-zero return be preferable?

[Severity: Medium]
Can an out-of-range Fvco here silently misprogram DIVO?

The value is stored without any plausibility check, sit9531x_get_fvco()
returns it verbatim, and sit9531x_output_freq_set() skips the band clamp
for it, so fvco is no longer bounded by SIT9531X_FVCO_HIGHBAND_MAX.  DIVO
is then only checked for zero:

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

but it is packed into a 34-bit field with the upper bits masked away:

	divo_bytes[4] = (divo >> 32) & 0x03;  /* only bits [1:0] */

Before this patch fvco was always clamped to <= 7.8125 GHz, so divo stayed
below 2^34 for any frequency >= 1 Hz and the mask could not lose anything.
With an unclamped DT Fvco (say a units typo giving 69 GHz) and a low output
rate reachable through the esync path, divo >= 2^34 is truncated, written,
and committed by sit9531x_prg_commit() with a phase flush, while
sitdev->out[out_idx].freq is cached from the untruncated divo and the call
returns success.

Would bounding the DT Fvco (and adding minimum/maximum to the schema, which
currently states the bands only in prose), or rejecting divo >= BIT_ULL(34)
instead of truncating, close that?

> +
> +	if (!device_property_present(sitdev->dev, "sitime,output-pll-map"))
> +		return;
> +
> +	/*
> +	 * Any 1..MAX_OUTPUTS length is accepted so the 8-output SiT95317 need
> +	 * not pad to 12; variant detection has not run yet and entries past
> +	 * the detected num_outputs are never indexed.  Trailing entries of a
> +	 * short map must read as unmapped rather than 0 (== PLLA), which
> +	 * would mark unrouted outputs active in sit9531x_out_state_fetch().
> +	 */
> +	memset(sitdev->out_pll_map, SIT9531X_OUT_PLL_UNMAPPED,
> +	       sizeof(sitdev->out_pll_map));
> +
> +	n = device_property_count_u32(sitdev->dev, "sitime,output-pll-map");
> +	if (n <= 0 || n > SIT9531X_MAX_OUTPUTS ||
> +	    device_property_read_u32_array(sitdev->dev, "sitime,output-pll-map",
> +					   map, n))
> +		return;

Related to the previous comment on silent failures: all three rejection
cases here return with out_pll_map_valid still false and no diagnostic, so
sit9531x_out_state_fetch() goes back to the OUT_MAP registers the board just
declared wrong.

Since the parse runs before sit9531x_read_variant_id() and
sit9531x_match_variant(), n also cannot be compared against
info->num_outputs.  A 12-output SiT95316 node carrying only 8 entries is
accepted, outputs 8..11 stay SIT9531X_OUT_PLL_UNMAPPED, and four DPLL pins
never appear to userspace even though the binding's minItems: 12
conditional forbids that DT.  Would moving the parse after variant
detection let the length be validated?

> +
> +	for (i = 0; i < n; i++)
> +		sitdev->out_pll_map[i] = map[i];
> +	sitdev->out_pll_map_valid = true;
> +}

[Severity: Medium]
Should the element values be validated before they are narrowed into the u8
out_pll_map[]?

The binding states the values exhaustively:

    items:
      enum: [0, 1, 2, 3, 255]

and the kernel-doc of struct sit9531x_dev repeats it:

 * @out_pll_map:	optional per-output source PLL (0-3, 0xff =
 *			unmapped) from DT "sitime,output-pll-map"

Neither is enforced at runtime, and out_pll_map_valid is set
unconditionally.  Two results follow in sit9531x_out_state_fetch():

	if (sitdev->out_pll_map_valid) {
		u8 m = sitdev->out_pll_map[index];

		if (m < SIT9531X_NUM_PLLS) {
			out->pll_idx = m;
			out->routed = true;
			out->enabled = !muted;
		} else {
			out->pll_idx = 0;
			out->routed = false;
			out->enabled = false;
		}
		return 0;
	}

An entry in 4..254 becomes "unmapped", so an output that is not muted loses
its DPLL pin and userspace can no longer see, mute or retune a clock that is
actively driving.

An entry >= 0x100 whose low byte is 0..3 (0x100, say) truncates to a
valid-looking index, passes m < SIT9531X_NUM_PLLS, and marks the output
routed on a PLL that does not drive it.  That index is then the driver's
only routing model: it picks the VCO band in sit9531x_output_freq_set()

	if (pll_idx == 1 || pll_idx == 3) {
		/* PLLB, PLLD: high band */
		fvco_min = SIT9531X_FVCO_HIGHBAND_MIN;

and selects the PLL passed to sit9531x_output_phase_flush(), which disturbs
every output of that unrelated PLL.

Isn't that the same failure the 0xFF pre-fill comment above says it
prevents ("would mark unrouted outputs active in
sit9531x_out_state_fetch()")?  Would rejecting, or storing as
SIT9531X_OUT_PLL_UNMAPPED with a warning, any entry that is neither
< SIT9531X_NUM_PLLS nor SIT9531X_OUT_PLL_UNMAPPED cover both cases?

[ ... ]

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