Hi Dan,
On 27.08.2021 14:51, Dan Carpenter wrote:
On Thu, Aug 26, 2021 at 07:26:22PM +0300, Evgeny Novikov wrote:
I added Dan to the discussion since he is a developer of one of such
tools.
Thanks for that... :P
I never warn about "forgot to check the return" bugs except in the case
of error pointers or allocation failures. There are too many false
positives. If people want to do that they should add a __must_check
attribute to the function.
Maybe you will be able to convince the developers of the clock framework
to add this attribute to some of their functions. For instance, this is
already the case, say, for clk_bulk_prepare() and clk_bulk_enable() that
seem to represent a number of clk_prepare() and clk_enable(). For those
functions the __must_check attribute was added in commit 6e0d4ff4580c
without providing any specific reasons why it is necessary for them and
is not necessary for usual clk_prepare() and clk_enable().
You linked to another thread: https://lkml.org/lkml/2021/8/17/239
That patch isn't correct. Miquel was on the right track but not 100%.
The nand_scan() calls mxic_nfc_clk_enable() so we should disable it
until it has been successfully enabled. The current code can trigger a
WARN() in __clk_disable(). In other words it should have been:
diff --git a/drivers/mtd/nand/raw/mxic_nand.c b/drivers/mtd/nand/raw/mxic_nand.c
index da1070993994..87aef98f5b9d 100644
--- a/drivers/mtd/nand/raw/mxic_nand.c
+++ b/drivers/mtd/nand/raw/mxic_nand.c
@@ -535,11 +535,11 @@ static int mxic_nfc_probe(struct platform_device *pdev)
err = devm_request_irq(&pdev->dev, irq, mxic_nfc_isr,
0, "mxic-nfc", nfc);
if (err)
- goto fail;
+ return err;
err = nand_scan(nand_chip, 1);
if (err)
- goto fail;
+ return err;
err = mtd_device_register(mtd, NULL, 0);
if (err)
Thank you for this explanation. Now I understand better the Miquel's
comment. Nevertheless, I still have doubts that your fix is completely
correct since mxic_nfc_set_freq() invokes mxic_nfc_clk_disable() first
that still should raise a warning. It seems that the driver developers
are looking on this issue, so, let's wait a bug fix from them. At least
they will be able to test that everything is okay after all.
nand_scan() error handling is still leaky, but it's hard to fix without
re-working the API.
Anyway, thanks for the fixes. I've been inspired by the Linux Driver
Verification project work.
It would be great to collaborate with each other. For instance, for the
aforementioned clock API your tool can perform better checking and find
more potential bugs in some (maybe even all) cases due to a number of
reasons. Unless it will be possible to detect all target issues
automatically with static analysis tools, we can try to reveal some of
the remaining ones with our heavyweight approach.
Best regards,
Evgeny Novikov
regards,
dan carpenter
