iio: ad7949: fix wrong ADC result due to incorrect bit mask
Subsystems (2)
PATCH v2 1/1 iio: ad7949: fix wrong ADC result due to incorrect bit mask
Fixes a wrong bit mask used for the ADC's result, which was caused by an improper usage of the GENMASK() macro. The bits higher than ADC's resolution are undefined and if not masked out correctly, a wrong result can be given. The GENMASK() macro indexing is zero based, so the mask has to go from [resolution - 1 , 0].
--- drivers/iio/adc/ad7949.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/iio/adc/ad7949.c +1 -1
@@ -91,7 +91,7 @@ static int ad7949_spi_read_channel(struct ad7949_adc_chip *ad7949_adc, int *val,int ret;int i;int bits_per_word = ad7949_adc->resolution; -int mask =GENMASK (ad7949_adc->resolution,0 ); +int mask =GENMASK (ad7949_adc->resolution -1 ,0 );struct spi_message msg;struct spi_transfer tx[] = { {
Signature · 1 line
2.25.1
Jonathan Cameron
maintainerQuoted 5 lines · On Sat, 6 Feb 2021 15:52:58 +0100 Wilfried Wessner…
Fixes a wrong bit mask used for the ADC's result, which was caused by an improper usage of the
GENMASK()macro. The bits higher than ADC's resolution are undefined and if not masked out correctly, a wrong result can be given. TheGENMASK()macro indexing is zero based, so the mask has to go from [resolution - 1 , 0].
Hi Wilfried,
Welcome to IIO and kernel in general!
It's useful to add to the description if the error was found by inspection / script or by observing an actual error on hardware?
Also, needs a fixes tag so we can work out what kernels to back port it to.
+CC Charle-Antoine Couret as the original driver author.
Thanks,
Jonathan
Quoted 20 lines
Signed-off-by Wilfried Wessner <[email protected]>--- drivers/iio/adc/ad7949.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)drivers/iio/adc/ad7949.c +1
-1@@ -91,7 +91,7 @@ static int ad7949_spi_read_channel(struct ad7949_adc_chip *ad7949_adc, int *val,int ret;int i;int bits_per_word = ad7949_adc->resolution; -int mask =GENMASK (ad7949_adc->resolution,0 ); +int mask =GENMASK (ad7949_adc->resolution -1 ,0 );struct spi_message msg;struct spi_transfer tx[] = { {
Quoted 10 lines · Jonathan Cameron
Quoted 5 lines · On Sat, 6 Feb 2021 15:52:58 +0100 Wilfried Wessner…
Fixes a wrong bit mask used for the ADC's result, which was caused by an improper usage of the
GENMASK()macro. The bits higher than ADC's resolution are undefined and if not masked out correctly, a wrong result can be given. TheGENMASK()macro indexing is zero based, so the mask has to go from [resolution - 1 , 0].
Quoted 6 lines
Hi Wilfried,
Welcome to IIO and kernel in general!
It's useful to add to the description if the error was found by inspection / script or by observing an actual error on hardware?
The issue was found in combination of an AD7682 ADC with an ARM based iMX7-CPU. The SPI line was analyzed with a logic analyzer and a discrepancy between applied
voltage level and the ADC reported value in user space was observed. Digging into
the driver code revealed the error.
Quoted 2 lines
Also, needs a fixes tag so we can work out what kernels to back port it to.
Not sure about that, but I found the issue in:
commit 61556703b610a104de324e4f061dc6cf7b218b46 (HEAD -> master, origin/master, origin/HEAD)
Merge: 3afe9076a7c1 7f3414226b58
Author: Linus Torvalds <[email protected]>
Date: Wed Feb 3 11:56:58 2021 -0800
Merge tag 'for-linus-5.11-rc7' of
git://git.kernel.org/pub/scm/linux/kernel/git/rw/uml
Quoted 2 lines
+CC Charle-Antoine Couret as the original driver author.
done.
I wrote also an email to Charles-Antoine with the proposed fix, his comment was:
------
[Wilfried:]
Quoted 2 lines
Quoted 2 lines
since the GENMASK macro uses zero-based indexing?
[Charles-Antoine:]
Quoted 2 lines
You're right, it's a mistake.
It wouldn't be a problem in many cases but it's better to be compliant.
[Wilfried:]
Quoted 2 lines
Quoted 2 lines
Could you pls. comment on that?
[Charles-Antoine:]
Quoted 1 line
Good for me.
------
Quoted 26 lines
Thanks,
Jonathan
Quoted 20 lines
Signed-off-by Wilfried Wessner <[email protected]>--- drivers/iio/adc/ad7949.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)drivers/iio/adc/ad7949.c +1
-1@@ -91,7 +91,7 @@ static int ad7949_spi_read_channel(struct ad7949_adc_chip *ad7949_adc, int *val,int ret;int i;int bits_per_word = ad7949_adc->resolution; -int mask =GENMASK (ad7949_adc->resolution,0 ); +int mask =GENMASK (ad7949_adc->resolution -1 ,0 );struct spi_message msg;struct spi_transfer tx[] = { {
Jonathan Cameron
maintainerQuoted 18 lines · On Sat, 6 Feb 2021 19:48:26 +0100 Wilfried Wessner…
Quoted 10 lines · Jonathan Cameron
Quoted 5 lines · On Sat, 6 Feb 2021 15:52:58 +0100 Wilfried Wessner…
Fixes a wrong bit mask used for the ADC's result, which was caused by an improper usage of the
GENMASK()macro. The bits higher than ADC's resolution are undefined and if not masked out correctly, a wrong result can be given. TheGENMASK()macro indexing is zero based, so the mask has to go from [resolution - 1 , 0].Quoted 6 lines
Hi Wilfried,
Welcome to IIO and kernel in general!
It's useful to add to the description if the error was found by inspection / script or by observing an actual error on hardware?
Quoted 6 lines
The issue was found in combination of an AD7682 ADC with an ARM based iMX7-CPU. The SPI line was analyzed with a logic analyzer and a discrepancy between applied
voltage level and the ADC reported value in user space was observed. Digging into
the driver code revealed the error.
Thanks for the info.
Quoted 6 lines
Quoted 3 lines
Also, needs a fixes tag so we can work out what kernels to back port it to.
Not sure about that, but I found the issue in:
Quoted 9 lines
commit
61556703b610a104de324e4f061dc6cf7b218b46(HEAD -> master, origin/master, origin/HEAD)
Merge:3afe9076a7c17f3414226b58
Author: Linus Torvalds <[email protected]>
Date: Wed Feb 3 11:56:58 2021 -0800Merge tag 'for-linus-5.11-rc7' of
git://git.kernel.org/pub/scm/linux/kernel/git/rw/uml
Ok, that doesn't tell us anything much so here we need to look back and find where the bug was introduced by looking at the code.
git blame drivers/iio/adc/ad7949.c
7f40e0614317f (Charles-Antoine Couret 2018-10-22 23:02:42 +0200 94) int mask = GENMASK(ad7949_adc->resolution, 0);
git log 7f40e0614317f
commit 7f40e0614317f20ac07b5aa5cec2eb43737e28d6
Author: Charles-Antoine Couret <[email protected]> Date: Mon Oct 22 23:02:42 2018 +0200
iio:adc:ad7949: Add AD7949 ADC driver family
Compatible with AD7682 and AD7689 chips.
It is a Analog Devices ADC driver 14/16 bits 4/8 channels with SPI protocol
Datasheet of the device:
http://www.analog.com/media/en/technical-documentation/data-sheets/AD7949.pdf
So it goes all the way back to the beginning. The fixes tag will therefore need to be
Please add that Fixes tag to your v3 with a short statement of how you identified the issue.
Thanks,
Jonathan
Quoted 46 lines
Quoted 2 lines
+CC Charle-Antoine Couret as the original driver author.
done.
I wrote also an email to Charles-Antoine with the proposed fix, his comment was:
------
[Wilfried:]Quoted 2 lines
Quoted 2 lines
since the GENMASK macro uses zero-based indexing?
[Charles-Antoine:]
Quoted 2 lines
You're right, it's a mistake.
It wouldn't be a problem in many cases but it's better to be compliant.[Wilfried:]
Quoted 2 lines
Quoted 2 lines
Could you pls. comment on that?
[Charles-Antoine:]
Quoted 1 line
Good for me.
------
Quoted 26 lines
Thanks,
Jonathan
Quoted 20 lines
Signed-off-by Wilfried Wessner <[email protected]>--- drivers/iio/adc/ad7949.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)drivers/iio/adc/ad7949.c +1
-1@@ -91,7 +91,7 @@ static int ad7949_spi_read_channel(struct ad7949_adc_chip *ad7949_adc, int *val,int ret;int i;int bits_per_word = ad7949_adc->resolution; -int mask =GENMASK (ad7949_adc->resolution,0 ); +int mask =GENMASK (ad7949_adc->resolution -1 ,0 );struct spi_message msg;struct spi_transfer tx[] = { {