Re: [PATCH v1 3/4] iio: adc: ltc2378: Enable high-speed data capture

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] Missing early return in `ltc2378_offload_buffer_setup()` causes error pointer dereference and breaks fallback mode.
- [High] Unconditional exposure of sysfs attributes causes a NULL pointer dereference on devices without SPI offload.
- [High] Unbounded `do...while` loops querying hardware limits can cause an infinite loop and system hang.
- [High] Hardcoded 4-byte SPI offload transfers conflict with 16-bit IIO buffer allocations, causing data corruption.
- [Medium] Kconfig dependency mismatch prevents enabling the offload feature when the driver is compiled as a module.
--

commit 87ed4789078a187c9262ff17873220fb5108858f
Author: Marcelo Schmitt <marcelo.schmitt@analog.com>

iio: adc: ltc2378: Enable high-speed data capture

Makes use of SPI transfer offloading to speed up data capture, enabling data
acquisition at faster sample rates (up to 2 MSPS).

> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> index 70fec8e3e891f..b5368ee783f7b 100644
> --- a/drivers/iio/adc/Kconfig
> +++ b/drivers/iio/adc/Kconfig
[ ... ]
> +config LTC2378_OFFLOAD_BUFFER
> +	bool "Offloaded data capture with LTC2378"
> +	depends on SPI && LTC2378

LTC2378_OFFLOAD_BUFFER is a bool that depends on LTC2378. Since LTC2378 is a
tristate, if a user compiles the driver as a module, won't Kconfig restrict
this dependent boolean to n? Does this prevent enabling the offload feature
entirely unless the driver is built-in?

> diff --git a/drivers/iio/adc/ltc2378-offload-buffer.c b/drivers/iio/adc/ltc2378-offload-buffer.c
> new file mode 100644
> index 0000000000000..ed09f9a55f93e
> --- /dev/null
> +++ b/drivers/iio/adc/ltc2378-offload-buffer.c
[ ... ]
> +static int ltc2378_update_conversion_rate(struct ltc2378_state *st, int freq_Hz)
> +{
[ ... ]
> +	do {
> +		cnv_wf.duty_length_ns = target;
> +		ret = pwm_round_waveform_might_sleep(st->cnv_trigger, &cnv_wf);
> +		if (ret)
> +			return ret;
> +		target += 10;  /* Increment by PWM duty cycle period */
> +	} while (cnv_wf.duty_length_ns < LTC2378_TCNV_HIGH_NS);

If the physical PWM hardware clamps the duty cycle below LTC2378_TCNV_HIGH_NS
due to hardware limits or resolution, will this loop increment target
indefinitely and stall the CPU? Should there be a maximum iteration limit?

This same concern applies to the SPI trigger offset negotiation loop located
just below this one.

[ ... ]
> +static int ltc2378_prepare_offload_message(struct device *dev,
> +					   struct ltc2378_state *st)
> +{
> +	st->offload_xfer.bits_per_word = st->info->resolution;
> +	/*
> +	 * Ideally, we would ask the offload provider what data word sizes are
> +	 * supported so we could use smaller words for less precise ADCs.
> +	 * Though, the currently available SPI offloading hardware only supports
> +	 * pushing 32-bit sized data elements to DMA memory. Because of that,
> +	 * we hardcode set 4 byte sized transfers.
> +	 */
> +	st->offload_xfer.len = 4;

Since st->offload_xfer.len is hardcoded to 4 bytes here, but
ltc2378_chan->scan_type.storagebits remains at 16 for ADCs with 16-bit
or lower resolution, does this cause a mismatch? Will the IIO core allocate
DMA buffers assuming 2 bytes per sample while the DMA hardware pushes
4 bytes, potentially leading to buffer overruns and data corruption?

[ ... ]
> +int ltc2378_offload_buffer_setup(struct iio_dev *indio_dev, struct spi_device *spi)
> +{
> +	struct ltc2378_state *st = iio_priv(indio_dev);
> +	struct device *dev = &spi->dev;
> +	int ret;
> +
> +	st->offload = devm_spi_offload_get(dev, spi, &ltc2378_offload_config);
> +	ret = PTR_ERR_OR_ZERO(st->offload);
> +	if (ret && ret != -ENODEV)
> +		return dev_err_probe(dev, ret, "failed to get offload\n");
> +
> +	ret = ltc2378_spi_offload_setup(indio_dev, st);

If devm_spi_offload_get() returns -ENODEV, this check bypasses the
dev_err_probe() call but continues to ltc2378_spi_offload_setup() without
returning. 

Could this pass an ERR_PTR(-ENODEV) via st->offload to
devm_spi_offload_trigger_get(), causing a panic or breaking ltc2378_probe()'s
intended fallback to non-offloaded direct mode?

> diff --git a/drivers/iio/adc/ltc2378.c b/drivers/iio/adc/ltc2378.c
> index 7916500c470cf..fdbe919d45d55 100644
> --- a/drivers/iio/adc/ltc2378.c
> +++ b/drivers/iio/adc/ltc2378.c
[ ... ]
> @@ -226,6 +266,9 @@ static int ltc2378_read_raw(struct iio_dev *indio_dev,
>  }
>  
>  static const struct iio_info ltc2378_iio_info = {
> +#ifdef CONFIG_LTC2378_OFFLOAD_BUFFER
> +	.attrs = &ltc2378_offload_attribute_group,
> +#endif
>  	.read_raw = &ltc2378_read_raw,
>  };

If a device probes successfully without SPI offload using standard direct mode,
st->cnv_trigger remains uninitialized. Since these attributes are statically
assigned without an is_visible callback, they remain exposed to userspace.

Could writing to sampling_frequency in sysfs trigger a NULL pointer
dereference when ltc2378_update_conversion_rate() passes the uninitialized
st->cnv_trigger to pwm_round_waveform_might_sleep()?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1779117444.git.marcelo.schmitt1@gmail.com?part=3