[patch] iio: imu: adis16400: Fix sign extension

[patch] iio: imu: adis16400: Fix sign extension

[Dan Carpenter]

Because of C's type promotion rules, the code shifts in
"((val16 & 0xFFF) << 4) >> 4;" cancel each other out and they're a
no-op.

The intention here was to sign-extend then 11th bit so we can use the
sign_extend32() function.

Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
Please review this carefully.  I'm pretty sure it's correct but this is
the first time I've ever used the sign_extend32() function.

diff --git a/drivers/iio/imu/adis16400_core.c b/drivers/iio/imu/adis16400_core.c
index b70873d..ed636c9 100644
--- a/drivers/iio/imu/adis16400_core.c
+++ b/drivers/iio/imu/adis16400_core.c
@@ -414,7 +414,7 @@ static int adis16400_read_raw(struct iio_dev *indio_dev,
 		mutex_unlock(&indio_dev->mlock);
 		if (ret)
 			return ret;
-		val16 = ((val16 & 0xFFF) << 4) >> 4;
+		val16 = sign_extend32(val16, 11);
 		*val = val16;
 		return IIO_VAL_INT;
 	case IIO_CHAN_INFO_OFFSET:

[patch] iio: imu: adis16400: Fix sign extension

[Dan Carpenter]

Because of C's type promotion rules, the code shifts in
"((val16 & 0xFFF) << 4) >> 4;" cancel each other out and they're a
no-op.

The intention here was to sign-extend then 11th bit so we can use the
sign_extend32() function.

Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
Please review this carefully.  I'm pretty sure it's correct but this is
the first time I've ever used the sign_extend32() function.

diff --git a/drivers/iio/imu/adis16400_core.c b/drivers/iio/imu/adis16400_core.c
index b70873d..ed636c9 100644
--- a/drivers/iio/imu/adis16400_core.c
+++ b/drivers/iio/imu/adis16400_core.c
@@ -414,7 +414,7 @@ static int adis16400_read_raw(struct iio_dev *indio_dev,
 		mutex_unlock(&indio_dev->mlock);
 		if (ret)
 			return ret;
-		val16 = ((val16 & 0xFFF) << 4) >> 4;
+		val16 = sign_extend32(val16, 11);
 		*val = val16;
 		return IIO_VAL_INT;
 	case IIO_CHAN_INFO_OFFSET:

Re: [patch] iio: imu: adis16400: Fix sign extension

[Lars-Peter Clausen]

On 01/30/2015 09:30 AM, Dan Carpenter wrote:
> Because of C's type promotion rules, the code shifts in
> "((val16 & 0xFFF) << 4) >> 4;" cancel each other out and they're a
> no-op.
>
> The intention here was to sign-extend then 11th bit so we can use the
> sign_extend32() function.
>
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> ---
> Please review this carefully.  I'm pretty sure it's correct but this is
> the first time I've ever used the sign_extend32() function.

Hi,

Rasmus already fixed this a couple of days ago.

Thanks,
- Lars

Re: [patch] iio: imu: adis16400: Fix sign extension

[Lars-Peter Clausen]

On 01/30/2015 09:30 AM, Dan Carpenter wrote:
> Because of C's type promotion rules, the code shifts in
> "((val16 & 0xFFF) << 4) >> 4;" cancel each other out and they're a
> no-op.
>
> The intention here was to sign-extend then 11th bit so we can use the
> sign_extend32() function.
>
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> ---
> Please review this carefully.  I'm pretty sure it's correct but this is
> the first time I've ever used the sign_extend32() function.

Hi,

Rasmus already fixed this a couple of days ago.

Thanks,
- Lars

Re: [patch] iio: imu: adis16400: Fix sign extension

[Dan Carpenter]

On Fri, Jan 30, 2015 at 09:34:01AM +0100, Lars-Peter Clausen wrote:
> On 01/30/2015 09:30 AM, Dan Carpenter wrote:
> >Because of C's type promotion rules, the code shifts in
> >"((val16 & 0xFFF) << 4) >> 4;" cancel each other out and they're a
> >no-op.
> >
> >The intention here was to sign-extend then 11th bit so we can use the
> >sign_extend32() function.
> >
> >Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> >---
> >Please review this carefully.  I'm pretty sure it's correct but this is
> >the first time I've ever used the sign_extend32() function.
> 
> Hi,
> 
> Rasmus already fixed this a couple of days ago.
> 
> Thanks,
> - Lars

Oh.  Good.  I was CC'd on the other one that he fixed so I went looking
for other bugs and I found this one.

Rasmus, are you using a static checker?  Which one?

regards,
dan carpenter

Re: [patch] iio: imu: adis16400: Fix sign extension

[Dan Carpenter]

On Fri, Jan 30, 2015 at 09:34:01AM +0100, Lars-Peter Clausen wrote:
> On 01/30/2015 09:30 AM, Dan Carpenter wrote:
> >Because of C's type promotion rules, the code shifts in
> >"((val16 & 0xFFF) << 4) >> 4;" cancel each other out and they're a
> >no-op.
> >
> >The intention here was to sign-extend then 11th bit so we can use the
> >sign_extend32() function.
> >
> >Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> >---
> >Please review this carefully.  I'm pretty sure it's correct but this is
> >the first time I've ever used the sign_extend32() function.
> 
> Hi,
> 
> Rasmus already fixed this a couple of days ago.
> 
> Thanks,
> - Lars

Oh.  Good.  I was CC'd on the other one that he fixed so I went looking
for other bugs and I found this one.

Rasmus, are you using a static checker?  Which one?

regards,
dan carpenter

Re: [patch] iio: imu: adis16400: Fix sign extension

[Rasmus Villemoes]

On Fri, Jan 30 2015, Dan Carpenter <dan.carpenter@oracle.com> wrote:

> Rasmus, are you using a static checker?  Which one?

Sort of:

https://github.com/Villemoes/linux-cocci/blob/master/misc/sign_extend.cocci

Rasmus

Re: [patch] iio: imu: adis16400: Fix sign extension

[Rasmus Villemoes]

On Fri, Jan 30 2015, Dan Carpenter <dan.carpenter@oracle.com> wrote:

> Rasmus, are you using a static checker?  Which one?

Sort of:

https://github.com/Villemoes/linux-cocci/blob/master/misc/sign_extend.cocci

Rasmus

Re: [patch] iio: imu: adis16400: Fix sign extension

[Dan Carpenter]

On Fri, Jan 30, 2015 at 10:31:55AM +0100, Rasmus Villemoes wrote:
> On Fri, Jan 30 2015, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> 
> > Rasmus, are you using a static checker?  Which one?
> 
> Sort of:
> 
> https://github.com/Villemoes/linux-cocci/blob/master/misc/sign_extend.cocci
> 

Did you find the bug in fm10k_update_xc_addr_pf()?  That's the other bug
I found.

regards,
dan carpenter

Re: [patch] iio: imu: adis16400: Fix sign extension

[Dan Carpenter]

On Fri, Jan 30, 2015 at 10:31:55AM +0100, Rasmus Villemoes wrote:
> On Fri, Jan 30 2015, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> 
> > Rasmus, are you using a static checker?  Which one?
> 
> Sort of:
> 
> https://github.com/Villemoes/linux-cocci/blob/master/misc/sign_extend.cocci
> 

Did you find the bug in fm10k_update_xc_addr_pf()?  That's the other bug
I found.

regards,
dan carpenter

[PATCH] iio: imu: adis16400: Fix sign extension

[Rasmus Villemoes]

The intention is obviously to sign-extend a 12 bit quantity. But
because of C's promotion rules, the assignment is equivalent to "val16
&= 0xfff;". Use the proper API for this.

Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
---
 drivers/iio/imu/adis16400_core.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/iio/imu/adis16400_core.c b/drivers/iio/imu/adis16400_core.c
index b70873de04ea..fa795dcd5f75 100644
--- a/drivers/iio/imu/adis16400_core.c
+++ b/drivers/iio/imu/adis16400_core.c
@@ -26,6 +26,7 @@
 #include <linux/list.h>
 #include <linux/module.h>
 #include <linux/debugfs.h>
+#include <linux/bitops.h>
 
 #include <linux/iio/iio.h>
 #include <linux/iio/sysfs.h>
@@ -414,7 +415,7 @@ static int adis16400_read_raw(struct iio_dev *indio_dev,
 		mutex_unlock(&indio_dev->mlock);
 		if (ret)
 			return ret;
-		val16 = ((val16 & 0xFFF) << 4) >> 4;
+		val16 = sign_extend32(val16, 11);
 		*val = val16;
 		return IIO_VAL_INT;
 	case IIO_CHAN_INFO_OFFSET:
-- 
2.1.3

Re: [PATCH] iio: imu: adis16400: Fix sign extension

[Lars-Peter Clausen]

On 01/23/2015 12:34 AM, Rasmus Villemoes wrote:
> The intention is obviously to sign-extend a 12 bit quantity. But
> because of C's promotion rules, the assignment is equivalent to "val16
> &= 0xfff;". Use the proper API for this.
>
> Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>

Acked-by: Lars-Peter Clausen <lars@metafoo.de>

Re: [PATCH] iio: imu: adis16400: Fix sign extension

[Jonathan Cameron]

On 23/01/15 08:56, Lars-Peter Clausen wrote:
> On 01/23/2015 12:34 AM, Rasmus Villemoes wrote:
>> The intention is obviously to sign-extend a 12 bit quantity. But
>> because of C's promotion rules, the assignment is equivalent to "val16
>> &= 0xfff;". Use the proper API for this.
>>
>> Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
> 
> Acked-by: Lars-Peter Clausen <lars@metafoo.de>
> 
Applied to the fixes-togreg branch of iio.git and cc'd to stable.