Re: [PATCH v5 08/10] iio: adc: ti-ads1262: support triggered buffer sampling

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] spi: DMA API violation due to cacheline sharing between tx and rx buffers
- [High] iio: Out-of-bounds array access on soft timestamp channel in trigger handlers
--

commit 498de2a7a709d846f73f3af6fbc4fac307b5368e
Author: Kurt Borja <kuurtb@gmail.com>

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

Add triggered buffer support and a data-ready (DRDY) hardware trigger.

> diff --git a/drivers/iio/adc/ti-ads1262.c b/drivers/iio/adc/ti-ads1262.c
> index 49d4bf206be07..0606e712feec8 100644
> --- a/drivers/iio/adc/ti-ads1262.c
> +++ b/drivers/iio/adc/ti-ads1262.c
[ ... ]
> @@ -233,6 +240,11 @@ struct ads1262 {
>  	u32 rref_ohms[ADS1262_EXT_REF_COUNT][ADS1262_EXT_REF_COUNT];
>  	int refp_uV[ADS1262_EXT_REF_COUNT];
>  	int refn_uV[ADS1262_EXT_REF_COUNT];
> +	IIO_DECLARE_BUFFER_WITH_TS(__be32, scan_buffer,
> +				   ADS1262_FW_CHANNEL_COUNT +
> +				   ADS1262_MON_CHANNEL_COUNT);
> +	u8 tx[ADS1262_SPI_XFER_SZ] __aligned(IIO_DMA_MINALIGN);
> +	u8 rx[ADS1262_SPI_XFER_SZ];
        ^^^^^^^^^^^^^^^^^^^^^^^^^^

[Severity: High]
Does this code risk a DMA API violation on non-coherent architectures?

The compiler places rx immediately after tx at offset 13. While tx has the
__aligned(IIO_DMA_MINALIGN) attribute, its size (13 bytes) is not padded
to the cacheline boundary, causing rx to share the same cacheline as tx.

During SPI transfers, both buffers are mapped for DMA concurrently.
Unmapping rx (which invalidates the CPU cache for that line) can silently
destroy adjacent CPU writes or unflushed data in tx, leading to data
corruption and triggering CONFIG_DMA_API_DEBUG warnings.

Should rx also be explicitly marked with __aligned(IIO_DMA_MINALIGN)?

[ ... ]
> +static void ads1262_channel_prep_tx(struct ads1262 *st,
> +				    const struct iio_chan_spec *spec)
> +{
> +	struct ads1262_channel *chan = &st->channels[spec->scan_index];
[ ... ]
> +static int ads1262_fill_buffer_mult(struct iio_dev *indio_dev)
> +{
> +	struct ads1262 *st = iio_priv(indio_dev);
> +	unsigned int chan;
> +	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) {
> +		ads1262_channel_prep_tx(st, &indio_dev->channels[chan]);
                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

[Severity: High]
Can this code read out-of-bounds when chan is the soft timestamp channel?

When triggered buffer support is set up, the IIO core automatically
appends a software timestamp channel to the indio_dev->channels array
and reserves a bit for it in the active scan mask.

The loop over iio_for_each_active_channel() does not skip the IIO_TIMESTAMP
channel. When ads1262_channel_prep_tx() is called for the timestamp channel,
it accesses st->channels[spec->scan_index]. Because the timestamp channel's
scan_index is numerically greater than the hardware channels, it indexes
out-of-bounds of the driver's allocated st->channels array.

The driver then sends configuration data read from this out-of-bounds memory
to the ADC and blocks on wait_for_completion_interruptible_timeout(). Since
the ADC was improperly configured, the expected DRDY interrupt may never
arrive, causing a timeout.

Does this same issue exist in ads1262_buffer_postenable_one() if only the
timestamp channel is enabled?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261010-ads126x-v5-0-49e331cb2fed@gmail.com?part=8