[PATCH v3] staging: iio: meter: add check on return variables

[PATCH v3] staging: iio: meter: add check on return variables

[Aya Mahfouz]

Adds checks on variables that are used to return values. If
the value is less than zero, this indicates that an error
occured and hence a message is printed through dev_err. Labels
have been added too, since the check is done more than once.

Signed-off-by: Aya Mahfouz <mahfouz.saif.elyazal@gmail.com>
---
v1: added checks on last calls only.
v2: added checks on all calls that return values along with labels.
v3: added changelog that was missed in v2.

 drivers/staging/iio/meter/ade7758_core.c | 35 ++++++++++++++++++++------------
 1 file changed, 22 insertions(+), 13 deletions(-)

diff --git a/drivers/staging/iio/meter/ade7758_core.c b/drivers/staging/iio/meter/ade7758_core.c
index 70e96b2..cab392f 100644
--- a/drivers/staging/iio/meter/ade7758_core.c
+++ b/drivers/staging/iio/meter/ade7758_core.c
@@ -303,14 +303,18 @@ static int ade7758_reset(struct device *dev)
 	int ret;
 	u8 val;
 
-	ade7758_spi_read_reg_8(dev,
-			ADE7758_OPMODE,
-			&val);
+	ret = ade7758_spi_read_reg_8(dev, ADE7758_OPMODE, &val);
+	if (ret < 0) {
+		dev_err(dev, "failed to read from device");
+		goto error_ret;
+	}
 	val |= 1 << 6; /* Software Chip Reset */
-	ret = ade7758_spi_write_reg_8(dev,
-			ADE7758_OPMODE,
-			val);
-
+	ret = ade7758_spi_write_reg_8(dev, ADE7758_OPMODE, val);
+	if (ret < 0) {
+		dev_err(dev, "failed to reset device");
+		goto error_ret;
+	}
+error_ret:
 	return ret;
 }
 
@@ -444,14 +448,19 @@ static int ade7758_stop_device(struct device *dev)
 	int ret;
 	u8 val;
 
-	ade7758_spi_read_reg_8(dev,
-			ADE7758_OPMODE,
-			&val);
+	ret = ade7758_spi_read_reg_8(dev, ADE7758_OPMODE, &val);
+	if (ret < 0) {
+		dev_err(dev, "failed to read from device");
+		goto error_ret;
+	}
 	val |= 7 << 3;  /* ADE7758 powered down */
-	ret = ade7758_spi_write_reg_8(dev,
-			ADE7758_OPMODE,
-			val);
+	ret = ade7758_spi_write_reg_8(dev, ADE7758_OPMODE, val);
+	if (ret < 0) {
+		dev_err(dev, "failed to stop device");
+		goto error_ret;
+	}
 
+error_ret:
 	return ret;
 }
 
-- 
1.9.3


-- 
Kind Regards,
Aya Saif El-yazal Mahfouz

Re: [Outreachy kernel] [PATCH v3] staging: iio: meter: add check on return variables

[Daniel Baluta]

+ list :)

On Tue, Mar 3, 2015 at 1:57 PM, Daniel Baluta <daniel.baluta@gmail.com> wrote:
> On Tue, Mar 3, 2015 at 1:27 PM, Aya Mahfouz
> <mahfouz.saif.elyazal@gmail.com> wrote:
>> Adds checks on variables that are used to return values. If
>> the value is less than zero, this indicates that an error
>> occured and hence a message is printed through dev_err. Labels
>> have been added too, since the check is done more than once.
>>
>> Signed-off-by: Aya Mahfouz <mahfouz.saif.elyazal@gmail.com>
>> ---
>> v1: added checks on last calls only.
>> v2: added checks on all calls that return values along with labels.
>> v3: added changelog that was missed in v2.
>>
>>  drivers/staging/iio/meter/ade7758_core.c | 35 ++++++++++++++++++++------------
>>  1 file changed, 22 insertions(+), 13 deletions(-)
>>
>> diff --git a/drivers/staging/iio/meter/ade7758_core.c b/drivers/staging/iio/meter/ade7758_core.c
>> index 70e96b2..cab392f 100644
>> --- a/drivers/staging/iio/meter/ade7758_core.c
>> +++ b/drivers/staging/iio/meter/ade7758_core.c
>> @@ -303,14 +303,18 @@ static int ade7758_reset(struct device *dev)
>>         int ret;
>>         u8 val;
>>
>> -       ade7758_spi_read_reg_8(dev,
>> -                       ADE7758_OPMODE,
>> -                       &val);
>> +       ret = ade7758_spi_read_reg_8(dev, ADE7758_OPMODE, &val);
>> +       if (ret < 0) {
>> +               dev_err(dev, "failed to read from device");
>> +               goto error_ret;
>> +       }
>>         val |= 1 << 6; /* Software Chip Reset */
>> -       ret = ade7758_spi_write_reg_8(dev,
>> -                       ADE7758_OPMODE,
>> -                       val);
>> -
>> +       ret = ade7758_spi_write_reg_8(dev, ADE7758_OPMODE, val);
>> +       if (ret < 0) {
>> +               dev_err(dev, "failed to reset device");
>> +               goto error_ret;
>> +       }
>> +error_ret:
>>         return ret;
>>  }
>>
>> @@ -444,14 +448,19 @@ static int ade7758_stop_device(struct device *dev)
>>         int ret;
>>         u8 val;
>>
>> -       ade7758_spi_read_reg_8(dev,
>> -                       ADE7758_OPMODE,
>> -                       &val);
>> +       ret = ade7758_spi_read_reg_8(dev, ADE7758_OPMODE, &val);
>> +       if (ret < 0) {
>> +               dev_err(dev, "failed to read from device");
>> +               goto error_ret;
>> +       }
>>         val |= 7 << 3;  /* ADE7758 powered down */
>> -       ret = ade7758_spi_write_reg_8(dev,
>> -                       ADE7758_OPMODE,
>> -                       val);
>> +       ret = ade7758_spi_write_reg_8(dev, ADE7758_OPMODE, val);
>> +       if (ret < 0) {
>> +               dev_err(dev, "failed to stop device");
>> +               goto error_ret;
>> +       }
>>
>> +error_ret:
>>         return ret;
>>  }
>
> Is there any particular reason for which you preferred gotos here?
> I don't think we need them. Just directly return ret;.
>
> Daniel.

Re: [Outreachy kernel] [PATCH v3] staging: iio: meter: add check on return variables

[Julia Lawall]

On Tue, 3 Mar 2015, Aya Mahfouz wrote:

> Adds checks on variables that are used to return values. If
> the value is less than zero, this indicates that an error
> occured and hence a message is printed through dev_err. Labels
> have been added too, since the check is done more than once.

How did you find the functions that have this property?

julia

> Signed-off-by: Aya Mahfouz <mahfouz.saif.elyazal@gmail.com>
> ---
> v1: added checks on last calls only.
> v2: added checks on all calls that return values along with labels.
> v3: added changelog that was missed in v2.
>
>  drivers/staging/iio/meter/ade7758_core.c | 35 ++++++++++++++++++++------------
>  1 file changed, 22 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/staging/iio/meter/ade7758_core.c b/drivers/staging/iio/meter/ade7758_core.c
> index 70e96b2..cab392f 100644
> --- a/drivers/staging/iio/meter/ade7758_core.c
> +++ b/drivers/staging/iio/meter/ade7758_core.c
> @@ -303,14 +303,18 @@ static int ade7758_reset(struct device *dev)
>  	int ret;
>  	u8 val;
>
> -	ade7758_spi_read_reg_8(dev,
> -			ADE7758_OPMODE,
> -			&val);
> +	ret = ade7758_spi_read_reg_8(dev, ADE7758_OPMODE, &val);
> +	if (ret < 0) {
> +		dev_err(dev, "failed to read from device");
> +		goto error_ret;
> +	}
>  	val |= 1 << 6; /* Software Chip Reset */
> -	ret = ade7758_spi_write_reg_8(dev,
> -			ADE7758_OPMODE,
> -			val);
> -
> +	ret = ade7758_spi_write_reg_8(dev, ADE7758_OPMODE, val);
> +	if (ret < 0) {
> +		dev_err(dev, "failed to reset device");
> +		goto error_ret;
> +	}
> +error_ret:
>  	return ret;
>  }
>
> @@ -444,14 +448,19 @@ static int ade7758_stop_device(struct device *dev)
>  	int ret;
>  	u8 val;
>
> -	ade7758_spi_read_reg_8(dev,
> -			ADE7758_OPMODE,
> -			&val);
> +	ret = ade7758_spi_read_reg_8(dev, ADE7758_OPMODE, &val);
> +	if (ret < 0) {
> +		dev_err(dev, "failed to read from device");
> +		goto error_ret;
> +	}
>  	val |= 7 << 3;  /* ADE7758 powered down */
> -	ret = ade7758_spi_write_reg_8(dev,
> -			ADE7758_OPMODE,
> -			val);
> +	ret = ade7758_spi_write_reg_8(dev, ADE7758_OPMODE, val);
> +	if (ret < 0) {
> +		dev_err(dev, "failed to stop device");
> +		goto error_ret;
> +	}
>
> +error_ret:
>  	return ret;
>  }
>
> --
> 1.9.3
>
>
> --
> Kind Regards,
> Aya Saif El-yazal Mahfouz
>
> --
> You received this message because you are subscribed to the Google Groups "outreachy-kernel" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to outreachy-kernel+unsubscribe@googlegroups.com.
> To post to this group, send email to outreachy-kernel@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/outreachy-kernel/20150303112705.GA11879%40waves.
> For more options, visit https://groups.google.com/d/optout.
>

Re: [Outreachy kernel] [PATCH v3] staging: iio: meter: add check on return variables

[Arnd Bergmann]

On Tuesday 03 March 2015 07:01:47 Julia Lawall wrote:
> > -     ade7758_spi_read_reg_8(dev,
> > -                     ADE7758_OPMODE,
> > -                     &val);
> > +     ret = ade7758_spi_read_reg_8(dev, ADE7758_OPMODE, &val);
> > +     if (ret < 0) {
> > +             dev_err(dev, "failed to read from device");
> > +             goto error_ret;
> > +     }
> 

The ade7758_spi_read_reg_8 already prints an error message, I don't
think we want to see two messages about the same error.

	Arnd

Re: [Outreachy kernel] [PATCH v3] staging: iio: meter: add check on return variables

[Aya Mahfouz]

On Tue, Mar 03, 2015 at 01:13:09PM +0100, Arnd Bergmann wrote:
> On Tuesday 03 March 2015 07:01:47 Julia Lawall wrote:
> > > -     ade7758_spi_read_reg_8(dev,
> > > -                     ADE7758_OPMODE,
> > > -                     &val);
> > > +     ret = ade7758_spi_read_reg_8(dev, ADE7758_OPMODE, &val);
> > > +     if (ret < 0) {
> > > +             dev_err(dev, "failed to read from device");
> > > +             goto error_ret;
> > > +     }
> > 
> 
> The ade7758_spi_read_reg_8 already prints an error message, I don't
> think we want to see two messages about the same error.
>
good to know. Thanks Arnd!
> 	Arnd

-- 
Kind Regards,
Aya Saif El-yazal Mahfouz

Re: [Outreachy kernel] [PATCH v3] staging: iio: meter: add check on return variables

[Aya Mahfouz]

On Tue, Mar 03, 2015 at 07:01:47AM -0500, Julia Lawall wrote:
> On Tue, 3 Mar 2015, Aya Mahfouz wrote:
> 
> > Adds checks on variables that are used to return values. If
> > the value is less than zero, this indicates that an error
> > occured and hence a message is printed through dev_err. Labels
> > have been added too, since the check is done more than once.
> 
> How did you find the functions that have this property?
>
Do you mean that I should state that I found it using coccinelle?
> julia
> 
> > Signed-off-by: Aya Mahfouz <mahfouz.saif.elyazal@gmail.com>
> > ---
> > v1: added checks on last calls only.
> > v2: added checks on all calls that return values along with labels.
> > v3: added changelog that was missed in v2.
> >
> >  drivers/staging/iio/meter/ade7758_core.c | 35 ++++++++++++++++++++------------
> >  1 file changed, 22 insertions(+), 13 deletions(-)
> >
> > diff --git a/drivers/staging/iio/meter/ade7758_core.c b/drivers/staging/iio/meter/ade7758_core.c
> > index 70e96b2..cab392f 100644
> > --- a/drivers/staging/iio/meter/ade7758_core.c
> > +++ b/drivers/staging/iio/meter/ade7758_core.c
> > @@ -303,14 +303,18 @@ static int ade7758_reset(struct device *dev)
> >  	int ret;
> >  	u8 val;
> >
> > -	ade7758_spi_read_reg_8(dev,
> > -			ADE7758_OPMODE,
> > -			&val);
> > +	ret = ade7758_spi_read_reg_8(dev, ADE7758_OPMODE, &val);
> > +	if (ret < 0) {
> > +		dev_err(dev, "failed to read from device");
> > +		goto error_ret;
> > +	}
> >  	val |= 1 << 6; /* Software Chip Reset */
> > -	ret = ade7758_spi_write_reg_8(dev,
> > -			ADE7758_OPMODE,
> > -			val);
> > -
> > +	ret = ade7758_spi_write_reg_8(dev, ADE7758_OPMODE, val);
> > +	if (ret < 0) {
> > +		dev_err(dev, "failed to reset device");
> > +		goto error_ret;
> > +	}
> > +error_ret:
> >  	return ret;
> >  }
> >
> > @@ -444,14 +448,19 @@ static int ade7758_stop_device(struct device *dev)
> >  	int ret;
> >  	u8 val;
> >
> > -	ade7758_spi_read_reg_8(dev,
> > -			ADE7758_OPMODE,
> > -			&val);
> > +	ret = ade7758_spi_read_reg_8(dev, ADE7758_OPMODE, &val);
> > +	if (ret < 0) {
> > +		dev_err(dev, "failed to read from device");
> > +		goto error_ret;
> > +	}
> >  	val |= 7 << 3;  /* ADE7758 powered down */
> > -	ret = ade7758_spi_write_reg_8(dev,
> > -			ADE7758_OPMODE,
> > -			val);
> > +	ret = ade7758_spi_write_reg_8(dev, ADE7758_OPMODE, val);
> > +	if (ret < 0) {
> > +		dev_err(dev, "failed to stop device");
> > +		goto error_ret;
> > +	}
> >
> > +error_ret:
> >  	return ret;
> >  }
> >
> > --
> > 1.9.3
> >
> >
> > --
> > Kind Regards,
> > Aya Saif El-yazal Mahfouz
> >
> > --
> > You received this message because you are subscribed to the Google Groups "outreachy-kernel" group.
> > To unsubscribe from this group and stop receiving emails from it, send an email to outreachy-kernel+unsubscribe@googlegroups.com.
> > To post to this group, send email to outreachy-kernel@googlegroups.com.
> > To view this discussion on the web visit https://groups.google.com/d/msgid/outreachy-kernel/20150303112705.GA11879%40waves.
> > For more options, visit https://groups.google.com/d/optout.
> >

-- 
Kind Regards,
Aya Saif El-yazal Mahfouz

Re: [Outreachy kernel] [PATCH v3] staging: iio: meter: add check on return variables

[Julia Lawall]

On Tue, 3 Mar 2015, Aya Mahfouz wrote:

> On Tue, Mar 03, 2015 at 07:01:47AM -0500, Julia Lawall wrote:
> > On Tue, 3 Mar 2015, Aya Mahfouz wrote:
> >
> > > Adds checks on variables that are used to return values. If
> > > the value is less than zero, this indicates that an error
> > > occured and hence a message is printed through dev_err. Labels
> > > have been added too, since the check is done more than once.
> >
> > How did you find the functions that have this property?
> >
> Do you mean that I should state that I found it using coccinelle?

No, I meant that there are millions offunctions used in this code, so what
motivated you to look at these ones.  But the explanation you put in a
later message is fine.

julia


> > julia
> >
> > > Signed-off-by: Aya Mahfouz <mahfouz.saif.elyazal@gmail.com>
> > > ---
> > > v1: added checks on last calls only.
> > > v2: added checks on all calls that return values along with labels.
> > > v3: added changelog that was missed in v2.
> > >
> > >  drivers/staging/iio/meter/ade7758_core.c | 35 ++++++++++++++++++++------------
> > >  1 file changed, 22 insertions(+), 13 deletions(-)
> > >
> > > diff --git a/drivers/staging/iio/meter/ade7758_core.c b/drivers/staging/iio/meter/ade7758_core.c
> > > index 70e96b2..cab392f 100644
> > > --- a/drivers/staging/iio/meter/ade7758_core.c
> > > +++ b/drivers/staging/iio/meter/ade7758_core.c
> > > @@ -303,14 +303,18 @@ static int ade7758_reset(struct device *dev)
> > >  	int ret;
> > >  	u8 val;
> > >
> > > -	ade7758_spi_read_reg_8(dev,
> > > -			ADE7758_OPMODE,
> > > -			&val);
> > > +	ret = ade7758_spi_read_reg_8(dev, ADE7758_OPMODE, &val);
> > > +	if (ret < 0) {
> > > +		dev_err(dev, "failed to read from device");
> > > +		goto error_ret;
> > > +	}
> > >  	val |= 1 << 6; /* Software Chip Reset */
> > > -	ret = ade7758_spi_write_reg_8(dev,
> > > -			ADE7758_OPMODE,
> > > -			val);
> > > -
> > > +	ret = ade7758_spi_write_reg_8(dev, ADE7758_OPMODE, val);
> > > +	if (ret < 0) {
> > > +		dev_err(dev, "failed to reset device");
> > > +		goto error_ret;
> > > +	}
> > > +error_ret:
> > >  	return ret;
> > >  }
> > >
> > > @@ -444,14 +448,19 @@ static int ade7758_stop_device(struct device *dev)
> > >  	int ret;
> > >  	u8 val;
> > >
> > > -	ade7758_spi_read_reg_8(dev,
> > > -			ADE7758_OPMODE,
> > > -			&val);
> > > +	ret = ade7758_spi_read_reg_8(dev, ADE7758_OPMODE, &val);
> > > +	if (ret < 0) {
> > > +		dev_err(dev, "failed to read from device");
> > > +		goto error_ret;
> > > +	}
> > >  	val |= 7 << 3;  /* ADE7758 powered down */
> > > -	ret = ade7758_spi_write_reg_8(dev,
> > > -			ADE7758_OPMODE,
> > > -			val);
> > > +	ret = ade7758_spi_write_reg_8(dev, ADE7758_OPMODE, val);
> > > +	if (ret < 0) {
> > > +		dev_err(dev, "failed to stop device");
> > > +		goto error_ret;
> > > +	}
> > >
> > > +error_ret:
> > >  	return ret;
> > >  }
> > >
> > > --
> > > 1.9.3
> > >
> > >
> > > --
> > > Kind Regards,
> > > Aya Saif El-yazal Mahfouz
> > >
> > > --
> > > You received this message because you are subscribed to the Google Groups "outreachy-kernel" group.
> > > To unsubscribe from this group and stop receiving emails from it, send an email to outreachy-kernel+unsubscribe@googlegroups.com.
> > > To post to this group, send email to outreachy-kernel@googlegroups.com.
> > > To view this discussion on the web visit https://groups.google.com/d/msgid/outreachy-kernel/20150303112705.GA11879%40waves.
> > > For more options, visit https://groups.google.com/d/optout.
> > >
>
> --
> Kind Regards,
> Aya Saif El-yazal Mahfouz
>