Re: [PATCH net-next v8 04/15] dpll: sit9531x: read DPLL types and pin properties from system firmware

[netdev-bot+sashiko]

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

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

- [Medium] sit9531x_pin_props_get() fabricates the pin's advertised
  frequency capability set instead of deriving it from firmware: after…
- [Low] Concern (largely INVALID at the end of the series): the per-pin
  'hardware state' (sitdev->ref[].freq / sitdev->out[].freq and the…
- [Low] sit9531x_pin_props_get() accepts two 'connection-type' spellings
  that the device-tree binding forbids: it compares the property against…
- [Low] Concern (verified as practically unreachable): supported-
  frequencies-hz is a 64-bit DT property and dpll_pin_frequency.min/max…
- [Low] The XO (crystal) input pin has two conflicting sources of truth
  for its frequency.
- [Low] sit9531x_pin_props_get() ignores the return value of
  fwnode_property_read_u64_array() for "supported-frequencies-hz": it…

--

dpll: sit9531x: read DPLL types and pin properties from system firmware

This patch adds drivers/dpll/sit9531x/prop.c, which builds the per-pin
dpll_pin_properties (package label, type, capabilities, supported
frequency list) and the per-channel dpll_type from the firmware node,
falling back to defaults when the node does not describe them.  A few
questions below about the frequency list and the connection-type parsing.

> diff --git a/drivers/dpll/sit9531x/prop.c b/drivers/dpll/sit9531x/prop.c
> new file mode 100644
> index 000000000000..94f3da716297
> --- /dev/null
> +++ b/drivers/dpll/sit9531x/prop.c
> @@ -0,0 +1,406 @@

[ ... ]

> +	} else if (dir == DPLL_PIN_DIRECTION_INPUT &&
> +		   index == SIT9531X_MAX_INPUTS) {
> +		/* The XO reference is fixed: no state or priority control. */
> +		props->dpll_props.type = DPLL_PIN_TYPE_INT_OSCILLATOR;
> +		props->dpll_props.capabilities = 0;
> +		sitdev->ref[index].freq = sitdev->xtal_freq;
> +		curr_freq = sitdev->xtal_freq;

[Severity: Low]
Does the XO pin end up with two sources of truth for its frequency here?

The branch seeds sitdev->ref[SIT9531X_MAX_INPUTS].freq from
sitdev->xtal_freq (the clk_get_rate() value read at probe), but unlike the
two INTSYNC pins the XO index is not excluded from the firmware-node path
further down in sit9531x_pin_props_get().

dpll-device.yaml allows an input-pins child for this index:

    patternProperties:
      "^pin@[0-9a-f]+$":
        $ref: /schemas/dpll/dpll-pin.yaml

so a schema-valid DT can supply supported-frequencies-hz for pin 8, and the
seeding block later overwrites ref[8].freq (and curr_freq) with freqs[0].
>From then on sit9531x_dpll_input_pin_frequency_get() reports one crystal
rate to userspace while all the divider and VCO math keeps using
sitdev->xtal_freq.

Would it be simpler to skip the frequency-seeding path for index
SIT9531X_MAX_INPUTS the same way both INTSYNC pins already are?

[ ... ]

> +	/* Look for "connection-type" property -> pin type enum */
> +	if (!fwnode_property_read_string(props->fwnode, "connection-type",
> +					 &type)) {
> +		if (!strcmp(type, "ext"))
> +			props->dpll_props.type = DPLL_PIN_TYPE_EXT;
> +		else if (!strcmp(type, "gnss"))
> +			props->dpll_props.type = DPLL_PIN_TYPE_GNSS;
> +		else if (!strcmp(type, "int") ||
> +			 !strcmp(type, "int-oscillator"))
> +			props->dpll_props.type = DPLL_PIN_TYPE_INT_OSCILLATOR;
> +		else if (!strcmp(type, "synce") ||
> +			 !strcmp(type, "synce-eth-port"))
> +			props->dpll_props.type = DPLL_PIN_TYPE_SYNCE_ETH_PORT;

[Severity: Low]
Can the "int-oscillator" and "synce-eth-port" comparisons ever match?

Documentation/devicetree/bindings/dpll/dpll-pin.yaml restricts the property
to a closed set:

  connection-type:
    description: Connection type of the pin
    $ref: /schemas/types.yaml#/definitions/string
    enum: [ext, gnss, int, mux, synce]

and sitime,sit95316.yaml inherits that pin schema through dpll-device.yaml.
A DT using either alias fails dt_binding_check/dtbs_check, so those two
comparisons look like dead code for any schema-valid DT.  Should they just
be dropped, or should the binding gain the aliases?

> +	num_freqs = fwnode_property_count_u64(props->fwnode,
> +					      "supported-frequencies-hz");
> +	if (num_freqs <= 0) {
> +		num_freqs = 0;
> +		goto skip_fwnode_props;
> +	}
> +
> +	freqs = kcalloc(num_freqs, sizeof(*freqs), GFP_KERNEL);
> +	if (!freqs) {
> +		rc = -ENOMEM;
> +		goto err_alloc_freqs;
> +	}
> +
> +	fwnode_property_read_u64_array(props->fwnode,
> +				       "supported-frequencies-hz",
> +				       freqs, num_freqs);

[Severity: Low]
Should the return value of fwnode_property_read_u64_array() be checked
before freqs[] is consumed?

For device tree the element count and the array read parse the same
property length and cannot disagree.  For an ACPI-described device (this
driver can still be instantiated through PRP0001, which matches on
of_match_table and takes properties from _DSD), acpi_data_prop_read()
returns the package element count without type-checking each element, while
acpi_copy_property_array_uint() fails with -EPROTO on the first
non-integer element.

Since the failure is discarded, the remaining kcalloc zeros are then used
as data: ref[index].freq / out[index].freq and curr_freq get freqs[0], and
every entry including the zeros is published as a supported range.

> +	/*
> +	 * Seed the runtime ref->freq / out->freq with the first DT-listed
> +	 * supported frequency so the netlink frequency_get callback reports
> +	 * a sane initial value before any pin_set occurs.  DT lists the
> +	 * physically-wired reference frequency for each input pin and the
> +	 * default output frequency for each output pin.
> +	 */
> +	if (num_freqs > 0) {
> +		if (dir == DPLL_PIN_DIRECTION_INPUT)
> +			sitdev->ref[index].freq = (u32)freqs[0];
> +		else
> +			sitdev->out[index].freq = (u32)freqs[0];
> +		curr_freq = (u32)freqs[0];
> +	}
> +
> +skip_fwnode_props:

[ ... ]

> +	/* Allocate frequency ranges list -- DT discrete entries + current
> +	 * freq + one catch-all wide range so the subsystem never pre-
> +	 * rejects a frequency_set call.  The chip's real admissible set
> +	 * is bounded by VCO / divider math in sit9531x_output_freq_set().
> +	 */
> +	ranges = kcalloc(num_freqs + 2, sizeof(*ranges), GFP_KERNEL);
> +	if (!ranges) {
> +		rc = -ENOMEM;
> +		goto err_alloc_ranges;
> +	}
> +
> +	/* Current freq as first entry */
> +	ranges[0] = (struct dpll_pin_frequency)DPLL_PIN_FREQUENCY(curr_freq);
> +	j = 1;

[ ... ]

> +	/* Always append a wide catch-all range */
> +	ranges[j].min = 1;
> +	ranges[j].max = 1000000000ULL; /* 1 GHz */
> +	j++;
> +
> +	props->dpll_props.freq_supported = ranges;
> +	props->dpll_props.freq_supported_num = j;

[Severity: Medium]
Does the appended catch-all range make the firmware-declared frequency list
meaningless, and does it publish a capability set the driver cannot honour?

The core treats every advertised range as genuinely supported, both when
validating a set and when exporting the list to userspace:

drivers/dpll/dpll_netlink.c:dpll_pin_is_freq_supported() {
	for (fs = 0; fs < pin->prop.freq_supported_num; fs++)
		if (freq >= pin->prop.freq_supported[fs].min &&
		    freq <= pin->prop.freq_supported[fs].max)
			return true;
	return false;
}

and it is the only generic pre-validation of a userspace request:

drivers/dpll/dpll_netlink.c:dpll_pin_freq_set() {
	if (!dpll_pin_is_freq_supported(pin, freq)) {
		NL_SET_ERR_MSG_ATTR(extack, a, "frequency is not supported by the device");
		return -EINVAL;
	}

With {1, 1000000000} counted in freq_supported_num for every non-INTSYNC
pin, the DT-listed entries never constrain anything, and the same range is
attached to the XO pin whose capabilities are deliberately 0 and to every
input pin, which has no .frequency_set at all.

The array is also copied into the pin by dpll_pin_prop_dup() and exported
through DPLL_A_PIN_FREQUENCY_SUPPORTED, so 1 Hz..1 GHz becomes what
userspace is told this device supports.

Separately, ranges[0] is always DPLL_PIN_FREQUENCY(curr_freq), and
curr_freq stays 0 for any pin with no firmware node or no
supported-frequencies-hz list (both permitted by dpll-device.yaml and
dpll-pin.yaml), so those pins advertise 0 Hz as a supported frequency.

The commit message says:

  "which frequencies a board actually presents on a given input, and
  whether an output's embedded sync is wired to be controllable, are
  board facts -- so they come from the firmware node"

Is that consistent with the catch-all range, which overrides the board
facts for every pin?

For reference, Documentation/devicetree/bindings/dpll/dpll-pin.yaml
describes the property as the pin's supported set:

  supported-frequencies-hz:
    description: List of supported frequencies for this pin, expressed in Hz.

Would it work to advertise only the firmware-listed entries (plus the
current frequency) and let sit9531x_output_freq_set() reject the rest with
-EINVAL, rather than widening the advertised set?

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