| Message ID | 20200908224006.25636-28-digetx@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Improvements for Tegra I2C driver | expand |
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
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
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>
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
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 --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);