Re: [PATCH v1 03/13] iio: adc: ad4134: Update include list to comply with IWYU principles

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [High] tx_buf and rx_buf in ad4134_state share the same cacheline, leading to potential data corruption during concurrent DMA mappings.
- [High] The 24-bit ADC raw data is read as an unsigned integer but never sign-extended to 32 bits, causing negative voltages to be reported as large positive values.
--

commit 7ad0a307d46e16f77ecfa435dfba2e8a4ef0334d
Author: Marcelo Schmitt <marcelo.schmitt@analog.com>

iio: adc: ad4134: Update include list to comply with IWYU principles

This commit updates the AD4134 driver's include list to comply with 
include-what-you-use principles. It adds necessary headers like bits.h and
time64.h, and removes redundant ones.

> diff --git a/drivers/iio/adc/ad4134.c b/drivers/iio/adc/ad4134.c
> index 70f80bf1825f8..7a3b05aacc744 100644
> --- a/drivers/iio/adc/ad4134.c
> +++ b/drivers/iio/adc/ad4134.c

[ ... ]

drivers/iio/adc/ad4134.c:ad4134_state()
> 	/*
> 	 * DMA (thus cache coherency maintenance) requires the transfer buffers
> 	 * to live in their own cache lines.
> 	 */
> 	u8 rx_buf[AD4134_SPI_MAX_XFER_LEN] __aligned(IIO_DMA_MINALIGN);
> 	u8 tx_buf[AD4134_SPI_MAX_XFER_LEN];
> };

[Severity: High]
This isn't a bug introduced by this patch, but does tx_buf lack the necessary
IIO_DMA_MINALIGN attribute here in ad4134_state?

Since rx_buf is aligned to a cacheline boundary but only takes 3 bytes
(AD4134_SPI_MAX_XFER_LEN), tx_buf appears to be placed at offset 3 within the
exact same cacheline.

During SPI transfers (such as in ad4134_register_read), the SPI core
executes dma_map_single() for both tx_buf and rx_buf independently on the
same transfer. Can performing concurrent DMA cache maintenance on the same
cacheline (flushing tx_buf and invalidating rx_buf) cause data corruption
on non-coherent systems?

While later patches in this series move the alignment attribute to the
preceding scan buffer, it seems rx_buf and tx_buf still continue to share a
cacheline with each other in the final state.


drivers/iio/adc/ad4134.c:ad4134_data_read()
> 		/*
> 		 * AD4134 has a built-in feature that flags when data transfers
> 		 * don't run enough clock cycles to read the entire data frame.
> 		 * Clock out data from all channels to avoid that.
> 		 */
> 		if (i == AD4134_VREG_CH(reg))
> 			*val = get_unaligned_be24(st->rx_buf);
> 	}
> 
> 	return 0;
> }

[Severity: High]
This is a pre-existing issue, but does this code fail to properly sign-extend
the 24-bit raw ADC data in ad4134_data_read()?

The ADC channels are defined with IIO_SCAN_FORMAT_SIGNED_INT, meaning the
hardware outputs 24-bit signed two's complement data. However, reading the
3 bytes with get_unaligned_be24() returns an unsigned 24-bit value.

Because this value is not sign-extended (e.g. using sign_extend32), could
negative ADC readings (like 0xFFFFFF) be incorrectly reported to userspace as
large positive integers (16777215) instead of negative values?

It appears this issue also remains unfixed in the final state of this series.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1788368334.git.marcelo.schmitt@analog.com?part=3