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@xxxxxxxxxxxxxxx>
> > > > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>
> > > >
> > > > 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.
--
With Best Regards,
Andy Shevchenko
