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.
Colin
On Thu, 24 Feb 2022 22:56:20 +0000
"Colin King (gmail)" <[email protected]> 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
On 02/24, Colin King (gmail) 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 catch. This has really slipped through my review.
It's impressive how we can discuss changes at one specific part of the
code and miss inconsistencies a few lines below it.
Thanks,
Marcelo
>
> Colin