Re: [PATCH v1 12/13] iio: adc: ad4134: Support high-speed data capture

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [Critical] Buffer overflow in `ad4134_trigger_handler` when iterating over the scan mask length.
- [High] NULL pointer dereference in `ad4134_offload_buffer_postenable` error path for AD4134_SPI_MODE_NO_CS.
- [High] Incorrect SPI transfer length in 1-lane offload mode causes buffer format mismatch and data corruption.
- [High] Hardware PWM resource leak on driver unbind due to missing disable action.
- [Medium] Data race on `st->odr_hz` in `ad4134_read_raw` due to missing lock.
--

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

iio: adc: ad4134: Support high-speed data capture

This commit adds support for SPI transfer offloading to speed up data
capture, enabling data acquisition at faster sample rates up to 1.496 MSPS.

> diff --git a/drivers/iio/adc/ad4134.c b/drivers/iio/adc/ad4134.c
> index 8453b44ef368f..cf84d2b24417d 100644
> --- a/drivers/iio/adc/ad4134.c
> +++ b/drivers/iio/adc/ad4134.c
[ ... ]
> @@ -454,6 +509,90 @@ static const struct regmap_config ad4134_regmap_config = {
>  	.max_register = AD4134_CH_VREG(ARRAY_SIZE(ad4134_chan_set)),
>  };
>  
> +static int ad4134_update_conversion_rate(struct ad4134_state *st,
> +					 unsigned int freq_Hz)
> +{
[ ... ]
> +	ret = pwm_set_waveform_might_sleep(st->odr_pwm, &odr_wf, false);

[Severity: High]
This starts the ODR PWM hardware, but is there a corresponding cleanup
action or devm callback registered to stop it on driver unbind? Does this
leave the hardware toggling indefinitely if the driver is unbound or if an
error occurs during probe?

> +	if (ret)
> +		return ret;
> +
> +	st->offload_trigger_config = config;
> +	st->odr_wf = odr_wf;
> +	st->odr_hz = DIV_ROUND_UP_ULL(NSEC_PER_SEC, odr_wf.period_length_ns);
> +
> +	return 0;
> +}
[ ... ]
> @@ -537,6 +676,42 @@ static int ad4134_read_raw(struct iio_dev *indio_dev,
>  		*val2 = AD4134_CHAN_PRECISION_BITS - 1;
>  
>  		return IIO_VAL_FRACTIONAL_LOG2;
> +	case IIO_CHAN_INFO_SAMP_FREQ:
> +		*val = st->odr_hz;
> +		return IIO_VAL_INT;

[Severity: Medium]
Is it safe to read st->odr_hz locklessly here? Concurrent writes via
ad4134_write_raw() hold st->lock while modifying this value, which could
lead to a data race if userspace concurrently reads and writes the
sampling frequency sysfs attribute.

> +	default:
> +		return -EINVAL;
> +	}
> +}
[ ... ]
> @@ -588,6 +763,176 @@ static const struct iio_info ad4134_info = {
>  	.debugfs_reg_access = ad4134_debugfs_reg_access,
>  };
>  
> +static const struct iio_info ad4134_offload_info = {
[ ... ]
> +static void ad4134_prepare_offload_msg(struct iio_dev *indio_dev)
> +{
> +	struct ad4134_state *st = iio_priv(indio_dev);
> +	unsigned int base_len = roundup_pow_of_two(BITS_TO_BYTES(AD4134_CHAN_PRECISION_BITS));
> +	unsigned int bpw = AD4134_CHAN_PRECISION_BITS;
> +
> +	st->xfers.cs_off = 1;
> +	st->xfers.bits_per_word = bpw;
> +	st->xfers.len = base_len * st->spi->num_rx_lanes;

[Severity: High]
Since ad4134_scan_masks statically requires all 4 channels to be present
via GENMASK(3, 0), meaning 16 bytes per trigger are required, will this
length calculation of just 4 bytes in 1-lane mode cause the DMA to push
incomplete frames and corrupt the IIO buffer?

> +	if (st->spi->num_rx_lanes > 1)
> +		st->xfers.multi_lane_mode = SPI_MULTI_LANE_MODE_STRIPE;
> +
> +	st->xfers.offload_flags = SPI_OFFLOAD_XFER_RX_STREAM;
> +
> +	spi_message_init_with_transfers(&st->msg, &st->xfers, 1);
> +}
> +
> +static int ad4134_offload_buffer_postenable(struct iio_dev *indio_dev)
> +{
[ ... ]
> +	ret = spi_offload_trigger_enable(st->offload, st->offload_trigger,
> +					 &st->offload_trigger_config);
> +	if (ret)
> +		goto out_unoptimize;
> +
> +	return 0;
> +
> +out_unoptimize:
> +	spi_unoptimize_message(&st->msg);
> +
> +out_set_register_input:
> +	ret2 = ad4134_set_register_access(st);

[Severity: High]
If the SPI mode is AD4134_SPI_MODE_NO_CS, the mux control pointer
st->mux_st[AD4134_DOUT0_INPUT] is never initialized. When jumping to
out_set_register_input on an error path, will ad4134_set_register_access()
blindly dereference the NULL mux state when calling
mux_state_deselect(st->mux_st[AD4134_DOUT0_INPUT]), crashing the kernel?

> +	if (ret2)
> +		dev_err(&st->spi->dev, "reg input select error: %d\n", ret2);
> +
> +	return ret;
> +}
[ ... ]
> @@ -747,12 +1089,30 @@ static int ad4134_probe(struct spi_device *spi)
>  		indio_dev->setup_ops = &ad4134_buffer_setup_ops;
>  	}
>  
> -	ret = devm_iio_triggered_buffer_setup(dev, indio_dev,
> -					      iio_pollfunc_store_time,
> -					      ad4134_trigger_handler,
> -					      NULL);
> -	if (ret)
> -		return ret;
> +	st->offload = devm_spi_offload_get(dev, spi, &ad4134_offload_config);
> +	ret = PTR_ERR_OR_ZERO(st->offload);
> +	/* Fall back to low speed usage when no SPI offload is available. */
> +	if (ret == -ENODEV) {
> +		indio_dev->info = &ad4134_info;
> +		indio_dev->channels = ad4134_chan_set;
> +		indio_dev->num_channels = ARRAY_SIZE(ad4134_chan_set);
> +		ret = devm_iio_triggered_buffer_setup(dev, indio_dev,
> +						      iio_pollfunc_store_time,
> +						      ad4134_trigger_handler,
> +						      NULL);

[Severity: Critical]
When falling back to low speed usage, ad4134_chan_set configures 5 channels
total (4 data channels plus IIO_CHAN_SOFT_TIMESTAMP). Inside
ad4134_trigger_handler(), the code loops up to iio_get_masklength() which
will be 5:

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

But st->scan is only sized for AD4134_NUM_CHANNELS (4 elements). Does this
cause an out-of-bounds write for the 5th channel, corrupting memory?

> +		if (ret)
> +			return ret;
> +	} else if (ret) {
[ ... ]

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