* re: iio:adc:ad7280a: Move out of staging
@ 2022-02-24 22:56 Colin King (gmail)
2022-02-26 18:01 ` Jonathan Cameron
2022-03-05 2:32 ` Marcelo Schmitt
0 siblings, 2 replies; 3+ messages in thread
From: Colin King (gmail) @ 2022-02-24 22:56 UTC (permalink / raw)
To: Jonathan Cameron; +Cc: Marcelo Schmitt, linux-iio, linux-kernel
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
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: iio:adc:ad7280a: Move out of staging
2022-02-24 22:56 iio:adc:ad7280a: Move out of staging Colin King (gmail)
@ 2022-02-26 18:01 ` Jonathan Cameron
2022-03-05 2:32 ` Marcelo Schmitt
1 sibling, 0 replies; 3+ messages in thread
From: Jonathan Cameron @ 2022-02-26 18:01 UTC (permalink / raw)
To: Colin King (gmail); +Cc: Marcelo Schmitt, linux-iio, linux-kernel
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
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: iio:adc:ad7280a: Move out of staging
2022-02-24 22:56 iio:adc:ad7280a: Move out of staging Colin King (gmail)
2022-02-26 18:01 ` Jonathan Cameron
@ 2022-03-05 2:32 ` Marcelo Schmitt
1 sibling, 0 replies; 3+ messages in thread
From: Marcelo Schmitt @ 2022-03-05 2:32 UTC (permalink / raw)
To: Colin King (gmail); +Cc: Jonathan Cameron, linux-iio, linux-kernel
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
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2022-03-05 2:32 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-02-24 22:56 iio:adc:ad7280a: Move out of staging Colin King (gmail)
2022-02-26 18:01 ` Jonathan Cameron
2022-03-05 2:32 ` Marcelo Schmitt
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).