| Message ID | 72d5aaef3445edaca08498b4d7aaff19d4ac0232.1521037060.git.rodrigosiqueiramelo@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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
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
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
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
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 --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 */