Re: [PATCH v4 1/3] iio: adc: Fix incorrect reading when datarate changed in single mode

[sashiko-bot@kernel.org]

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

New issues:
- [High] I2C read failures inside the polling loop are improperly mapped to 'device busy', causing a 7.5-second hang and massive dmesg log flooding.
- [High] The data ready polling logic fails to synchronize because the ST/BSY bit is likely static in continuous conversion mode.
- [Medium] The `ads1100_poll_data_ready` function performs a completely redundant I2C read that wastes bus bandwidth.

Pre-existing issues:
- [High] The return value of `ads1100_set_config_bits` is ignored in `ads1100_set_scale`, leading to silent configuration failures.
- [High] Reading the ADC result immediately after runtime resume returns an invalid or stale conversion.
- [High] I2C read functions allocate transfer buffers on the stack, which is unsafe for DMA operations.
--

commit 06506d1fcb678ffc0137dbc1df3913f95ce47327
Author: Jakub Szczudlo <jakubszczudlo40@gmail.com>
Subject: iio: adc: Fix incorrect reading when datarate changed in single mode

This commit attempts to fix incorrect ADC readings after changing
datarate or gain while suspended in single mode. It introduces a
polling loop to wait for new data before returning from the configuration
functions.

> diff --git a/drivers/iio/adc/ti-ads1100.c b/drivers/iio/adc/ti-ads1100.c
> index 9fe8d54cce83c..e3c801381434d 100644
> --- a/drivers/iio/adc/ti-ads1100.c
> +++ b/drivers/iio/adc/ti-ads1100.c
[ ... ]
> @@ -123,10 +127,49 @@ static int ads1100_get_adc_result(struct ads1100_data *data, int chan, int *val)
>  	return 0;
>  }

[Severity: High]
This is a pre-existing issue, but doesn't reading the ADC result immediately
after runtime resume return an invalid or stale conversion?

Looking at 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));

When the device auto-suspends, a subsequent sysfs read of in_voltage0_raw
calls pm_runtime_resume_and_get(), which transitions the ADC to continuous
mode and starts a new conversion. However, ads1100_get_adc_result()
immediately reads the I2C output register without waiting for the conversion
to complete, which can take up to 125ms at 8 SPS.

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

[Severity: High]
This isn't a bug introduced by this patch, but is it safe to use
stack-allocated buffers for I2C transfers?

Using stack-allocated buffers for I2C transfers violates subsystem rules and
can lead to memory corruption or kernel panics on architectures where
CONFIG_VMAP_STACK is enabled and the underlying I2C controller utilizes DMA.

> +	if (ret < 0) {
> +		dev_err(&data->client->dev, "I2C read fail: %d\n", ret);
> +		return true;

[Severity: High]
Does returning true on hard I2C errors trap the driver in an infinite polling
loop?

Returning true signals to the read_poll_timeout() macro that the device is
merely not ready, causing it to endlessly retry for up to 7.5 seconds instead
of aborting. This masks the actual I2C error code with -ETIMEDOUT and
generates massive dmesg log flooding.

> +	} else if (ret < 3) {
> +		dev_err(&data->client->dev, "Short I2C read\n");
> +		return true;
> +	}
> +
> +	return FIELD_GET(ADS1100_CFG_ST_BSY, buffer[2]);

[Severity: High]
Will this polling logic synchronize properly in continuous conversion mode?

When changing scale or datarate, PM_RUNTIME_ACQUIRE_AUTOSUSPEND() is called,
transitioning the ADC to continuous conversion mode. In continuous conversion
mode on these TI ADCs, the ST_BSY bit does not pulse to indicate new data;
it is typically static.

If ST_BSY reads as 0, ads1100_new_data_not_ready() returns false, breaking the
loop instantly. If it reads as 1, the driver would hang for 7.5 seconds.

> +}
> +
> +static int ads1100_poll_data_ready(struct ads1100_data *data)
> +{
> +	int ret;
> +	u8 buffer[3];
> +	bool data_ready;
> +	int datarate = ads1100_data_rate[FIELD_GET(ADS1100_DR_MASK, data->config)];
> +	/* To be sure we wait 5 times more than datarate */
> +	unsigned long wait_time = DIV_ROUND_CLOSEST(MICRO, 5 * datarate);
> +
> +	/* To be sure that polled value will have value after config change */
> +	ret = i2c_master_recv(data->client, (char *)&buffer, sizeof(buffer));

[Severity: Medium]
Is this initial I2C read redundant?

Executing ads1100_poll_data_ready() triggers this initial i2c_master_recv(),
discards the populated local buffer, and immediately enters
read_poll_timeout() with sleep_before_read = false. This instantly performs
another identical i2c_master_recv() inside ads1100_new_data_not_ready(),
needlessly occupying the I2C bus.

[ ... ]
> @@ -149,19 +198,36 @@ static int ads1100_set_scale(struct ads1100_data *data, int val, int val2)
>  
>  	ads1100_set_config_bits(data, ADS1100_PGA_MASK, ffs(gain) - 1);
>  
> -	return 0;
> +	ret = ads1100_poll_data_ready(data);
> +
> +	return ret;
>  }

[Severity: High]
This is a pre-existing issue, but does ads1100_set_scale() ignore the return
value of ads1100_set_config_bits()?

If the I2C write to update the hardware PGA scale fails, the error is
discarded. The function proceeds to poll for new data and may eventually
return success to userspace.

This leads to silently corrupted or miscalculated voltage measurements because
the sysfs attribute appears to have updated while the ADC is physically
operating with the old scale.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260622221550.374235-1-jakubszczudlo40@gmail.com?part=1