Re: [PATCH 08/10] iio: imu: adis16480: assign bias value only if operation succeeded

[Ardelean, Alexandru]

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@xxxxxxxxxx> 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@xxxxxxxxxx>
> 
> 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;