Received-SPF: softfail (google.com: domain of transitioning linux-kernel-owner@vger.kernel.org does not designate 23.128.96.19 as permitted sender) client-ip=23.128.96.19;
Date:   Sat, 26 Feb 2022 18:01:24 +0000
From:   Jonathan Cameron <jic23@kernel.org>
To:     "Colin King (gmail)" <colin.i.king@gmail.com>
Cc:     Marcelo Schmitt <marcelo.schmitt1@gmail.com>,
        linux-iio@vger.kernel.org,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: Re: iio:adc:ad7280a: Move out of staging
Message-ID: <20220226180124.175f6162@jic23-huawei>
In-Reply-To: <69aeef03-69ba-07f4-2506-31e481f86c76@gmail.com>
References: <69aeef03-69ba-07f4-2506-31e481f86c76@gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Precedence: bulk

On Thu, 24 Feb 2022 22:56:20 +0000
"Colin King (gmail)" <colin.i.king@gmail.com> wrote:

> Hi,
> 
> Static analysis with clang scan picked up a potential issue with 
> drivers/iio/adc/ad7280a.c in function ad7280a_write_thresh, the analysis 
> is as follows:
> 
>          switch (chan->type) {
>          case IIO_VOLTAGE:
>                  value = ((val - 1000) * 100) / 1568; /* LSB 15.68mV */
>                  value = clamp(value, 0L, 0xFFL);
> 
>                  ^^
> Note: variable value is being assigned a value
> 
>                  switch (dir) {
>                  case IIO_EV_DIR_RISING:
>                          addr = AD7280A_CELL_OVERVOLTAGE_REG;
>                          ret = ad7280_write(st, AD7280A_DEVADDR_MASTER, 
> addr,
>                                             1, val);
>                          if (ret)
>                                  break;
>                          st->cell_threshhigh = value;
> 
> ..and value is being used here ^^
> 
>                          break;
>                  case IIO_EV_DIR_FALLING:
>                          addr = AD7280A_CELL_UNDERVOLTAGE_REG;
>                          ret = ad7280_write(st, AD7280A_DEVADDR_MASTER, 
> addr,
>                                             1, val);
>                          if (ret)
>                                  break;
>                          st->cell_threshlow = value;
> 
> and value is being used here ^^
> 
>                          break;
>                  default:
>                          ret = -EINVAL;
>                          goto err_unlock;
>                  }
>                  break;
> 
> However for the IIO_TEMP case:
> 
>          case IIO_TEMP:
>                  value = (val * 10) / 196; /* LSB 19.6mV */
>                  value = clamp(value, 0L, 0xFFL);
> 
>                  ^^
> Note: variable value is being assigned a value
> 
>                  switch (dir) {
>                  case IIO_EV_DIR_RISING:
>                          addr = AD7280A_AUX_ADC_OVERVOLTAGE_REG;
>                          ret = ad7280_write(st, AD7280A_DEVADDR_MASTER, 
> addr,
>                                             1, val);
>                          if (ret)
>                                  break;
>                          st->aux_threshhigh = val;
>                                               ^^
> But val is being used here rather than value
> 
>                          break;
>                  case IIO_EV_DIR_FALLING:
>                          addr = AD7280A_AUX_ADC_UNDERVOLTAGE_REG;
>                          ret = ad7280_write(st, AD7280A_DEVADDR_MASTER, 
> addr,
>                                             1, val);
>                          if (ret)
>                                  break;
>                          st->aux_threshlow = val;
>                                              ^^
> and val us being used here rather than value too
> 
> 
> So for the IIO_TEMP case either the assignment to value is redundant or 
> the setting of st->aux_threshhigh or st->auxthreashlow is incorrect.

Good spot Colin.

There is clearly something wrong here. Looking back at the patch where
I refactored this it looks to me like I messed up a variable rename
and all those writes + setting of aux_threshlow and similar should be using
value, not val.
https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git/commit/?h=togreg&id=112bf4aa4afb5608d77ac9208758528bcdfae70d

The modeling we used to verify that particular feature 'worked'
was rather minimal so missed this.

I'll roll a patch an send out shortly.  No huge rush as fix can go in during
the rc phase of 5.18 if we don't get it reviewed fast enough to make the
merge window.

Thanks,

Jonathan


> 
> Colin