| Message ID | 20191101093505.9408-9-alexandru.ardelean@analog.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | iio: adis: cleanups and fixes | expand |
On Fri, 1 Nov 2019 11:35:03 +0200 Alexandru Ardelean <alexandru.ardelean@analog.com> wrote: > This was found only after the whole thing with the inline functions, but > the compiler actually found something. The value of the `bias` (in > adis16480_get_calibbias()) should only be set if the read operation was > successful. > > Fixes: 2f3abe6cbb6c9 ("iio:imu: Add support for the ADIS16480 and similar IMUs") > Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com> Hmm. It's not really a fix as such unless there is an in kernel consumer that is using this and not checking the return value. I thought about dropping the fixes tag, but it is avoiding confusing the compiler so I suppose we might as well leave it marked as a fix. I will add a note that this doesn't cause any known real problems and so probably isn't stable material. Applied. Thanks, Jonathan > --- > drivers/iio/imu/adis16480.c | 6 ++++-- > 1 file changed, 4 insertions(+), 2 deletions(-) > > diff --git a/drivers/iio/imu/adis16480.c b/drivers/iio/imu/adis16480.c > index b9e2695bcfad..c0e7e768be41 100644 > --- a/drivers/iio/imu/adis16480.c > +++ b/drivers/iio/imu/adis16480.c > @@ -451,12 +451,14 @@ static int adis16480_get_calibbias(struct iio_dev *indio_dev, > case IIO_MAGN: > case IIO_PRESSURE: > ret = adis_read_reg_16(&st->adis, reg, &val16); > - *bias = sign_extend32(val16, 15); > + if (ret == 0) > + *bias = sign_extend32(val16, 15); > break; > case IIO_ANGL_VEL: > case IIO_ACCEL: > ret = adis_read_reg_32(&st->adis, reg, &val32); > - *bias = sign_extend32(val32, 31); > + if (ret == 0) > + *bias = sign_extend32(val32, 31); > break; > default: > ret = -EINVAL;
On Sun, 2019-11-03 at 10:41 +0000, Jonathan Cameron wrote: > On Fri, 1 Nov 2019 11:35:03 +0200 > Alexandru Ardelean <alexandru.ardelean@analog.com> wrote: > > > This was found only after the whole thing with the inline functions, > > but > > the compiler actually found something. The value of the `bias` (in > > adis16480_get_calibbias()) should only be set if the read operation was > > successful. > > > > Fixes: 2f3abe6cbb6c9 ("iio:imu: Add support for the ADIS16480 and > > similar IMUs") > > Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com> > > Hmm. It's not really a fix as such unless there is an in kernel consumer > that is using this and not checking the return value. I thought about > dropping > the fixes tag, but it is avoiding confusing the compiler so I suppose we > might > as well leave it marked as a fix. I will add a note that this doesn't > cause > any known real problems and so probably isn't stable material. Thanks. No idea if it's worth having a Fixes tag or not. I thought about adding it, since it seemed like a-bit-of-a-fix, and wasn't sure, so I added. I guess after upstreaming things a bit, I started wondering about these tags a bit more. Alex > > Applied. > > Thanks, > > Jonathan > > > --- > > drivers/iio/imu/adis16480.c | 6 ++++-- > > 1 file changed, 4 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/iio/imu/adis16480.c b/drivers/iio/imu/adis16480.c > > index b9e2695bcfad..c0e7e768be41 100644 > > --- a/drivers/iio/imu/adis16480.c > > +++ b/drivers/iio/imu/adis16480.c > > @@ -451,12 +451,14 @@ static int adis16480_get_calibbias(struct iio_dev > > *indio_dev, > > case IIO_MAGN: > > case IIO_PRESSURE: > > ret = adis_read_reg_16(&st->adis, reg, &val16); > > - *bias = sign_extend32(val16, 15); > > + if (ret == 0) > > + *bias = sign_extend32(val16, 15); > > break; > > case IIO_ANGL_VEL: > > case IIO_ACCEL: > > ret = adis_read_reg_32(&st->adis, reg, &val32); > > - *bias = sign_extend32(val32, 31); > > + if (ret == 0) > > + *bias = sign_extend32(val32, 31); > > break; > > default: > > ret = -EINVAL;
diff --git a/drivers/iio/imu/adis16480.c b/drivers/iio/imu/adis16480.c index b9e2695bcfad..c0e7e768be41 100644 --- a/drivers/iio/imu/adis16480.c +++ b/drivers/iio/imu/adis16480.c @@ -451,12 +451,14 @@ static int adis16480_get_calibbias(struct iio_dev *indio_dev, case IIO_MAGN: case IIO_PRESSURE: ret = adis_read_reg_16(&st->adis, reg, &val16); - *bias = sign_extend32(val16, 15); + if (ret == 0) + *bias = sign_extend32(val16, 15); break; case IIO_ANGL_VEL: case IIO_ACCEL: ret = adis_read_reg_32(&st->adis, reg, &val32); - *bias = sign_extend32(val32, 31); + if (ret == 0) + *bias = sign_extend32(val32, 31); break; default: ret = -EINVAL;
This was found only after the whole thing with the inline functions, but the compiler actually found something. The value of the `bias` (in adis16480_get_calibbias()) should only be set if the read operation was successful. Fixes: 2f3abe6cbb6c9 ("iio:imu: Add support for the ADIS16480 and similar IMUs") Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com> --- drivers/iio/imu/adis16480.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)