Re: [PATCH] usb: ehci-orion: Handle errors of clk_prepare_enable() in probe

From: Evgeny Novikov
Date: Sat Aug 28 2021 - 06:37:18 EST

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
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:

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
dan carpenter