Re: [PATCH v1 08/13] dt-bindings: iio: adc: adi,ad4134: Document SPI connection mode

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [Critical] Unconditional buffer overflow in `ad4134_trigger_handler()` and silent data loss when the timestamp channel is enabled.
- [High] The ad4134 driver ignores unsupported `adi,spi-mode` values and silently falls back to `no-cs` mode without throwing an error.
- [Medium] Adding daisy-chain modes without explicitly allowing the '#daisy-chained-devices' property.
- [Low] The `adi,spi-mode` property describes a default fallback in text but lacks the YAML `default` keyword in the schema.
--

commit 5c85e25862888f2e9a104a75c26404312b5feaf0
Author: Marcelo Schmitt <marcelo.schmitt@analog.com>

dt-bindings: iio: adc: adi,ad4134: Document SPI connection mode

Document how AD4134 chips are connected to the host SPI controller
according to different wiring configurations.

> diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad4134.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad4134.yaml
> index ea6d7e0264190..d843c02a394a3 100644
> --- a/Documentation/devicetree/bindings/iio/adc/adi,ad4134.yaml
> +++ b/Documentation/devicetree/bindings/iio/adc/adi,ad4134.yaml
> @@ -131,6 +131,28 @@ properties:
>      enum: [ free-running, gated ]
>      default: gated
>  
> +  adi,spi-mode:
> +    $ref: /schemas/types.yaml#/definitions/string
> +    enum: [ no-cs, 4-wire, one-channel-chain, two-channel-chain ]

[Severity: Medium]
Does this schema need to explicitly allow the '#daisy-chained-devices'
property, or reference /schemas/spi/spi-peripheral-props.yaml?

Standard devicetree descriptions for daisy-chained IIO devices require
'#daisy-chained-devices' to communicate chain length.

Without it, won't any devicetree using the newly added 'one-channel-chain'
mode fail dt_binding_check validation?

> +    description: |
> +      This property indicates the SPI wiring configuration.
> +
> +      When this property is omitted, it is assumed that the device is using
> +      'no-cs' wiring.

[Severity: Low]
Is it possible to add the YAML 'default: no-cs' keyword to the schema
definition?

Since the text describes a default fallback, structurally representing it
ensures tooling can infer the default configuration.

[ ... ]

I also noticed some regressions in the driver code related to these SPI modes.

In drivers/iio/adc/ad4134.c:ad4134_probe():

	ret = device_property_match_property_string(dev, "adi,spi-mode",
						    ad4134_spi_modes,
						    ARRAY_SIZE(ad4134_spi_modes));
	/* Default to "no-cs" mode if adi,spi-mode is not specified */
	if (ret == -EINVAL)
		st->spi_mode = AD4134_SPI_MODE_NO_CS;

[Severity: High]
Does this code silently ignore unsupported strings?

device_property_match_property_string() returns -EINVAL when the property
exists but contains a string not in the array (such as the newly added
"one-channel-chain" mode).

If the driver doesn't support the chain modes yet, will this incorrectly
configure the SPI host and multiplexer for no-cs minimum I/O mode?

Could this cause hardware protocol violations and garbage data reads during
system initialization when a misconfigured or untrusted DT is provided?


In drivers/iio/adc/ad4134.c:ad4134_trigger_handler():

	for (unsigned int ch = 0; ch < iio_get_masklength(indio_dev); ch++) {
		ret = spi_write_then_read(st->spi, NULL, 0, &st->scan[ch],
...
	iio_push_to_buffers_with_ts(indio_dev, &st->scan, sizeof(st->scan),
				    pf->timestamp);

[Severity: Critical]
Does this loop overflow the st->scan array?

Because ad4134_chan_set includes the soft timestamp channel,
iio_get_masklength(indio_dev) returns 5. The loop will unconditionally iterate
up to ch=4, performing an SPI read into &st->scan[4].

Since st->scan is only sized for 4 channels (u32 scan[AD4134_NUM_CHANNELS]),
won't this result in out-of-bounds memory corruption on every hardware trigger?

Furthermore, when the timestamp channel is enabled, IIO requires a 24-byte
buffer (4x4 bytes + 8-byte timestamp). Because sizeof(st->scan) is 16 bytes,
will iio_push_to_buffers_with_ts() detect the undersized buffer and silently
drop all captured data?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1788368334.git.marcelo.schmitt@analog.com?part=8