[PATCH v2] iio: ti-ads7138: Disable STATS_EN bit while reading conversion results

[PATCH v2] iio: ti-ads7138: Disable STATS_EN bit while reading conversion results

[Paul Geurts]

The device might update channel data while it's read by the host,
providing a data race. Disable the update of the channel stats before
reading the values.

Signed-off-by: Paul Geurts <paul.geurts@prodrive-technologies.com>
Fixes: 93a39542d3c3 ("iio: adc: Add driver for ADS7128 / ADS7138")
---

V1 -> V2: Checked return values and prefixed iio: in commit msg

v1: https://lore.kernel.org/all/20260619075646.4100193-1-paul.geurts@prodrive-technologies.com/
---
 drivers/iio/adc/ti-ads7138.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/iio/adc/ti-ads7138.c b/drivers/iio/adc/ti-ads7138.c
index ee5c1b8e3a8e..81380fd2badc 100644
--- a/drivers/iio/adc/ti-ads7138.c
+++ b/drivers/iio/adc/ti-ads7138.c
@@ -237,11 +237,21 @@ static int ads7138_read_raw(struct iio_dev *indio_dev,
 
 	switch (mask) {
 	case IIO_CHAN_INFO_RAW:
+		/* Disable statistics update so the value is not updated mid read */
+		ret = ads7138_i2c_clear_bit(data->client, ADS7138_REG_GENERAL_CFG,
+					    ADS7138_GENERAL_CFG_STATS_EN);
+		if (ret)
+			return ret;
 		ret = ads7138_i2c_read_block(data->client,
 					     ADS7138_REG_RECENT_LSB_CH(chan->channel),
 					     values, ARRAY_SIZE(values));
 		if (ret)
 			return ret;
+		/* Enable statistics update after read */
+		ret = ads7138_i2c_set_bit(data->client, ADS7138_REG_GENERAL_CFG,
+					  ADS7138_GENERAL_CFG_STATS_EN);
+		if (ret)
+			return ret;
 
 		*val = get_unaligned_le16(values);
 		return IIO_VAL_INT;
-- 
2.39.2

Re: [PATCH v2] iio: ti-ads7138: Disable STATS_EN bit while reading conversion results

[David Lechner]

On 6/19/26 4:00 AM, Paul Geurts wrote:
> The device might update channel data while it's read by the host,
> providing a data race. Disable the update of the channel stats before
> reading the values.

This description seems a bit short on details. It looks like this
driver doesn't support buffered reads. So if we disable statistics
during a direct read, why would we want to enable statistics in
the first place?

(This driver suffers from the comments say "what" rather than "why"
/* Enable statistics and digital window comparator */ so it is hard
to say what the original intention was.)

Can you explain more what this race condition is about and what
happens before the fix vs. after the fix? People generally do this
with two columns showing concurrent function calls.

It seems to me like disabling statistics would break IIO_CHAN_INFO_PEAK. 

> 
> Signed-off-by: Paul Geurts <paul.geurts@prodrive-technologies.com>
> Fixes: 93a39542d3c3 ("iio: adc: Add driver for ADS7128 / ADS7138")
> ---
> 
> V1 -> V2: Checked return values and prefixed iio: in commit msg
> 
> v1: https://lore.kernel.org/all/20260619075646.4100193-1-paul.geurts@prodrive-technologies.com/
> ---
>  drivers/iio/adc/ti-ads7138.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/drivers/iio/adc/ti-ads7138.c b/drivers/iio/adc/ti-ads7138.c
> index ee5c1b8e3a8e..81380fd2badc 100644
> --- a/drivers/iio/adc/ti-ads7138.c
> +++ b/drivers/iio/adc/ti-ads7138.c
> @@ -237,11 +237,21 @@ static int ads7138_read_raw(struct iio_dev *indio_dev,
>  
>  	switch (mask) {
>  	case IIO_CHAN_INFO_RAW:
> +		/* Disable statistics update so the value is not updated mid read */
> +		ret = ads7138_i2c_clear_bit(data->client, ADS7138_REG_GENERAL_CFG,
> +					    ADS7138_GENERAL_CFG_STATS_EN);
> +		if (ret)
> +			return ret;
>  		ret = ads7138_i2c_read_block(data->client,
>  					     ADS7138_REG_RECENT_LSB_CH(chan->channel),
>  					     values, ARRAY_SIZE(values));
>  		if (ret)
>  			return ret;
> +		/* Enable statistics update after read */
> +		ret = ads7138_i2c_set_bit(data->client, ADS7138_REG_GENERAL_CFG,
> +					  ADS7138_GENERAL_CFG_STATS_EN);
> +		if (ret)
> +			return ret;
>  
>  		*val = get_unaligned_le16(values);
>  		return IIO_VAL_INT;

Re: [PATCH v2] iio: ti-ads7138: Disable STATS_EN bit while reading conversion results

[Andy Shevchenko]

On Fri, Jun 19, 2026 at 09:42:08AM -0500, David Lechner wrote:
> On 6/19/26 4:00 AM, Paul Geurts wrote:
> > The device might update channel data while it's read by the host,
> > providing a data race. Disable the update of the channel stats before
> > reading the values.
> 
> This description seems a bit short on details. It looks like this
> driver doesn't support buffered reads. So if we disable statistics
> during a direct read, why would we want to enable statistics in
> the first place?

I believe this patch is initiated by
"Until a new conversion result is available, previous values can be read from
the statistics registers. Before reading the statistics registers, set STATS_EN
to 0 to prevent any updates to this register block."
from the datasheet.

I would rather like to know if this is IRL problem, or just a datasheet reading
based code.

> (This driver suffers from the comments say "what" rather than "why"
> /* Enable statistics and digital window comparator */ so it is hard
> to say what the original intention was.)
> 
> Can you explain more what this race condition is about and what
> happens before the fix vs. after the fix? People generally do this
> with two columns showing concurrent function calls.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2] iio: ti-ads7138: Disable STATS_EN bit while reading conversion results

[Paul Geurts]

> On Fri, Jun 19, 2026 at 09:42:08AM -0500, David Lechner wrote:
> > On 6/19/26 4:00 AM, Paul Geurts wrote:
> > > The device might update channel data while it's read by the host,
> > > providing a data race. Disable the update of the channel stats before
> > > reading the values.
> >
> > This description seems a bit short on details. It looks like this
> > driver doesn't support buffered reads. So if we disable statistics
> > during a direct read, why would we want to enable statistics in
> > the first place?
>
> I believe this patch is initiated by
> "Until a new conversion result is available, previous values can be read from
> the statistics registers. Before reading the statistics registers, set STATS_EN
> to 0 to prevent any updates to this register block."
> from the datasheet.
>
> I would rather like to know if this is IRL problem, or just a datasheet reading
> based code.

This is indeed the part of the datahseet I got the solution from. Our real life
scenario was that the register read from the RECENT register was constantly
flipping between values 0x24F0 and 0x2500. Due to the STATS_EN not being
cleared, we occasionally read 0x2400 and 0x25F0. So this is based on a real
life problem.

>
> > (This driver suffers from the comments say "what" rather than "why"
> > /* Enable statistics and digital window comparator */ so it is hard
> > to say what the original intention was.)
> >
> > Can you explain more what this race condition is about and what
> > happens before the fix vs. after the fix? People generally do this
> > with two columns showing concurrent function calls.
> >
> > It seems to me like disabling statistics would break IIO_CHAN_INFO_PEAK.

I think it indeed could break the PEAK value. But I think the data race is a
bigger issue. I'm not sure using the statistics at all is a good idea for
this device, but I'm also not sure there is another option. TBH, I don't
really have the time now to properly fix all of this. I just saw and
quickly fixed this, and thought it would be good to let you know this is
an issue.

>
> --
> With Best Regards,
> Andy Shevchenko

br,
Paul

Re: [PATCH v2] iio: ti-ads7138: Disable STATS_EN bit while reading conversion results

[David Lechner]

On 6/23/26 7:52 AM, Paul Geurts wrote:
>> On Fri, Jun 19, 2026 at 09:42:08AM -0500, David Lechner wrote:
>>> On 6/19/26 4:00 AM, Paul Geurts wrote:
>>>> The device might update channel data while it's read by the host,
>>>> providing a data race. Disable the update of the channel stats before
>>>> reading the values.
>>>
>>> This description seems a bit short on details. It looks like this
>>> driver doesn't support buffered reads. So if we disable statistics
>>> during a direct read, why would we want to enable statistics in
>>> the first place?
>>
>> I believe this patch is initiated by
>> "Until a new conversion result is available, previous values can be read from
>> the statistics registers. Before reading the statistics registers, set STATS_EN
>> to 0 to prevent any updates to this register block."
>> from the datasheet.
>>
>> I would rather like to know if this is IRL problem, or just a datasheet reading
>> based code.
> 
> This is indeed the part of the datahseet I got the solution from. Our real life
> scenario was that the register read from the RECENT register was constantly
> flipping between values 0x24F0 and 0x2500. Due to the STATS_EN not being
> cleared, we occasionally read 0x2400 and 0x25F0. So this is based on a real
> life problem.

Thanks, this is the kind of description we want in the commit message.

> 
>>
>>> (This driver suffers from the comments say "what" rather than "why"
>>> /* Enable statistics and digital window comparator */ so it is hard
>>> to say what the original intention was.)
>>>
>>> Can you explain more what this race condition is about and what
>>> happens before the fix vs. after the fix? People generally do this
>>> with two columns showing concurrent function calls.
>>>
>>> It seems to me like disabling statistics would break IIO_CHAN_INFO_PEAK.
> 
> I think it indeed could break the PEAK value. But I think the data race is a
> bigger issue. I'm not sure using the statistics at all is a good idea for
> this device, but I'm also not sure there is another option. TBH, I don't
> really have the time now to properly fix all of this. I just saw and
> quickly fixed this, and thought it would be good to let you know this is
> an issue.

Now that it has been explained in more detail, I can see that your fix
is correct. And we should also do the same when reading IIO_CHAN_INFO_PEAK
and IIO_CHAN_INFO_TROUGH.

This would make it so that we could never get the peak and trough from
the same time period though since each read would reset all 3 values. Not
sure if that is important or not.

> 
>>
>> --
>> With Best Regards,
>> Andy Shevchenko
> 
> br,
> Paul

Re: [PATCH v2] iio: ti-ads7138: Disable STATS_EN bit while reading conversion results

[Paul Geurts]

> On 6/23/26 7:52 AM, Paul Geurts wrote:
>>> On Fri, Jun 19, 2026 at 09:42:08AM -0500, David Lechner wrote:
>>>> On 6/19/26 4:00 AM, Paul Geurts wrote:
>>>>> The device might update channel data while it's read by the host,
>>>>> providing a data race. Disable the update of the channel stats before
>>>>> reading the values.
>>>>
>>>> This description seems a bit short on details. It looks like this
>>>> driver doesn't support buffered reads. So if we disable statistics
>>>> during a direct read, why would we want to enable statistics in
>>>> the first place?
>>>
>>> I believe this patch is initiated by
>>> "Until a new conversion result is available, previous values can be read from
>>> the statistics registers. Before reading the statistics registers, set STATS_EN
>>> to 0 to prevent any updates to this register block."
>>> from the datasheet.
>>>
>>> I would rather like to know if this is IRL problem, or just a datasheet reading
>>> based code.
>> 
>> This is indeed the part of the datahseet I got the solution from. Our real life
>> scenario was that the register read from the RECENT register was constantly
>> flipping between values 0x24F0 and 0x2500. Due to the STATS_EN not being
>> cleared, we occasionally read 0x2400 and 0x25F0. So this is based on a real
>> life problem.
>
> Thanks, this is the kind of description we want in the commit message.

Ok I'll update that.

>
>> 
>>>
>>>> (This driver suffers from the comments say "what" rather than "why"
>>>> /* Enable statistics and digital window comparator */ so it is hard
>>>> to say what the original intention was.)
>>>>
>>>> Can you explain more what this race condition is about and what
>>>> happens before the fix vs. after the fix? People generally do this
>>>> with two columns showing concurrent function calls.
>>>>
>>>> It seems to me like disabling statistics would break IIO_CHAN_INFO_PEAK.
>> 
>> I think it indeed could break the PEAK value. But I think the data race is a
>> bigger issue. I'm not sure using the statistics at all is a good idea for
>> this device, but I'm also not sure there is another option. TBH, I don't
>> really have the time now to properly fix all of this. I just saw and
>> quickly fixed this, and thought it would be good to let you know this is
>> an issue.
>
> Now that it has been explained in more detail, I can see that your fix
> is correct. And we should also do the same when reading IIO_CHAN_INFO_PEAK
> and IIO_CHAN_INFO_TROUGH.
>
> This would make it so that we could never get the peak and trough from
> the same time period though since each read would reset all 3 values. Not
> sure if that is important or not.

Yes I can add that. It would probably not be ideal, but at least the data
being read is correct.

>
>> 
>>>
>>> --
>>> With Best Regards,
>>> Andy Shevchenko
>> 
>> br,
>> Paul

Thanks!
Paul