| Message ID | 20240822130722.1261891-2-andriy.shevchenko@linux.intel.com (mailing list archive) |
|---|---|
| State | Rejected, archived |
| Headers | show |
| Series | platform/x86: int3472: A few cleanups | expand |
On Thu, Aug 22, 2024 at 04:05:38PM +0300, Andy Shevchenko wrote: > In the similar way, ignore 0 error code (AKA "success") in > dev_err_probe(). This helps to simplify a code such as > > if (ret < 0) > return dev_err_probe(int3472->dev, ret, err_msg); > > return ret; > > to > > return dev_err_probe(int3472->dev, ret, err_msg); > > Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
On Sat, Aug 24, 2024 at 11:08:53AM +0800, Greg Kroah-Hartman wrote: > On Thu, Aug 22, 2024 at 04:05:38PM +0300, Andy Shevchenko wrote: > > In the similar way, ignore 0 error code (AKA "success") in > > dev_err_probe(). This helps to simplify a code such as > > > > if (ret < 0) > > return dev_err_probe(int3472->dev, ret, err_msg); > > > > return ret; > > > > to > > > > return dev_err_probe(int3472->dev, ret, err_msg); > > > > Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > > Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> Thank you! Hans, I think we all set to proceed with this. Do you have any comments?
On Thu, Aug 22, 2024 at 04:05:38PM +0300, Andy Shevchenko wrote: > In the similar way, ignore 0 error code (AKA "success") in > dev_err_probe(). This helps to simplify a code such as > > if (ret < 0) > return dev_err_probe(int3472->dev, ret, err_msg); > > return ret; > > to > > return dev_err_probe(int3472->dev, ret, err_msg); > > Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> This is a terrible idea because currently Smatch is able to detect about one bug per month where someone unintentionally passes the wrong error variable to dev_err_probe(). I really hate this. NAKed-by: Dan Carpenter <dan.carpenter@linaro.org> regards, dan carpenter
On Sat, Aug 31, 2024 at 11:25:54AM +0300, Dan Carpenter wrote: > On Thu, Aug 22, 2024 at 04:05:38PM +0300, Andy Shevchenko wrote: > > In the similar way, ignore 0 error code (AKA "success") in > > dev_err_probe(). This helps to simplify a code such as > > > > if (ret < 0) > > return dev_err_probe(int3472->dev, ret, err_msg); > > > > return ret; > > > > to > > > > return dev_err_probe(int3472->dev, ret, err_msg); > > > > Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > > This is a terrible idea because currently Smatch is able to detect about one > bug per month where someone unintentionally passes the wrong error variable > to dev_err_probe(). Here are the stats since Jan 2023. All these bugs are impossible to detect now. 2024-08-12 d3bde2243d42 iio: proximity: hx9023s: Fix error code in hx9023s_property_get() 2024-07-08 101e5c5c4e76 PCI: qcom: Fix missing error code in qcom_pcie_probe() 2024-02-22 debabbb1f272 iio: adc: ti-ads1298: Fix error code in probe() 2024-01-08 9c46e3a5232d iio: adc: ad7091r8: Fix error code in ad7091r8_gpio_setup() 2023-12-04 35ddd61cf023 platform/x86: x86-android-tablets: Fix an IS_ERR() vs NULL check in probe 2023-11-20 2d37b3649c41 hwrng: starfive - Fix dev_err_probe return error 2023-11-30 03219a3aa6c8 drm/imagination: Fix error codes in pvr_device_clk_init() 2023-09-07 4b2b39f9395b watchdog: marvell_gti_wdt: Fix error code in probe() 2023-08-24 8886e1b03669 ASoC: codecs: Fix error code in aw88261_i2c_probe() 2023-06-23 ad5152b85e8b leds: aw200xx: Fix error code in probe() 2023-07-18 86fe3e9f4c63 phy: starfive: fix error code in probe 2023-07-12 0b64150c3429 Input: bcm-keypad - correct dev_err_probe() error 2023-06-26 5fb2864cbd50 OPP: Properly propagate error along when failing to get icc_path 2023-06-19 02474880e8fd ASoC: max98388: fix error code in probe() 2023-05-25 cc5f2eb7ce11 mfd: tps6594: Fix an error code in probe() 2023-05-22 1ca04f21b204 remoteproc: stm32: Fix error code in stm32_rproc_parse_dt() 2023-05-15 46f5dd7439e3 fbdev: omapfb: panel-tpo-td043mtea1: fix error code in probe() 2023-05-13 3530167c6fe8 soc: qcom: icc-bwmon: fix incorrect error code passed to dev_err_probe() 2023-05-12 bc97139ff135 power: supply: rt9467: Fix passing zero to 'dev_err_probe' 2023-03-23 c4351b646123 iio: adc: ti-ads1100: fix error code in probe() 2023-02-27 65609d3206f7 i2c: gxp: fix an error code in probe 2023-02-15 76f5aaabce49 ASoC: soc-ac97: Return correct error codes regards, dan carpenter
On Sat, Aug 31, 2024 at 11:53:51AM +0300, Dan Carpenter wrote: > On Sat, Aug 31, 2024 at 11:25:54AM +0300, Dan Carpenter wrote: > > On Thu, Aug 22, 2024 at 04:05:38PM +0300, Andy Shevchenko wrote: > > > In the similar way, ignore 0 error code (AKA "success") in > > > dev_err_probe(). This helps to simplify a code such as > > > > > > if (ret < 0) > > > return dev_err_probe(int3472->dev, ret, err_msg); > > > > > > return ret; > > > > > > to > > > > > > return dev_err_probe(int3472->dev, ret, err_msg); > > > > > > Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> > > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > > > > This is a terrible idea because currently Smatch is able to detect about one > > bug per month where someone unintentionally passes the wrong error variable > > to dev_err_probe(). How many cases you know where smatch is false positive about this? I believe the number is only a few at most, which means that you may easily detect this still with this change being applied, i.e. "anything that terminates function flow with code 0, passed to dev_err_probe(), is suspicious". So, I don't see how it prevents you from detecting that. > Here are the stats since Jan 2023. All these bugs are impossible to detect now. > > 2024-08-12 d3bde2243d42 iio: proximity: hx9023s: Fix error code in hx9023s_property_get() Just looked at this one. Whoever writes something like if (ret || foo) return ...ret...; should be punished immediately. I.o.w. has nothing to do with the API. > 2024-07-08 101e5c5c4e76 PCI: qcom: Fix missing error code in qcom_pcie_probe() > 2024-02-22 debabbb1f272 iio: adc: ti-ads1298: Fix error code in probe() > 2024-01-08 9c46e3a5232d iio: adc: ad7091r8: Fix error code in ad7091r8_gpio_setup() > 2023-12-04 35ddd61cf023 platform/x86: x86-android-tablets: Fix an IS_ERR() vs NULL check in probe > 2023-11-20 2d37b3649c41 hwrng: starfive - Fix dev_err_probe return error > 2023-11-30 03219a3aa6c8 drm/imagination: Fix error codes in pvr_device_clk_init() > 2023-09-07 4b2b39f9395b watchdog: marvell_gti_wdt: Fix error code in probe() > 2023-08-24 8886e1b03669 ASoC: codecs: Fix error code in aw88261_i2c_probe() > 2023-06-23 ad5152b85e8b leds: aw200xx: Fix error code in probe() > 2023-07-18 86fe3e9f4c63 phy: starfive: fix error code in probe > 2023-07-12 0b64150c3429 Input: bcm-keypad - correct dev_err_probe() error > 2023-06-26 5fb2864cbd50 OPP: Properly propagate error along when failing to get icc_path > 2023-06-19 02474880e8fd ASoC: max98388: fix error code in probe() > 2023-05-25 cc5f2eb7ce11 mfd: tps6594: Fix an error code in probe() > 2023-05-22 1ca04f21b204 remoteproc: stm32: Fix error code in stm32_rproc_parse_dt() > 2023-05-15 46f5dd7439e3 fbdev: omapfb: panel-tpo-td043mtea1: fix error code in probe() > 2023-05-13 3530167c6fe8 soc: qcom: icc-bwmon: fix incorrect error code passed to dev_err_probe() > 2023-05-12 bc97139ff135 power: supply: rt9467: Fix passing zero to 'dev_err_probe' > 2023-03-23 c4351b646123 iio: adc: ti-ads1100: fix error code in probe() > 2023-02-27 65609d3206f7 i2c: gxp: fix an error code in probe > 2023-02-15 76f5aaabce49 ASoC: soc-ac97: Return correct error codes
On Mon, Sep 02, 2024 at 01:16:17PM +0300, Andy Shevchenko wrote: > On Sat, Aug 31, 2024 at 11:53:51AM +0300, Dan Carpenter wrote: > > On Sat, Aug 31, 2024 at 11:25:54AM +0300, Dan Carpenter wrote: > > > On Thu, Aug 22, 2024 at 04:05:38PM +0300, Andy Shevchenko wrote: > > > > In the similar way, ignore 0 error code (AKA "success") in > > > > dev_err_probe(). This helps to simplify a code such as > > > > > > > > if (ret < 0) > > > > return dev_err_probe(int3472->dev, ret, err_msg); > > > > > > > > return ret; > > > > > > > > to > > > > > > > > return dev_err_probe(int3472->dev, ret, err_msg); > > > > > > > > Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> > > > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > > > > > > This is a terrible idea because currently Smatch is able to detect about one > > > bug per month where someone unintentionally passes the wrong error variable > > > to dev_err_probe(). > > How many cases you know where smatch is false positive about this? > This check has a very low false positive rate. There is only one potential false positive in the current linux-next. Let me add Baolin Wang to get his take on this. I never mentioned reported this warning because the code was old when I wrote the check. drivers/spi/spi-sprd-adi.c 550 ret = of_hwspin_lock_get_id(np, 0); 551 if (ret > 0 || (IS_ENABLED(CONFIG_HWSPINLOCK) && ret == 0)) { Is it possible for the CONFIG_ to not be enabled but ret is zero? 552 sadi->hwlock = 553 devm_hwspin_lock_request_specific(&pdev->dev, ret); 554 if (!sadi->hwlock) { 555 ret = -ENXIO; 556 goto put_ctlr; 557 } 558 } else { 559 switch (ret) { 560 case -ENOENT: 561 dev_info(&pdev->dev, "no hardware spinlock supplied\n"); 562 break; 563 default: 564 dev_err_probe(&pdev->dev, ret, "failed to find hwlock id\n"); ^^^ 565 goto put_ctlr; 566 } 567 } > I believe the number is only a few at most, which means that you may easily > detect this still with this change being applied, i.e. "anything that > terminates function flow with code 0, passed to dev_err_probe(), is > suspicious". > I think you mean the opposite of what you wrote? That if we're passing zero to dev_err_probe() and it's the last line in a function it's *NOT* suspicious? Otherwise, I don't really understand the heuristic you're proposing. regards, dan carpenter
On Mon, Sep 02, 2024 at 02:10:41PM +0300, Dan Carpenter wrote: > On Mon, Sep 02, 2024 at 01:16:17PM +0300, Andy Shevchenko wrote: > > On Sat, Aug 31, 2024 at 11:53:51AM +0300, Dan Carpenter wrote: > > > On Sat, Aug 31, 2024 at 11:25:54AM +0300, Dan Carpenter wrote: > > > > On Thu, Aug 22, 2024 at 04:05:38PM +0300, Andy Shevchenko wrote: > > > > > In the similar way, ignore 0 error code (AKA "success") in > > > > > dev_err_probe(). This helps to simplify a code such as > > > > > > > > > > if (ret < 0) > > > > > return dev_err_probe(int3472->dev, ret, err_msg); > > > > > > > > > > return ret; > > > > > > > > > > to > > > > > > > > > > return dev_err_probe(int3472->dev, ret, err_msg); > > > > > > > > > > Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> > > > > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > > > > > > > > This is a terrible idea because currently Smatch is able to detect about one > > > > bug per month where someone unintentionally passes the wrong error variable > > > > to dev_err_probe(). > > > > How many cases you know where smatch is false positive about this? > > This check has a very low false positive rate. Yes, that's my expectation as well. > There is only one potential > false positive in the current linux-next. Let me add Baolin Wang to get his > take on this. I never mentioned reported this warning because the code was old > when I wrote the check. > > drivers/spi/spi-sprd-adi.c > 550 ret = of_hwspin_lock_get_id(np, 0); > 551 if (ret > 0 || (IS_ENABLED(CONFIG_HWSPINLOCK) && ret == 0)) { > > Is it possible for the CONFIG_ to not be enabled but ret is zero? > > 552 sadi->hwlock = > 553 devm_hwspin_lock_request_specific(&pdev->dev, ret); > 554 if (!sadi->hwlock) { > 555 ret = -ENXIO; > 556 goto put_ctlr; > 557 } > 558 } else { > 559 switch (ret) { > 560 case -ENOENT: > 561 dev_info(&pdev->dev, "no hardware spinlock supplied\n"); > 562 break; > 563 default: > 564 dev_err_probe(&pdev->dev, ret, "failed to find hwlock id\n"); > ^^^ > > 565 goto put_ctlr; > 566 } > 567 } > > > > I believe the number is only a few at most, which means that you may easily > > detect this still with this change being applied, i.e. "anything that > > terminates function flow with code 0, passed to dev_err_probe(), is > > suspicious". > > I think you mean the opposite of what you wrote? That if we're passing zero to > dev_err_probe() and it's the last line in a function it's *NOT* suspicious? Yes, sorry, I meant "...terminates function flow _in the middle_...". > Otherwise, I don't really understand the heuristic you're proposing.
On Mon, Sep 02, 2024 at 02:38:18PM +0300, Andy Shevchenko wrote: > > > I believe the number is only a few at most, which means that you may easily > > > detect this still with this change being applied, i.e. "anything that > > > terminates function flow with code 0, passed to dev_err_probe(), is > > > suspicious". > > > > I think you mean the opposite of what you wrote? That if we're passing zero to > > dev_err_probe() and it's the last line in a function it's *NOT* suspicious? > > Yes, sorry, I meant "...terminates function flow _in the middle_...". > I don't think that works. There are lots of success paths in the middle of functions. Smatch already has code to determine whether we should return an error code or not. 1) Was there a function that returned NULL 2) Was there a function that returned an erorr code/error pointer 3) Was there a bounds check where x >= y? 4) Did we print an error code? Etc.. I'd end up re-using this code. This heuristic is more error prone, so there would be false positives and missed bugs but I can't predict the future so I don't know how bad it would be. Looking through the warnings, we still would be able to detected a number of these because Smatch warnings when you pass NULL to IS_ERR() or PTR_ERR(). Probably the worse thing from a Smatch perspective is that now I can't just assume that dev_err_probe() is always an error path. So for example, we know that *foo is always initialized on success so we can eliminate all the "return dev_err_probe();" paths because those are failure path. I'm never going to like this patch because I always want to make the error paths more separate and more clear. regards, dan carpenter
On Sat, Aug 31, 2024 at 11:25:54AM +0300, Dan Carpenter wrote: > On Thu, Aug 22, 2024 at 04:05:38PM +0300, Andy Shevchenko wrote: > > In the similar way, ignore 0 error code (AKA "success") in > > dev_err_probe(). This helps to simplify a code such as > > > > if (ret < 0) > > return dev_err_probe(int3472->dev, ret, err_msg); > > > > return ret; > > > > to > > > > return dev_err_probe(int3472->dev, ret, err_msg); > > > > Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > > This is a terrible idea because currently Smatch is able to detect about one > bug per month where someone unintentionally passes the wrong error variable > to dev_err_probe(). > > I really hate this. > > NAKed-by: Dan Carpenter <dan.carpenter@linaro.org> Regardless of any issues this may cause for tooling, I fully agree that this is a terrible idea that will only result in unreadable code. return dev_err_probe(dev, ret, "registration failed\n"); Except it did not fail... NAK Johan
On Mon, Sep 02, 2024 at 05:58:18PM +0200, Johan Hovold wrote: > On Sat, Aug 31, 2024 at 11:25:54AM +0300, Dan Carpenter wrote: > > On Thu, Aug 22, 2024 at 04:05:38PM +0300, Andy Shevchenko wrote: > > > In the similar way, ignore 0 error code (AKA "success") in > > > dev_err_probe(). This helps to simplify a code such as > > > > > > if (ret < 0) > > > return dev_err_probe(int3472->dev, ret, err_msg); > > > > > > return ret; > > > > > > to > > > > > > return dev_err_probe(int3472->dev, ret, err_msg); > > > > > > Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> > > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > > > > This is a terrible idea because currently Smatch is able to detect about one > > bug per month where someone unintentionally passes the wrong error variable > > to dev_err_probe(). > > > > I really hate this. > > > > NAKed-by: Dan Carpenter <dan.carpenter@linaro.org> > > Regardless of any issues this may cause for tooling, I fully agree that > this is a terrible idea that will only result in unreadable code. > > return dev_err_probe(dev, ret, "registration failed\n"); > > Except it did not fail... > > NAK Fair enough. Thank you, guys, for reviewing.
diff --git a/drivers/base/core.c b/drivers/base/core.c index 4bc8b88d697e..830a14084bf6 100644 --- a/drivers/base/core.c +++ b/drivers/base/core.c @@ -4987,13 +4987,14 @@ define_dev_printk_level(_dev_info, KERN_INFO); * This helper implements common pattern present in probe functions for error * checking: print debug or error message depending if the error value is * -EPROBE_DEFER and propagate error upwards. + * * In case of -EPROBE_DEFER it sets also defer probe reason, which can be * checked later by reading devices_deferred debugfs attribute. * It replaces code sequence:: * * if (err != -EPROBE_DEFER) * dev_err(dev, ...); - * else + * else if (err) * dev_dbg(dev, ...); * return err; * @@ -5003,12 +5004,16 @@ define_dev_printk_level(_dev_info, KERN_INFO); * * Using this helper in your probe function is totally fine even if @err is * known to never be -EPROBE_DEFER. + * + * NOTE: The message is not going to be printed or saved in cases when @err + * is equal to -ENOMEM or 0. + * * The benefit compared to a normal dev_err() is the standardized format * of the error code, it being emitted symbolically (i.e. you get "EAGAIN" * instead of "-35") and the fact that the error code is returned which allows * more compact error paths. * - * Returns @err. + * Return: the value of @err. */ int dev_err_probe(const struct device *dev, int err, const char *fmt, ...) { @@ -5032,6 +5037,10 @@ int dev_err_probe(const struct device *dev, int err, const char *fmt, ...) */ break; + case 0: + /* Success, no need to issue an error message */ + break; + default: dev_err(dev, "error %pe: %pV", ERR_PTR(err), &vaf); break;
In the similar way, ignore 0 error code (AKA "success") in dev_err_probe(). This helps to simplify a code such as if (ret < 0) return dev_err_probe(int3472->dev, ret, err_msg); return ret; to return dev_err_probe(int3472->dev, ret, err_msg); Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> --- drivers/base/core.c | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-)