Re: [PATCH v9 1/3] iio: adc: ti-ads1100: Fix incorrect reading when datarate changed in single mode

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

New issues:
- [High] Error paths in `ads1100_start_single_conversion()` leave the device and driver in a permanently desynchronized state on timeout or I2C failure.
- [High] I2C buffers allocated on the stack are passed to DMA-capable I2C APIs.

Pre-existing issues:
- [High] ADC read after runtime resume returns garbage or power-on reset values without waiting for the first conversion to complete.
--

--- Patch [1]: [PATCH v9 1/3] iio: adc: ti-ads1100: Fix incorrect reading when datarate changed in single mode ---
commit e93e149965f30840eff030d72d0901bf664fc6dd
Author: Jakub Szczudlo <[email protected]>

iio: adc: ti-ads1100: Fix incorrect reading when datarate changed in single mode

When device is suspended and it is in single mode then changing
datarate doesn't make it actually wait for new measurement, so to
be sure that read after change is correct, functions that changes
datarate and gain will wait for a new data.

> diff --git a/drivers/iio/adc/ti-ads1100.c b/drivers/iio/adc/ti-ads1100.c
> index 9fe8d54cce83c..288d209ecf92d 100644
> --- a/drivers/iio/adc/ti-ads1100.c
> +++ b/drivers/iio/adc/ti-ads1100.c
> @@ -123,10 +128,87 @@ static int ads1100_get_adc_result(struct ads1100_data *data, int chan, int *val)

[Severity: High]
This isn't a bug introduced by this patch, but does ads1100_get_adc_result()
read garbage or power-on reset values if executed immediately after waking up?

In ads1100_get_adc_result():

	ret = pm_runtime_resume_and_get(&data->client->dev);
	if (ret < 0)
		return ret;

	ret = i2c_master_recv(data->client, (char *)&buffer, sizeof(buffer));

Since a single conversion takes up to 125ms (at 8 SPS), calling
i2c_master_recv() immediately after runtime resume reads the ADC before it
completes its initial measurement.

>  	return 0;
>  }
>  
> +static int ads1100_conversion_busy(struct ads1100_data *data)
> +{
> +	u8 buffer[3];
> +	int ret;
> +
> +	ret = i2c_master_recv(data->client, (char *)&buffer, sizeof(buffer));

[Severity: High]
Can this cause memory corruption?  Passing the stack-allocated buffer to
i2c_master_recv() is unsafe if the underlying I2C host controller uses DMA,
as it can corrupt adjacent stack variables due to cache line invalidation.

> +	if (ret < 0) {
[ ... ]
> +static int ads1100_start_single_conversion(struct ads1100_data *data)
> +{
> +	u8 config = data->config | ADS1100_CFG_SC;
> +	int ret;
> +
> +	ret = i2c_master_send(data->client, &config, sizeof(config));

[Severity: High]
Could passing this stack-allocated config variable to i2c_master_send() also
cause memory corruption if the host controller uses DMA?

> +	if (ret < 0) {
> +		dev_err(&data->client->dev, "I2C write fail: %d\n", ret);
> +		return ret;
> +	}
> +   /* Need to wait because of change from continuous to single mode */
> +	ret = ads1100_wait_single_conversion(data);
> +	if (ret)
> +		return ret;
> +
> +	config |= ADS1100_CFG_ST_BSY;
> +
> +	ret = i2c_master_send(data->client, &config, sizeof(config));
> +	if (ret < 0) {
> +		dev_err(&data->client->dev, "I2C write fail: %d\n", ret);
> +		return ret;
> +	}
> +
> +	/* No need to cache it, it's status bit */
> +	data->config = config & ~ADS1100_CFG_ST_BSY;

[Severity: High]
Are we leaving the driver and hardware permanently out of sync on error here?

If ads1100_wait_single_conversion() or the second i2c_master_send() fails,
we return early. However, the physical hardware was transitioned to
single-shot mode while the software cache data->config was not updated to
include ADS1100_CFG_SC.

If userspace later tries to restore continuous mode, won't it silently skip
the I2C write because data->config appears unchanged, leaving the device
stuck in single-shot mode indefinitely?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1