Re: [PATCH v3 7/9] iio: adc: ti-ads1262: support triggered buffer sampling

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Race condition between the hardware DRDY interrupt and `reinit_completion()` causes missed conversions and timeouts.
--

--- Patch [7]: [PATCH v3 7/9] iio: adc: ti-ads1262: support triggered buffer sampling ---
commit a9e8c114122dfc9b8eb2cdf234373c01ea35d2cf
Author: Kurt Borja <[email protected]>

iio: adc: ti-ads1262: support triggered buffer sampling

This commit adds triggered buffer support and a data-ready (DRDY) 
hardware trigger for the Texas Instruments ADS1262 and ADS1263 ADCs.

> diff --git a/drivers/iio/adc/ti-ads1262.c b/drivers/iio/adc/ti-ads1262.c
> index d5464b4f2bfb2..24a7ecb9fbd40 100644
> --- a/drivers/iio/adc/ti-ads1262.c
> +++ b/drivers/iio/adc/ti-ads1262.c

[ ... ]

> +static int ads1262_fill_buffer_mult(struct iio_dev *indio_dev)
> +{
> +	struct ads1262 *st = iio_priv(indio_dev);
> +	unsigned int chan;
> +	__be32 val;
> +	int i = -1;
> +	int ret;
> +
> +	/*
> +	 * This routine enables and reads channels in a full-duplex fashion.
> +	 *
> +	 * When a channel is enabled, the previous conversion is clocked out of
> +	 * the shift data register on the same transfer (Section 9.4.7.1). This
> +	 * allows for low latency software sequencing but forbids any
> +	 * communication with the chip in-between or data corruption may occur,
> +	 * hence the need to take the xfer_lock for the whole operation.
> +	 */
> +	guard(mutex)(&st->xfer_lock);
> +
> +	iio_for_each_active_channel(indio_dev, chan) {
> +		ret = ads1262_enable_and_read_last(st, &indio_dev->channels[chan],
> +						   &val);
> +		if (ret)
> +			return ret;
> +
> +		/*
> +		 * After writing to the channel configuration registers, the
> +		 * conversion-cycle is restarted and the data registers are
> +		 * cleared. This means we have to reinit the completion after
> +		 * enabling to avoid reading stale data.
> +		 */
> +		reinit_completion(&st->drdy);

[Severity: High]
Can a fast conversion cause a race condition here that leads to a timeout?

If the sample rate is high (e.g., 38400 SPS, ~26µs latency), is it possible
for the hardware DRDY interrupt to fire immediately after the SPI transfer
in ads1262_enable_and_read_last() finishes, but before reinit_completion()
executes?

If the interrupt handler executes first and calls complete(&st->drdy),
would this reinit_completion() locklessly reset the state back to 0,
wiping out the completion signal?

If so, would the subsequent ads1262_wait_for_conversion() then wait
for an event that was already cleared, eventually returning -ETIMEDOUT?

> +
> +		if (i > -1)
> +			st->scan_buffer[i] = val;
> +		i++;
> +
> +		ret = ads1262_wait_for_conversion(st);
> +		if (ret)
> +			return ret;
> +	}

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=7