diff mbox

[7/7] staging:iio:ade7854: Add proper error handling condition

Message ID 72d5aaef3445edaca08498b4d7aaff19d4ac0232.1521037060.git.rodrigosiqueiramelo@gmail.com (mailing list archive)
State New, archived
Headers show

Commit Message

Rodrigo Siqueira March 14, 2018, 6:12 p.m. UTC
There is some improper error handling for IRQ and device register.  This
patch adds a proper verification. The IRQ correction was extracted from
John Syne patches.

Signed-off-by: Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com>
Signed-off-by: John Syne <john3909@gmail.com>
---
 drivers/staging/iio/meter/ade7854.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Comments

Dan Carpenter March 15, 2018, 12:11 p.m. UTC | #1
On Wed, Mar 14, 2018 at 03:12:18PM -0300, Rodrigo Siqueira wrote:
> There is some improper error handling for IRQ and device register.  This
> patch adds a proper verification. The IRQ correction was extracted from
> John Syne patches.
> 
> Signed-off-by: Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com>
> Signed-off-by: John Syne <john3909@gmail.com>
> ---
>  drivers/staging/iio/meter/ade7854.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/staging/iio/meter/ade7854.c b/drivers/staging/iio/meter/ade7854.c
> index 09fd8c067738..49cbe365e43d 100644
> --- a/drivers/staging/iio/meter/ade7854.c
> +++ b/drivers/staging/iio/meter/ade7854.c
> @@ -436,7 +436,7 @@ static int ade7854_initial_setup(struct iio_dev *indio_dev)
>  
>  	/* Disable IRQ */
>  	ret = ade7854_set_irq(dev, false);
> -	if (ret) {
> +	if (ret < 0) {
>  		dev_err(dev, "disable irq failed");
>  		goto err_ret;
>  	}

Why is the original wrong?  It seems fine.

regards,
dan carpenter


--
To unsubscribe from this list: send the line "unsubscribe linux-iio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Rodrigo Siqueira March 16, 2018, 12:28 a.m. UTC | #2
On 03/15, Dan Carpenter wrote:
> On Wed, Mar 14, 2018 at 03:12:18PM -0300, Rodrigo Siqueira wrote:
> > There is some improper error handling for IRQ and device register.  This
> > patch adds a proper verification. The IRQ correction was extracted from
> > John Syne patches.
> > 
> > Signed-off-by: Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com>
> > Signed-off-by: John Syne <john3909@gmail.com>
> > ---
> >  drivers/staging/iio/meter/ade7854.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/staging/iio/meter/ade7854.c b/drivers/staging/iio/meter/ade7854.c
> > index 09fd8c067738..49cbe365e43d 100644
> > --- a/drivers/staging/iio/meter/ade7854.c
> > +++ b/drivers/staging/iio/meter/ade7854.c
> > @@ -436,7 +436,7 @@ static int ade7854_initial_setup(struct iio_dev *indio_dev)
> >  
> >  	/* Disable IRQ */
> >  	ret = ade7854_set_irq(dev, false);
> > -	if (ret) {
> > +	if (ret < 0) {
> >  		dev_err(dev, "disable irq failed");
> >  		goto err_ret;
> >  	}
> 
> Why is the original wrong?  It seems fine.

Hi,

If you look at "drivers/staging/iio/meter/ade7854-(i2c|spi).c", you will
find a line like this: " st->write_reg = ade7854_(i2c|spi)_write_reg;".
Than, if you go through the "ade7854_i2c_write_reg" (focus only on i2c
for while) you will see "i2c_master_send(st->i2c, st->tx, count)",
which can returns a positive number that does not necessary means an
error. If you find the same for the spi case, you will end up in the
function "spi_sync_transfer" that only return 0 for success or negative
for failure. So, handling the negative case better fit the i2c and spi case.

Thanks

> regards,
> dan carpenter
> 
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-iio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Dan Carpenter March 16, 2018, 8:02 a.m. UTC | #3
You're right that there is a bug but this is not the right fix.

The ade7854_i2c_write_reg_32() function returns 6 on success which makes
no sense.  It should be zero or negative error codes.  All the write_reg
functions in drivers/staging/iio/meter/ade7854-i2c.c have the same bug.

Please, fix that instead and leave ade7854_initial_setup() alone.

regards,
dan carpenter

--
To unsubscribe from this list: send the line "unsubscribe linux-iio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Rodrigo Siqueira March 16, 2018, 1:59 p.m. UTC | #4
On 03/16, Dan Carpenter wrote:
> You're right that there is a bug but this is not the right fix.
> 
> The ade7854_i2c_write_reg_32() function returns 6 on success which makes
> no sense.  It should be zero or negative error codes.  All the write_reg
> functions in drivers/staging/iio/meter/ade7854-i2c.c have the same bug.
> 
> Please, fix that instead and leave ade7854_initial_setup() alone.

I see. However, I think the following steps could be better:

1) Update ade7854_i2c_write_reg() in order to return 0

The primary objective of this patchset it removes the duplications
related to write_reg_* and read_reg_*. As a result, in the first patch
create the function ade7854_i2c_write_reg(). I will add the following
code to fix the problem that we are discussing:

ade7854_i2c_write_reg() {
...
ret = i2c_master_send(st->i2c, st->tx, count);
...
return ret < 0 ? ret : 0;
}

With this, I do not need to touch any of the
ade7854_i2c_write_reg_(8|16|24|32) functions. 

2) Drop out the last patch from the patchset

Is that ok? If so, I will send the V2 today.
 
> regards,
> dan carpenter
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-iio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Dan Carpenter March 16, 2018, 2:17 p.m. UTC | #5
Generally, it's better to fix the bug in the existing code, and then
do the cleanup later.  That way the fixes can be backported to stable
kernels more easily.

I don't know this subsystem very well.  Perhaps Jonathan doesn't care
for one reason or another (like maybe he's not going to back port the
fix).  So it might not matter how you do it.

regards,
dan carpenter


--
To unsubscribe from this list: send the line "unsubscribe linux-iio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Rodrigo Siqueira March 16, 2018, 8:03 p.m. UTC | #6
On 03/16, Dan Carpenter wrote:
> Generally, it's better to fix the bug in the existing code, and then
> do the cleanup later.  That way the fixes can be backported to stable
> kernels more easily.

Hummm... the backport argue make a lot of sense for me. I will generate
a v2 with the bug fixes in the beginning.

Thanks 
> I don't know this subsystem very well.  Perhaps Jonathan doesn't care
> for one reason or another (like maybe he's not going to back port the
> fix).  So it might not matter how you do it.
> 
> regards,
> dan carpenter
> 
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-iio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/drivers/staging/iio/meter/ade7854.c b/drivers/staging/iio/meter/ade7854.c
index 09fd8c067738..49cbe365e43d 100644
--- a/drivers/staging/iio/meter/ade7854.c
+++ b/drivers/staging/iio/meter/ade7854.c
@@ -436,7 +436,7 @@  static int ade7854_initial_setup(struct iio_dev *indio_dev)
 
 	/* Disable IRQ */
 	ret = ade7854_set_irq(dev, false);
-	if (ret) {
+	if (ret < 0) {
 		dev_err(dev, "disable irq failed");
 		goto err_ret;
 	}
@@ -544,7 +544,7 @@  int ade7854_probe(struct iio_dev *indio_dev, struct device *dev)
 	indio_dev->modes = INDIO_DIRECT_MODE;
 
 	ret = devm_iio_device_register(dev, indio_dev);
-	if (ret)
+	if (ret < 0)
 		return ret;
 
 	/* Get the device into a sane initial state */