diff mbox series

[v7,27/34] i2c: tegra: Check errors for both positive and negative values

Message ID 20200908224006.25636-28-digetx@gmail.com
State Superseded
Headers show
Series Improvements for Tegra I2C driver | expand

Commit Message

Dmitry Osipenko Sept. 8, 2020, 10:39 p.m. UTC
The driver's code is inconsistent in regards to the error values checking.
The correct way should be to check both positive and negative values.
This patch cleans up the error-checks in the code. Note that the
pm_runtime_get_sync() could return positive value on success, hence only
relevant parts of the code are changed by this patch.

Reviewed-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>
Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
---
 drivers/i2c/busses/i2c-tegra.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

Comments

Thierry Reding Sept. 17, 2020, 12:09 p.m. UTC | #1
On Wed, Sep 09, 2020 at 01:39:59AM +0300, Dmitry Osipenko wrote:
> The driver's code is inconsistent in regards to the error values checking.
> The correct way should be to check both positive and negative values.
> This patch cleans up the error-checks in the code. Note that the
> pm_runtime_get_sync() could return positive value on success, hence only
> relevant parts of the code are changed by this patch.
> 
> Reviewed-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>
> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
> ---
>  drivers/i2c/busses/i2c-tegra.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)

Why? All of these functions "return 0 on success or a negative error
code on failure", don't they?

I would actually argue that all of the other checks are wrong. As you
mention yourself, some functions may decide to return positive values on
success to convey some extra information, so making this just check for
non-zero carries a risk of its own.

Thierry
Andy Shevchenko Sept. 17, 2020, 1:50 p.m. UTC | #2
On Thu, Sep 17, 2020 at 3:09 PM Thierry Reding <thierry.reding@gmail.com> wrote:
> On Wed, Sep 09, 2020 at 01:39:59AM +0300, Dmitry Osipenko wrote:


> Why? All of these functions "return 0 on success or a negative error

> code on failure", don't they?


And what is the point of having ' < 0' in all those cases?

-- 
With Best Regards,
Andy Shevchenko
Thierry Reding Sept. 21, 2020, 10:20 a.m. UTC | #3
On Wed, 09 Sep 2020 01:39:59 +0300, Dmitry Osipenko wrote:
> The driver's code is inconsistent in regards to the error values checking.
> The correct way should be to check both positive and negative values.
> This patch cleans up the error-checks in the code. Note that the
> pm_runtime_get_sync() could return positive value on success, hence only
> relevant parts of the code are changed by this patch.
> 
> Reviewed-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>
> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
> ---
>  drivers/i2c/busses/i2c-tegra.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)

Tested-by: Thierry Reding <treding@nvidia.com>
Thierry Reding Sept. 21, 2020, 11:24 a.m. UTC | #4
On Thu, Sep 17, 2020 at 04:50:06PM +0300, Andy Shevchenko wrote:
> On Thu, Sep 17, 2020 at 3:09 PM Thierry Reding <thierry.reding@gmail.com> wrote:
> > On Wed, Sep 09, 2020 at 01:39:59AM +0300, Dmitry Osipenko wrote:
> 
> > Why? All of these functions "return 0 on success or a negative error
> > code on failure", don't they?
> 
> And what is the point of having ' < 0' in all those cases?

It's explicitly checking for the documented error cases. And you'll
occasionally have a function that can return non-zero on success.
Testing for < 0 is the safest way to check for failure in the majority
of cases.

Thierry
Dmitry Osipenko Sept. 21, 2020, 2:13 p.m. UTC | #5
21.09.2020 14:24, Thierry Reding пишет:
> On Thu, Sep 17, 2020 at 04:50:06PM +0300, Andy Shevchenko wrote:
>> On Thu, Sep 17, 2020 at 3:09 PM Thierry Reding <thierry.reding@gmail.com> wrote:
>>> On Wed, Sep 09, 2020 at 01:39:59AM +0300, Dmitry Osipenko wrote:
>>
>>> Why? All of these functions "return 0 on success or a negative error
>>> code on failure", don't they?
>>
>> And what is the point of having ' < 0' in all those cases?
> 
> It's explicitly checking for the documented error cases. And you'll
> occasionally have a function that can return non-zero on success.
> Testing for < 0 is the safest way to check for failure in the majority
> of cases.

If you're testing only for negative errors, then it means that you will
miss wrong positive errors, potentially setting machine on fire :) This
is not an often problem for kernel, but this is a problem that I
experienced with userspace more than one time.

Anyways, this patch also makes the errors checking consistent across the
whole driver and it makes the code look cleaner, so I'll prefer to keep
this patch as-is.
diff mbox series

Patch

diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c
index 2f39366b6d55..fe672cfebe12 100644
--- a/drivers/i2c/busses/i2c-tegra.c
+++ b/drivers/i2c/busses/i2c-tegra.c
@@ -981,7 +981,7 @@  static int tegra_i2c_config_fifo_trig(struct tegra_i2c_dev *i2c_dev, size_t len)
 
 		slv_config.device_fc = true;
 		ret = dmaengine_slave_config(chan, &slv_config);
-		if (ret < 0) {
+		if (ret) {
 			dev_err(i2c_dev->dev, "DMA slave config failed: %d\n",
 				ret);
 			return ret;
@@ -1224,7 +1224,7 @@  static int tegra_i2c_xfer_msg(struct tegra_i2c_dev *i2c_dev,
 						   xfer_size,
 						   DMA_FROM_DEVICE);
 			err = tegra_i2c_dma_submit(i2c_dev, xfer_size);
-			if (err < 0) {
+			if (err) {
 				dev_err(i2c_dev->dev,
 					"starting RX DMA failed, err %d\n",
 					err);
@@ -1249,7 +1249,7 @@  static int tegra_i2c_xfer_msg(struct tegra_i2c_dev *i2c_dev,
 						   xfer_size,
 						   DMA_TO_DEVICE);
 			err = tegra_i2c_dma_submit(i2c_dev, xfer_size);
-			if (err < 0) {
+			if (err) {
 				dev_err(i2c_dev->dev,
 					"starting TX DMA failed, err %d\n",
 					err);