| Message ID | 20210206145258.GA603024@ubuntu (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] iio: ad7949: fix wrong ADC result due to incorrect bit mask | expand |
On Sat, 6 Feb 2021 15:52:58 +0100 Wilfried Wessner <wilfried.wessner@gmail.com> wrote: > 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]. 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 > > Signed-off-by: Wilfried Wessner <wilfried.wessner@gmail.com> > > --- > drivers/iio/adc/ad7949.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/iio/adc/ad7949.c b/drivers/iio/adc/ad7949.c > index 5d597e5050f6..1b4b3203e428 100644 > --- a/drivers/iio/adc/ad7949.c > +++ b/drivers/iio/adc/ad7949.c > @@ -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[] = { > {
On Sat, Feb 6, 2021 at 6:01 PM Jonathan Cameron <jic23@kernel.org> wrote: > > On Sat, 6 Feb 2021 15:52:58 +0100 > Wilfried Wessner <wilfried.wessner@gmail.com> wrote: > > > 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]. > > 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. > > 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 <torvalds@linux-foundation.org> 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 > > +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:] >> since the GENMASK macro uses zero-based indexing? >> [Charles-Antoine:] >You're right, it's a mistake. >It wouldn't be a problem in many cases but it's better to be compliant. [Wilfried:] >> Could you pls. comment on that? >> [Charles-Antoine:] >Good for me. ------ > > Thanks, > > Jonathan > > > > > Signed-off-by: Wilfried Wessner <wilfried.wessner@gmail.com> > > > > --- > > drivers/iio/adc/ad7949.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/iio/adc/ad7949.c b/drivers/iio/adc/ad7949.c > > index 5d597e5050f6..1b4b3203e428 100644 > > --- a/drivers/iio/adc/ad7949.c > > +++ b/drivers/iio/adc/ad7949.c > > @@ -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[] = { > > { >
On Sat, 6 Feb 2021 19:48:26 +0100 Wilfried Wessner <wilfried.wessner@gmail.com> wrote: > On Sat, Feb 6, 2021 at 6:01 PM Jonathan Cameron <jic23@kernel.org> wrote: > > > > On Sat, 6 Feb 2021 15:52:58 +0100 > > Wilfried Wessner <wilfried.wessner@gmail.com> wrote: > > > > > 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]. > > > > 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. Thanks for the info. > > > > > 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 <torvalds@linux-foundation.org> > 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 > 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 <charles-antoine.couret@essensium.com> 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 Signed-off-by: Charles-Antoine Couret <charles-antoine.couret@essensium.com> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> So it goes all the way back to the beginning. The fixes tag will therefore need to be Fixes: 7f40e0614317f ("iio:adc:ad7949: Add AD7949 ADC driver family") Please add that Fixes tag to your v3 with a short statement of how you identified the issue. Thanks, Jonathan > > > > +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:] > >> since the GENMASK macro uses zero-based indexing? > >> > [Charles-Antoine:] > >You're right, it's a mistake. > >It wouldn't be a problem in many cases but it's better to be compliant. > [Wilfried:] > >> Could you pls. comment on that? > >> > [Charles-Antoine:] > >Good for me. > ------ > > > > > > Thanks, > > > > Jonathan > > > > > > > > Signed-off-by: Wilfried Wessner <wilfried.wessner@gmail.com> > > > > > > --- > > > drivers/iio/adc/ad7949.c | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/drivers/iio/adc/ad7949.c b/drivers/iio/adc/ad7949.c > > > index 5d597e5050f6..1b4b3203e428 100644 > > > --- a/drivers/iio/adc/ad7949.c > > > +++ b/drivers/iio/adc/ad7949.c > > > @@ -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[] = { > > > { > >
diff --git a/drivers/iio/adc/ad7949.c b/drivers/iio/adc/ad7949.c index 5d597e5050f6..1b4b3203e428 100644 --- a/drivers/iio/adc/ad7949.c +++ b/drivers/iio/adc/ad7949.c @@ -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[] = { {
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]. Signed-off-by: Wilfried Wessner <wilfried.wessner@gmail.com> --- drivers/iio/adc/ad7949.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)