Re: [PATCH v1 05/13] iio: adc: ad4134: Run shorter transfers when CRC is disabled

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] The TX and RX SPI buffers (`tx_buf` and `rx_buf`) share the same cacheline, violating the DMA API constraints for concurrent bidirectional mapping and causing potential memory corruption.
- [Medium] Incorrect SPI CRC verification logic compares the received response CRC against the transmitted command CRC, causing spurious validation failures and rendering the CRC check useless.
--

commit 6e10b7a255a6c4b304554d7ef28e9e98a2139736
Author: Marcelo Schmitt <marcelo.schmitt@analog.com>

iio: adc: ad4134: Run shorter transfers when CRC is disabled

When AD4134 has its SPI CRC feature enabled, one extra byte (the CRC code)
is sent through SPI for both ends to verify the integrity of data
transfers. Though, when CRC is not enabled, SPI transfers can be shorter.

> diff --git a/drivers/iio/adc/ad4134.c b/drivers/iio/adc/ad4134.c
> index 3460ecf4cafd4..7da6734d4609f 100644
> --- a/drivers/iio/adc/ad4134.c
> +++ b/drivers/iio/adc/ad4134.c
> @@ -102,6 +102,7 @@ struct ad4134_state {
>  	unsigned long sys_clk_hz;
>  	struct gpio_desc *odr_gpio;
>  	int refin_mv;
> +	bool crc_en;

[Severity: High]
This is a pre-existing issue, but does struct ad4134_state force tx_buf
and rx_buf to share a cacheline?

Looking at the struct definition, rx_buf is annotated with
__aligned(IIO_DMA_MINALIGN), which aligns its start address:

	u8 rx_buf[AD4134_SPI_MAX_XFER_LEN] __aligned(IIO_DMA_MINALIGN);
	u8 tx_buf[AD4134_SPI_MAX_XFER_LEN];

Because rx_buf is only 3 bytes long and tx_buf immediately follows it
without its own alignment, tx_buf resides in the exact same cacheline.

When both buffers are passed simultaneously to spi_sync_transfer() during
ad4134_reg_write() or ad4134_register_read(), they are mapped concurrently.

Will performing a simultaneous flush for tx_buf and invalidate for rx_buf
on the same cacheline violate DMA API rules and cause memory corruption
on non-coherent architectures?

>  	/*
>  	 * Synchronize access to members the of driver state, and ensure
>  	 * atomicity of consecutive register access operations.

[ ... ]

> @@ -170,7 +171,7 @@ static int ad4134_reg_write(void *context, unsigned int reg, unsigned int val)
>  	if (ret)
>  		return ret;
>  
> -	if (st->rx_buf[2] != st->tx_buf[2])
> +	if (st->crc_en && st->rx_buf[2] != st->tx_buf[2])

[Severity: Medium]
This isn't a bug introduced by this patch, but is the CRC validation logic
comparing the correct values?

In ad4134_reg_write(), the outgoing command CRC is calculated over the
instruction and stored in st->tx_buf[2]. The incoming response CRC computed
by the device is placed in st->rx_buf[2].

Because the transmitted data differs from the received data, won't comparing
the response CRC directly against the command CRC result in constant
mismatch failures?

>  		dev_dbg(&st->spi->dev, "reg write CRC check failed\n");
>  
>  	return 0;

[ ... ]

> @@ -231,7 +232,7 @@ static int ad4134_register_read(struct ad4134_state *st, unsigned int reg,
>  	*val = st->rx_buf[1];
>  
>  	/* Check CRC */
> -	if (st->rx_buf[2] != st->tx_buf[2])
> +	if (st->crc_en && st->rx_buf[2] != st->tx_buf[2])

[Severity: Medium]
This is a pre-existing issue, but does this comparison render the CRC check
useless for register reads?

Similar to the write path above, st->tx_buf[2] holds the CRC for the
outgoing command (including a dummy data byte), while st->rx_buf[2] holds
the device's CRC over the read response (which includes the actual register
value).

Since the response data is mathematically unrelated to the outgoing command,
will this always produce false-positive CRC check failures rather than
actually validating incoming data integrity?

>  		dev_dbg(&st->spi->dev, "reg read CRC check failed\n");
>  
>  	return 0;

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