Re: [PATCH 5/9] iio: adc: rzt2h: implement DMA buffer support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] The cyclic DMA buffer read pointer (`period_index`) permanently loses synchronization with the hardware write pointer if the consumer loop aborts early.
- [Medium] Missing dma_sync_single_for_device() call after CPU accesses non-coherent DMA memory.
- [Medium] Kthread will enter a 100% CPU infinite loop if it receives a signal.
--

commit 39a43c5f71d2917000960d73a8ad0233a9d65f07
Author: Cosmin Tanislav <cosmin-gabriel.tanislav.xa@renesas.com>
    
    iio: adc: rzt2h: implement DMA buffer support

This commit implements buffered capture using a cyclic DMA transfer into
a kfifo buffer for the RZ/T2H ADC. It allocates a non-coherent DMA buffer
and handles periodic data pushes via a kthread.

> diff --git a/drivers/iio/adc/rzt2h_adc.c b/drivers/iio/adc/rzt2h_adc.c
> index 95bcebdc02cb3..d76226375f220 100644
> --- a/drivers/iio/adc/rzt2h_adc.c
> +++ b/drivers/iio/adc/rzt2h_adc.c

[ ... ]

> @@ -151,6 +186,253 @@ static int rzt2h_adc_calibrate(struct rzt2h_adc *adc)
>  	return 0;
>  }
>  
> +static void rzt2h_adc_push_period(struct iio_dev *indio_dev, u16 *period,
> +				  dma_addr_t addr)
> +{
> +	struct rzt2h_adc *adc = iio_priv(indio_dev);
> +	u16 *dst = adc->buf;
> +	u16 *src = period;
> +
> +	dma_sync_single_for_cpu(adc->dev, addr, adc->dma.period_bytes,
> +				DMA_FROM_DEVICE);
> +
> +	for (unsigned int sample = 0; sample < RZT2H_ADC_DMA_PERIOD_SAMPLES; sample++) {
> +		for (unsigned int i = 0; i < adc->dma.gather_len; i++)
> +			dst[i] = src[adc->dma.gather[i]];
> +
> +		src += adc->dma.sample_chans;
> +
> +		iio_push_to_buffers(indio_dev, adc->buf);
> +	}

[Severity: Medium]
Does this code miss a call to dma_sync_single_for_device()?

The DMA API strictly requires that ownership of non-coherent mapped memory
be transferred back to the device after CPU access. Failing to do so
violates the API contract and can lead to the CPU reading stale data due
to speculative prefetches that overwrite the device's DMA data later.

> +}
> +
> +static void rzt2h_adc_advance_period_index(struct rzt2h_adc *adc, unsigned int i)
> +{
> +	adc->dma.period_index += i;
> +	adc->dma.period_index %= RZT2H_ADC_DMA_PERIODS;
> +}
> +
> +static void rzt2h_adc_dma_thread_loop(struct iio_dev *indio_dev)
> +{
> +	struct rzt2h_adc *adc = iio_priv(indio_dev);
> +	int pending, drop;
> +	dma_addr_t addr;
> +	u16 *period;
> +
> +	pending = atomic_xchg(&adc->dma.pending_periods, 0);
> +
> +	if (pending >= RZT2H_ADC_DMA_PERIODS) {
> +		drop = pending - RZT2H_ADC_DMA_PERIODS + 1;
> +
> +		rzt2h_adc_advance_period_index(adc, drop);
> +		pending -= drop;
> +	}
> +
> +	for (unsigned int i = 0; i < pending; i++) {
> +		unsigned int backlog = atomic_read(&adc->dma.pending_periods) +
> +				       pending - i;
> +
> +		/*
> +		 * Bail if enough new periods have completed since reading the
> +		 * pending_periods that the next period about to be read is at
> +		 * risk of being overwritten.
> +		 */
> +		if (backlog >= RZT2H_ADC_DMA_PERIODS)
> +			break;

[Severity: High]
Does breaking early here cause the read and write pointers to permanently
lose synchronization?

Since all pending periods were extracted using atomic_xchg() at the start
of this loop, breaking out without advancing period_index for the skipped
periods or restoring the pending count permanently discards those periods.
The cyclic DMA buffer read pointer (period_index) will permanently lose
synchronization with the hardware write pointer. The consumer kthread will
then continuously read from the wrong offsets in the DMA cyclic buffer,
pushing stale or corrupted data to userspace without recovering.

> +
> +		period = adc->dma.buf + adc->dma.period_index *
> +			 RZT2H_ADC_DMA_PERIOD_SAMPLES * adc->dma.sample_chans;
> +		addr = adc->dma.addr + adc->dma.period_index *
> +		       adc->dma.period_bytes;
> +
> +		rzt2h_adc_push_period(indio_dev, period, addr);
> +		rzt2h_adc_advance_period_index(adc, 1);
> +	}
> +}
> +
> +static int rzt2h_adc_dma_thread(void *data)
> +{
> +	struct iio_dev *indio_dev = data;
> +	struct rzt2h_adc *adc = iio_priv(indio_dev);
> +
> +	while (!kthread_should_stop()) {
> +		wait_event_interruptible(adc->dma.wq,
> +					 atomic_read(&adc->dma.pending_periods) ||
> +					 kthread_should_stop());

[Severity: Medium]
Can this kthread spin in an infinite loop consuming 100% CPU if it receives
a signal?

By using wait_event_interruptible(), if a signal is delivered to the
kthread (such as SIGKILL), it will return immediately with -ERESTARTSYS.
Since the thread does not check signal_pending() or clear the signal,
and kthread_should_stop() remains false, the loop will instantly restart,
returning immediately again and spinning forever.

> +
> +		if (kthread_should_stop())
> +			break;
> +
> +		rzt2h_adc_dma_thread_loop(indio_dev);
> +	}
> +
> +	return 0;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260828145943.2077589-1-cosmin-gabriel.tanislav.xa@renesas.com?part=5