[PATCH] iio: adc: ina2xx: fix missing break statement

[PATCH] iio: adc: ina2xx: fix missing break statement

[Colin King]

From: Colin Ian King <colin.king@canonical.com>

The IIO_CHAN_INFO_SCALE case is missing a break statement and in
the unlikely event that chan->address is not matched in the nested
switch statement then the code falls through to the following
IIO_CHAN_INFO_HARDWAREGAIN case.  Fix this by adding the missing
break.   While we are fixing this, it's probably a good idea to
add in a break statement to the IIO_CHAN_INFO_HARDWAREGAIN case
too (this is a moot point).

Detected by CoverityScan, CID#1462408 ("Missing break in switch")

Fixes: ca6a2d86acae ("iio: adc: ina2xx: Allow setting Shunt Voltage PGA gain and Bus Voltage range")
Signed-off-by: Colin Ian King <colin.king@canonical.com>
---
 drivers/iio/adc/ina2xx-adc.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/iio/adc/ina2xx-adc.c b/drivers/iio/adc/ina2xx-adc.c
index d1239624187d..9bc5986780b9 100644
--- a/drivers/iio/adc/ina2xx-adc.c
+++ b/drivers/iio/adc/ina2xx-adc.c
@@ -250,6 +250,7 @@ static int ina2xx_read_raw(struct iio_dev *indio_dev,
 			*val2 = chip->shunt_resistor_uohm;
 			return IIO_VAL_FRACTIONAL;
 		}
+		break;
 
 	case IIO_CHAN_INFO_HARDWAREGAIN:
 		switch (chan->address) {
@@ -262,6 +263,7 @@ static int ina2xx_read_raw(struct iio_dev *indio_dev,
 			*val = chip->range_vbus == 32 ? 1 : 2;
 			return IIO_VAL_INT;
 		}
+		break;
 	}
 
 	return -EINVAL;
-- 
2.17.1

Re: [PATCH] iio: adc: ina2xx: fix missing break statement

[Matt Ranostay]

On Tue, Oct 9, 2018 at 5:09 AM Colin King <colin.king@canonical.com> wrote:
>
> From: Colin Ian King <colin.king@canonical.com>
>
> The IIO_CHAN_INFO_SCALE case is missing a break statement and in
> the unlikely event that chan->address is not matched in the nested
> switch statement then the code falls through to the following
> IIO_CHAN_INFO_HARDWAREGAIN case.  Fix this by adding the missing
> break.   While we are fixing this, it's probably a good idea to
> add in a break statement to the IIO_CHAN_INFO_HARDWAREGAIN case
> too (this is a moot point).
>
> Detected by CoverityScan, CID#1462408 ("Missing break in switch")

I'm not familiar with running Coverity scans myself, but is this CID
some publicly accessible report?
If it is an in-house scan then it should be dropped IMHO

- Matt

>
> Fixes: ca6a2d86acae ("iio: adc: ina2xx: Allow setting Shunt Voltage PGA gain and Bus Voltage range")
> Signed-off-by: Colin Ian King <colin.king@canonical.com>
> ---
>  drivers/iio/adc/ina2xx-adc.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/drivers/iio/adc/ina2xx-adc.c b/drivers/iio/adc/ina2xx-adc.c
> index d1239624187d..9bc5986780b9 100644
> --- a/drivers/iio/adc/ina2xx-adc.c
> +++ b/drivers/iio/adc/ina2xx-adc.c
> @@ -250,6 +250,7 @@ static int ina2xx_read_raw(struct iio_dev *indio_dev,
>                         *val2 = chip->shunt_resistor_uohm;
>                         return IIO_VAL_FRACTIONAL;
>                 }
> +               break;
>
>         case IIO_CHAN_INFO_HARDWAREGAIN:
>                 switch (chan->address) {
> @@ -262,6 +263,7 @@ static int ina2xx_read_raw(struct iio_dev *indio_dev,
>                         *val = chip->range_vbus == 32 ? 1 : 2;
>                         return IIO_VAL_INT;
>                 }
> +               break;
>         }
>
>         return -EINVAL;
> --
> 2.17.1
>

Re: [PATCH] iio: adc: ina2xx: fix missing break statement

[Colin Ian King]

On 10/10/18 08:51, Matt Ranostay wrote:
> On Tue, Oct 9, 2018 at 5:09 AM Colin King <colin.king@canonical.com> wrote:
>>
>> From: Colin Ian King <colin.king@canonical.com>
>>
>> The IIO_CHAN_INFO_SCALE case is missing a break statement and in
>> the unlikely event that chan->address is not matched in the nested
>> switch statement then the code falls through to the following
>> IIO_CHAN_INFO_HARDWAREGAIN case.  Fix this by adding the missing
>> break.   While we are fixing this, it's probably a good idea to
>> add in a break statement to the IIO_CHAN_INFO_HARDWAREGAIN case
>> too (this is a moot point).
>>
>> Detected by CoverityScan, CID#1462408 ("Missing break in switch")
> 
> I'm not familiar with running Coverity scans myself, but is this CID
> some publicly accessible report?
> If it is an in-house scan then it should be dropped IMHO

It is publicly accessible:

https://scan.coverity.com/projects/linux-next-weekly-scan

Colin

> 
> - Matt
> 
>>
>> Fixes: ca6a2d86acae ("iio: adc: ina2xx: Allow setting Shunt Voltage PGA gain and Bus Voltage range")
>> Signed-off-by: Colin Ian King <colin.king@canonical.com>
>> ---
>>  drivers/iio/adc/ina2xx-adc.c | 2 ++
>>  1 file changed, 2 insertions(+)
>>
>> diff --git a/drivers/iio/adc/ina2xx-adc.c b/drivers/iio/adc/ina2xx-adc.c
>> index d1239624187d..9bc5986780b9 100644
>> --- a/drivers/iio/adc/ina2xx-adc.c
>> +++ b/drivers/iio/adc/ina2xx-adc.c
>> @@ -250,6 +250,7 @@ static int ina2xx_read_raw(struct iio_dev *indio_dev,
>>                         *val2 = chip->shunt_resistor_uohm;
>>                         return IIO_VAL_FRACTIONAL;
>>                 }
>> +               break;
>>
>>         case IIO_CHAN_INFO_HARDWAREGAIN:
>>                 switch (chan->address) {
>> @@ -262,6 +263,7 @@ static int ina2xx_read_raw(struct iio_dev *indio_dev,
>>                         *val = chip->range_vbus == 32 ? 1 : 2;
>>                         return IIO_VAL_INT;
>>                 }
>> +               break;
>>         }
>>
>>         return -EINVAL;
>> --
>> 2.17.1
>>

Re: [PATCH] iio: adc: ina2xx: fix missing break statement

[Stefan Brüns]

[-- Attachment #1: Type: text/plain, Size: 1767 bytes --]

On Montag, 8. Oktober 2018 23:09:04 CEST Colin King wrote:
> From: Colin Ian King <colin.king@canonical.com>
> 
> The IIO_CHAN_INFO_SCALE case is missing a break statement and in
> the unlikely event that chan->address is not matched in the nested
> switch statement then the code falls through to the following
> IIO_CHAN_INFO_HARDWAREGAIN case.  Fix this by adding the missing
> break.   While we are fixing this, it's probably a good idea to
> add in a break statement to the IIO_CHAN_INFO_HARDWAREGAIN case
> too (this is a moot point).
> 
> Detected by CoverityScan, CID#1462408 ("Missing break in switch")

Although it is good for code clarity to add a break statement, the code can 
never return anything but -EINVAL in case chan->address is not handled in 
IIO_CHAN_INFO_SCALE:

-----
switch (mask) {
case IIO_CHAN_INFO_SCALE:
   switch (chan->address) {
       case INA2XX_SHUNT_VOLTAGE:
       ... return IIO_VAL_FRACTIONAL;
       
       case INA2XX_BUS_VOLTAGE:
       ... return IIO_VAL_FRACTIONAL;

       case INA2XX_CURRENT:
       ... return IIO_VAL_FRACTIONAL;

       case INA2XX_POWER:
       ... return IIO_VAL_FRACTIONAL;
   }

case IIO_CHAN_INFO_HARDWAREGAIN:
   switch (chan->address) {
       case INA2XX_SHUNT_VOLTAGE:
       ... return IIO_VAL_FRACTIONAL;
       
       case INA2XX_BUS_VOLTAGE:
       ... return IIO_VAL_INT;
   }
}
return -EINVAL;
-----

The addresses handled in INFO_HARDWAREGAIN is a subset of the ones in 
INFO_SCALE.

I would prefer an early "return -EINVAL" here, as it matches better with the 
other "switch (mask)" cases above.

Kind regards,

Stefan

-- 
Stefan Brüns  /  Bergstraße 21  /  52062 Aachen
home: +49 241 53809034     mobile: +49 151 50412019

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

Re: [PATCH] iio: adc: ina2xx: fix missing break statement

[Jonathan Cameron]

On Wed, 10 Oct 2018 12:42:39 +0200
Stefan Brüns <stefan.bruens@rwth-aachen.de> wrote:

> On Montag, 8. Oktober 2018 23:09:04 CEST Colin King wrote:
> > From: Colin Ian King <colin.king@canonical.com>
> > 
> > The IIO_CHAN_INFO_SCALE case is missing a break statement and in
> > the unlikely event that chan->address is not matched in the nested
> > switch statement then the code falls through to the following
> > IIO_CHAN_INFO_HARDWAREGAIN case.  Fix this by adding the missing
> > break.   While we are fixing this, it's probably a good idea to
> > add in a break statement to the IIO_CHAN_INFO_HARDWAREGAIN case
> > too (this is a moot point).
> > 
> > Detected by CoverityScan, CID#1462408 ("Missing break in switch")  
> 
> Although it is good for code clarity to add a break statement, the code can 
> never return anything but -EINVAL in case chan->address is not handled in 
> IIO_CHAN_INFO_SCALE:
> 
> -----
> switch (mask) {
> case IIO_CHAN_INFO_SCALE:
>    switch (chan->address) {
>        case INA2XX_SHUNT_VOLTAGE:
>        ... return IIO_VAL_FRACTIONAL;
>        
>        case INA2XX_BUS_VOLTAGE:
>        ... return IIO_VAL_FRACTIONAL;
> 
>        case INA2XX_CURRENT:
>        ... return IIO_VAL_FRACTIONAL;
> 
>        case INA2XX_POWER:
>        ... return IIO_VAL_FRACTIONAL;
>    }
> 
> case IIO_CHAN_INFO_HARDWAREGAIN:
>    switch (chan->address) {
>        case INA2XX_SHUNT_VOLTAGE:
>        ... return IIO_VAL_FRACTIONAL;
>        
>        case INA2XX_BUS_VOLTAGE:
>        ... return IIO_VAL_INT;
>    }
> }
> return -EINVAL;
> -----
> 
> The addresses handled in INFO_HARDWAREGAIN is a subset of the ones in 
> INFO_SCALE.
> 
> I would prefer an early "return -EINVAL" here, as it matches better with the 
> other "switch (mask)" cases above.
> 
> Kind regards,
> 
> Stefan

I agree with Stefan on this.  It is more in keeping with the local
style to use a direct return.

Colin, would you mind doing a v2 with that approach?

If not I'll get to it at somepoint if no one else does, but it may
take some time!

Please also change the title to make it clear that this is beyond unlikely
as I think it is impossible (without a gross bug somewhere else).
This is worthwhile as an improvement to code clarity and false warning
suppression, but it's not a fix I want to be pushed back to ancient kernels
as in that circumstance it's really just noise.

Jonathan

>