Re: [PATCH v2 1/6] iio: adc: ti-ads1298: fix the data read path

[Andy Shevchenko]

On Sat, Oct 24, 2026 at 08:13:30PM -0300, Stephan Costa Barros wrote:
> Fix five issues in how the driver reads samples:
> 
> - cmd_buffer serves both the register accessors, under the regmap lock,
>   and the commands, under the IIO device lock, and shares a cacheline
>   with rx_buffer and tx_buffer. A register access can then rewrite a
>   command on the bus, or corrupt an RDATA transfer in flight on a
>   platform without coherent DMA. Give each path its own buffer, each on
>   its own cacheline.
> 
> - After STOP, the conversion in progress still raises DRDY, so an RDATA
>   transfer can be in flight when the core frees the active scan mask.
>   Track in the driver whether samples go to the buffer, wait in
>   predisable for the last DRDY and its transfer, and push the sample
>   before marking the transfer done. update_scan_mode reset the transfer
>   counter, which let a new transfer be queued over one in flight; wait
>   for it instead.
> 
> - When a single-shot read times out, its transfer may still be in
>   flight, and the next read then wakes up with the old sample. On
>   unbind, nothing waits for the DRDY interrupt or its transfer. Wait for
>   the transfer before a single-shot read, and on teardown disable the
>   interrupt and wait for the last transfer.
> 
> - The return value of spi_async() and the status of the RDATA message
>   are ignored: a failure leaves the transfer counter stuck, and a failed
>   transfer is reported as a sample. Handle both.
> 
> - read_raw takes the sample out of rx_buffer after releasing direct
>   mode, so a buffer enabled in between can overwrite it. Take it while
>   direct mode is still held.
> 
> The first, second and last were found by review and not reproduced.
> The third and fourth were found by review and reproduced on an emulated
> device only.

...

> static void ads1298_rdata_complete(void *context)
>  {
>  	struct iio_dev *indio_dev = context;
>  	struct ads1298_private *priv = iio_priv(indio_dev);
> +	int status = priv->rdata_msg.status;
>  	int scan_index;
>  	u32 *bounce = priv->bounce_buffer;
> +	bool buffered;
>  
> -	if (!iio_buffer_enabled(indio_dev)) {

> +	/* Only changes while no transfer is in flight */
> +	scoped_guard(spinlock_irqsave, &priv->irq_busy_lock)
> +		buffered = priv->buffered;
> +
> +	if (!buffered) {

Strictly speaking you need to assign buffered to make any compiler not being
confused by the scoped_guard(), which cases IIRC we used to have the past.

...

> static int ads1298_buffer_postenable(struct iio_dev *indio_dev)

> -	return ads1298_write_cmd(priv, ADS1298_CMD_START);
> +	scoped_guard(spinlock_irqsave, &priv->irq_busy_lock)
> +		priv->buffered = true;
> +
> +	ret = ads1298_write_cmd(priv, ADS1298_CMD_START);
> +	if (ret) {
> +		scoped_guard(spinlock_irqsave, &priv->irq_busy_lock)
> +			priv->buffered = false;
> +	}
> +
> +	return ret;

Before write_cmd is called the buffered might have been triggered. Is it okay?

...

>  static int ads1298_buffer_predisable(struct iio_dev *indio_dev)
>  {
>  	struct ads1298_private *priv = iio_priv(indio_dev);
> +	int ret, ret2;
> +	int rate;
> +
> +	ret = ads1298_write_cmd(priv, ADS1298_CMD_STOP);
> +
> +	/*
> +	 * The conversion in progress completes and raises DRDY once more.
> +	 * Wait for it and for its transfer before the core tears down the
> +	 * scan.
> +	 */
> +	if (ads1298_get_samp_freq(priv, &rate) >= 0 && rate > 0)
> +		fsleep(2 * DIV_ROUND_UP(USEC_PER_SEC, rate));
> +	ret2 = ads1298_wait_rdata_idle(priv);
>  
> -	return ads1298_write_cmd(priv, ADS1298_CMD_STOP);
> +	scoped_guard(spinlock_irqsave, &priv->irq_busy_lock)
> +		priv->buffered = false;
> +
> +	return ret ?: ret2;
>  }

The code lacks of comments on why in all error cases (either first one, or
second, or both are triggered) it's fine to continue. And what will be the
meaning of the returned value in all three cases?

-- 
With Best Regards,
Andy Shevchenko