[RFC] iio: adis: Fix build warnings due to disagreement on whether ret can be > 0

[RFC] iio: adis: Fix build warnings due to disagreement on whether ret can be > 0

[jic23]

From: Jonathan Cameron <Jonathan.Cameron@huawei.com>

I added a check to the inline functions in the header that provide
the specific sized versions of __adis_read_reg_* to check if
they were returning 0 before setting the value.   This was
needed to solve a 0-day report which I was unable to replicate.

Unfortunately it caused some build warnings because the drivers
check for ret is not negative before using the value, leaving the
positive range where flow continues, but the value is unset.

This is one of several possible fixes, hence the RFC.

Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
 drivers/iio/gyro/adis16136.c | 4 ++--
 drivers/iio/imu/adis.c       | 2 +-
 drivers/iio/imu/adis16480.c  | 4 ++--
 3 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/iio/gyro/adis16136.c b/drivers/iio/gyro/adis16136.c
index 2d2c48f0b996..223c42ef6f86 100644
--- a/drivers/iio/gyro/adis16136.c
+++ b/drivers/iio/gyro/adis16136.c
@@ -191,7 +191,7 @@ static int __adis16136_get_freq(struct adis16136 *adis16136, unsigned int *freq)
 	int ret;
 
 	ret = __adis_read_reg_16(&adis16136->adis, ADIS16136_REG_SMPL_PRD, &t);
-	if (ret < 0)
+	if (ret)
 		return ret;
 
 	*freq = 32768 / (t + 1);
@@ -288,7 +288,7 @@ static int adis16136_get_filter(struct iio_dev *indio_dev, int *val)
 
 	ret = __adis_read_reg_16(&adis16136->adis, ADIS16136_REG_AVG_CNT,
 				 &val16);
-	if (ret < 0)
+	if (ret)
 		goto err_unlock;
 
 	ret = __adis16136_get_freq(adis16136, &freq);
diff --git a/drivers/iio/imu/adis.c b/drivers/iio/imu/adis.c
index 5e28464ea05b..6ee54996609a 100644
--- a/drivers/iio/imu/adis.c
+++ b/drivers/iio/imu/adis.c
@@ -289,7 +289,7 @@ int __adis_check_status(struct adis *adis)
 	int i;
 
 	ret = __adis_read_reg_16(adis, adis->data->diag_stat_reg, &status);
-	if (ret < 0)
+	if (ret)
 		return ret;
 
 	status &= adis->data->status_error_mask;
diff --git a/drivers/iio/imu/adis16480.c b/drivers/iio/imu/adis16480.c
index 01dae50e985b..ed3e52651003 100644
--- a/drivers/iio/imu/adis16480.c
+++ b/drivers/iio/imu/adis16480.c
@@ -564,7 +564,7 @@ static int adis16480_set_filter_freq(struct iio_dev *indio_dev,
 	mutex_lock(slock);
 
 	ret = __adis_read_reg_16(&st->adis, reg, &val);
-	if (ret < 0)
+	if (ret)
 		goto out_unlock;
 
 	if (freq == 0) {
@@ -944,7 +944,7 @@ static int adis16480_enable_irq(struct adis *adis, bool enable)
 	int ret;
 
 	ret = __adis_read_reg_16(adis, ADIS16480_REG_FNCTIO_CTRL, &val);
-	if (ret < 0)
+	if (ret)
 		return ret;
 
 	val &= ~ADIS16480_DRDY_EN_MSK;
-- 
2.23.0

Re: [RFC] iio: adis: Fix build warnings due to disagreement on whether ret can be > 0

[Alexandru Ardelean]

On Sat, Oct 12, 2019 at 7:32 PM <jic23@kernel.org> wrote:
>
> From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
>
> I added a check to the inline functions in the header that provide
> the specific sized versions of __adis_read_reg_* to check if
> they were returning 0 before setting the value.   This was
> needed to solve a 0-day report which I was unable to replicate.
>
> Unfortunately it caused some build warnings because the drivers
> check for ret is not negative before using the value, leaving the
> positive range where flow continues, but the value is unset.
>
> This is one of several possible fixes, hence the RFC.
>

I'll take a closer look at this patch on Monday.
But I'll also want to look at the whole series again for ADIS.

It looks to me that the rework (though it looked credible at review),
did expose some subtleties that the compiler complains about.
I'll probably use this issue as a test-bed for the CI improvements
I'll do on our end.

Thanks
Alex

> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> ---
>  drivers/iio/gyro/adis16136.c | 4 ++--
>  drivers/iio/imu/adis.c       | 2 +-
>  drivers/iio/imu/adis16480.c  | 4 ++--
>  3 files changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/iio/gyro/adis16136.c b/drivers/iio/gyro/adis16136.c
> index 2d2c48f0b996..223c42ef6f86 100644
> --- a/drivers/iio/gyro/adis16136.c
> +++ b/drivers/iio/gyro/adis16136.c
> @@ -191,7 +191,7 @@ static int __adis16136_get_freq(struct adis16136 *adis16136, unsigned int *freq)
>         int ret;
>
>         ret = __adis_read_reg_16(&adis16136->adis, ADIS16136_REG_SMPL_PRD, &t);
> -       if (ret < 0)
> +       if (ret)
>                 return ret;
>
>         *freq = 32768 / (t + 1);
> @@ -288,7 +288,7 @@ static int adis16136_get_filter(struct iio_dev *indio_dev, int *val)
>
>         ret = __adis_read_reg_16(&adis16136->adis, ADIS16136_REG_AVG_CNT,
>                                  &val16);
> -       if (ret < 0)
> +       if (ret)
>                 goto err_unlock;
>
>         ret = __adis16136_get_freq(adis16136, &freq);
> diff --git a/drivers/iio/imu/adis.c b/drivers/iio/imu/adis.c
> index 5e28464ea05b..6ee54996609a 100644
> --- a/drivers/iio/imu/adis.c
> +++ b/drivers/iio/imu/adis.c
> @@ -289,7 +289,7 @@ int __adis_check_status(struct adis *adis)
>         int i;
>
>         ret = __adis_read_reg_16(adis, adis->data->diag_stat_reg, &status);
> -       if (ret < 0)
> +       if (ret)
>                 return ret;
>
>         status &= adis->data->status_error_mask;
> diff --git a/drivers/iio/imu/adis16480.c b/drivers/iio/imu/adis16480.c
> index 01dae50e985b..ed3e52651003 100644
> --- a/drivers/iio/imu/adis16480.c
> +++ b/drivers/iio/imu/adis16480.c
> @@ -564,7 +564,7 @@ static int adis16480_set_filter_freq(struct iio_dev *indio_dev,
>         mutex_lock(slock);
>
>         ret = __adis_read_reg_16(&st->adis, reg, &val);
> -       if (ret < 0)
> +       if (ret)
>                 goto out_unlock;
>
>         if (freq == 0) {
> @@ -944,7 +944,7 @@ static int adis16480_enable_irq(struct adis *adis, bool enable)
>         int ret;
>
>         ret = __adis_read_reg_16(adis, ADIS16480_REG_FNCTIO_CTRL, &val);
> -       if (ret < 0)
> +       if (ret)
>                 return ret;
>
>         val &= ~ADIS16480_DRDY_EN_MSK;
> --
> 2.23.0
>