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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] The `rx` buffer in `struct ads1262` lacks the `__aligned(IIO_DMA_MINALIGN)` attribute, causing it to be placed at an unaligned memory offset and sharing a cacheline with the `tx` buffer.
- [High] The driver fails to filter out the IIO soft timestamp channel when iterating over `active_scan_mask`, treating it as a hardware channel. This leads to out-of-bounds array reads and writes, including a potential `FORTIFY_SOURCE` kernel panic.
- [High] The driver dynamically modifies a pre-optimized SPI transfer buffer without cache synchronization, breaking DMA coherency on architectures without hardware cache coherency.
--

commit 21b4fc84b4dd63fd1e3310ee7b41df30e8039785
Author: Kurt Borja <kuurtb@gmail.com>
Subject: 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 04566d00d7cb8..3f6d34a08b296 100644
> --- a/drivers/iio/adc/ti-ads1262.c
> +++ b/drivers/iio/adc/ti-ads1262.c
[ ... ]
> @@ -232,6 +239,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 the rx buffer need the __aligned(IIO_DMA_MINALIGN) attribute as well?

Without it, rx might be placed at an unaligned memory offset and share a
cacheline with the tx buffer. Since they are mapped for DMA in opposite
directions (DMA_TO_DEVICE and DMA_FROM_DEVICE), mapping overlapping
cachelines can violate the DMA API and cause memory corruption on
non-cache-coherent architectures.

>  };
>  
>  static const char * const ads1262_device_id_to_name[] = {
[ ... ]
> @@ -837,10 +849,284 @@ static const struct iio_info ads1262_iio_info = {
[ ... ]
> +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]
Is it possible for this loop to iterate over the IIO soft timestamp channel?

If the timestamp channel is enabled and its bit is set in active_scan_mask,
iio_for_each_active_channel() will yield the timestamp channel's index.

This would cause ads1262_channel_prep_tx() to perform an out-of-bounds read
on the st->channels array since the timestamp channel is treated as a
hardware channel:

	struct ads1262_channel *chan = &st->channels[spec->scan_index];

Additionally, the extra loop iteration could cause the following memcpy to
write past the bounds of the scan_buffer array when all channels are active:

	memcpy(&st->scan_buffer[i], st->rx, sizeof(st->scan_buffer[i]));

> +
> +		reinit_completion(&st->drdy);
> +
> +		ret = spi_sync(st->spi, &st->msg);

[Severity: High]
Could this sequence break DMA coherency on architectures without hardware
cache coherency?

If the SPI message is permanently mapped for DMA in
ads1262_buffer_postenable_mult() via spi_optimize_message():

	return spi_optimize_message(st->spi, &st->msg);

And then on every trigger event, ads1262_channel_prep_tx() dynamically writes
new register commands directly into the st->tx buffer from the CPU:

	st->tx[0] = ADS1262_MODE0_REG | ADS1262_OPCODE_WREG;

When ads1262_fill_buffer_mult() later calls spi_sync(), the message is
already marked as pre-optimized, so dma_sync_single_for_device() might
be skipped. Will the SPI controller fetch stale cached data instead of
the newly written configuration commands?

> +		if (ret)
> +			return ret;
[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260828-ads126x-v4-0-1dc27e9c0260@gmail.com?part=8