From: Evgeny Novikov <novikov@ispras.ru>
To: Dan Carpenter <dan.carpenter@oracle.com>
Cc: Alan Stern <stern@rowland.harvard.edu>,
Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
Andrew Lunn <andrew@lunn.ch>,
Mike Turquette <mturquette@linaro.org>,
Kirill Shilimanov <kirill.shilimanov@huawei.com>,
linux-usb@vger.kernel.org, linux-kernel@vger.kernel.org,
ldv-project@linuxtesting.org
Subject: Re: [PATCH] usb: ehci-orion: Handle errors of clk_prepare_enable() in probe
Date: Sat, 28 Aug 2021 13:37:10 +0300 [thread overview]
Message-ID: <2290efca-b6c9-6964-7b5c-7ad5d53d719c@ispras.ru> (raw)
In-Reply-To: <20210827115156.GK7722@kadam>
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
next prev parent reply other threads:[~2021-08-28 10:37 UTC|newest]
Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-08-25 17:09 [PATCH] usb: ehci-orion: Handle errors of clk_prepare_enable() in probe Evgeny Novikov
2021-08-25 17:29 ` Alan Stern
2021-08-26 13:30 ` Evgeny Novikov
2021-08-26 15:24 ` Alan Stern
2021-08-26 16:26 ` Evgeny Novikov
2021-08-27 11:51 ` Dan Carpenter
2021-08-28 10:37 ` Evgeny Novikov [this message]
2021-08-28 10:47 ` Evgeny Novikov
2021-08-28 15:11 ` Alan Stern
2021-08-25 17:33 ` Andrew Lunn
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=2290efca-b6c9-6964-7b5c-7ad5d53d719c@ispras.ru \
--to=novikov@ispras.ru \
--cc=andrew@lunn.ch \
--cc=dan.carpenter@oracle.com \
--cc=gregkh@linuxfoundation.org \
--cc=kirill.shilimanov@huawei.com \
--cc=ldv-project@linuxtesting.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-usb@vger.kernel.org \
--cc=mturquette@linaro.org \
--cc=stern@rowland.harvard.edu \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.