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

[Evgeny Novikov]

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