| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <OF1993890F.61B3A484-ON4825873D.001BF93D-4825873D.001C5C8F@mxic.com.tw>
Date: Thu, 26 Aug 2021 13:09:47 +0800
From: ycllin@...c.com.tw
To: "Miquel Raynal" <miquel.raynal@...tlin.com>, jaimeliao@...c.com.tw
Cc: "Andy Shevchenko" <andy.shevchenko@...il.com>,
"Kirill Shilimanov" <kirill.shilimanov@...wei.com>,
"ldv-project@...uxtesting.org" <ldv-project@...uxtesting.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"linux-mtd@...ts.infradead.org" <linux-mtd@...ts.infradead.org>,
"Evgeny Novikov" <novikov@...ras.ru>,
"Richard Weinberger" <richard@....at>,
"Vignesh Raghavendra" <vigneshr@...com>
Subject: Re: [PATCH] mtd: rawnand: mxic: Enable and prepare clocks in probe
Hi Miquel,
+ Jaime from Macronix.
Jaime will check and confirm this issue.
> Re: [PATCH] mtd: rawnand: mxic: Enable and prepare clocks in probe
>
>
> Miquel Raynal <miquel.raynal@...tlin.com> wrote on Tue, 17 Aug 2021
> 12:55:21 +0200:
>
> > Hi Evgeny,
> >
> > +Mason from Macronix
>
> Mason's e-mail bounced, including YouChing in the discussion who may
> also have an answer or perhaps knows who to include in the discussion
> (see below for the context and question).
>
> >
> > Evgeny Novikov <novikov@...ras.ru> wrote on Tue, 17 Aug 2021 13:36:03
> > +0300:
> >
> > > Hi Miquel,
> > >
> > > On 16.08.2021 11:01, Miquel Raynal wrote:
> > > > Hi Andy,
> > > >
> > > > Andy Shevchenko <andy.shevchenko@...il.com> wrote on Thu, 12 Aug
2021
> > > > 15:13:10 +0300:
> > > >
> > > >> On Thursday, August 12, 2021, Evgeny Novikov <novikov@...ras.ru>
wrote:
> > > >>
> > > >>> It seems that mxic_nfc_probe() missed invocation of
> > > >>> mxic_nfc_clk_enable(). The patch fixed that. In addition, error
handling
> > > >>> was refined appropriately.
> > > >>
> > > >> NAK. Until you provide a deeper analysis, like how this works
before your
> > > >> change.
> > > >>
> > > >>
> > > >> Please, don’t blindly generate patches, this can even your bot
do, just
> > > >> think about each change and preferable test on the real hardware.
> > > >>
> > > >> The above is to all your lovely contributions.
> > > >>
> > > >>
> > > >>> Found by Linux Driver Verification project (linuxtesting.org).
> > > >>>
> > > >>> Signed-off-by: Evgeny Novikov <novikov@...ras.ru>
> > > >>> Co-developed-by: Kirill Shilimanov
<kirill.shilimanov@...wei.com>
> > > >>> Signed-off-by: Kirill Shilimanov <kirill.shilimanov@...wei.com>
> > > >>> ---
> > > >>> drivers/mtd/nand/raw/mxic_nand.c | 16 ++++++++++++----
> > > >>> 1 file changed, 12 insertions(+), 4 deletions(-)
> > > >>>
> > > >>> diff --git a/drivers/mtd/nand/raw/mxic_nand.c
b/drivers/mtd/nand/raw/mxic_
> > > >>> nand.c
> > > >>> index da1070993994..37e75bf60ee5 100644
> > > >>> --- a/drivers/mtd/nand/raw/mxic_nand.c
> > > >>> +++ b/drivers/mtd/nand/raw/mxic_nand.c
> > > >>> @@ -509,9 +509,15 @@ static int mxic_nfc_probe(struct
platform_device
> > > >>> *pdev)
> > > >>> if (IS_ERR(nfc->send_dly_clk))
> > > >>> return PTR_ERR(nfc->send_dly_clk);
> > > >>>
> > > >>> + err = mxic_nfc_clk_enable(nfc);
> > > >>> + if (err)
> > > >>> + return err;
> > > > As Andy said, this is not needed.
> > > >
> > > >>> +
> > > >>> nfc->regs = devm_platform_ioremap_resource(pdev, 0);
> > > >>> - if (IS_ERR(nfc->regs))
> > > >>> - return PTR_ERR(nfc->regs);
> > > >>> + if (IS_ERR(nfc->regs)) {
> > > >>> + err = PTR_ERR(nfc->regs);
> > > >>> + goto fail;
> > > >>> + }
> > > >>>
> > > >>> nand_chip = &nfc->chip;
> > > >>> mtd = nand_to_mtd(nand_chip);
> > > >>> @@ -527,8 +533,10 @@ static int mxic_nfc_probe(struct
platform_device
> > > >>> *pdev)
> > > >>> nand_chip->controller = &nfc->controller;
> > > >>>
> > > >>> irq = platform_get_irq(pdev, 0);
> > > >>> - if (irq < 0)
> > > >>> - return irq;
> > > >>> + if (irq < 0) {
> > > >>> + err = irq;
> > > >>> + goto fail;
> > > > However some reworking is needed in the error path.
> > > >
> > > > That goto statement should be renamed and devm_request_irq()
should not
> > > > jump to it.
> > > >
> > >
> > > We still need some help and clarification from those who are very
familiar
> with this sort of drivers or/and can test this particular driver.
> mxic_nfc_clk_enable() is the complementary function for
mxic_nfc_clk_disable
> (). No other functions invoke
clk_prepare_enable()/clk_disable_unprepare() in
> the driver. Unlikely somebody in its environment does that since driver
> specific clocks are dealt with. At the moment the driver invokes
> mxic_nfc_clk_disable() on error handling paths in probe, in remove and
in
> mxic_nfc_set_freq(). mxic_nfc_clk_enable() is called just by
mxic_nfc_set_freq
> () that moreover does this after calling mxic_nfc_clk_disable() first.
So, we
> did not find any place in the driver that invokes mxic_nfc_clk_enable()
prior
> to mxic_nfc_clk_disable(). Basing on this we added mxic_nfc_clk_enable()
just
> after getting clocks. As I explained in the previous large e-mail, we
may be
> wrong in our understanding of the driver environment or/and at
specification
> of requirements being checked. It would be great if you will point out
on our mistakes.
> >
> > Enabling the clocks seems to only be needed to access the NAND device
> > and not the registers of the controller. Mason, is this statement
> > right? If this statement is wrong, then your proposal is not entirely
> > wrong in the sense that we must enable the missing clock *before*
> > accessing any register.
> >
> > Otherwise for the two other clocks, we don't really care to enable
them
> > before setting the actual frequency in ->setup_interface() (called by
> > nand_scan()) because at this point we don't yet know what timing mode
> > is best. Disabling the clock is not an issue even though it was not
> > enabled in the fist place anyway.
> >
> > In all cases, the error path is wrong.
> >
> > Thanks,
> > Miquèl
CONFIDENTIALITY NOTE:
This e-mail and any attachments may contain confidential information
and/or personal data, which is protected by applicable laws. Please be
reminded that duplication, disclosure, distribution, or use of this e-mail
(and/or its attachments) or any part thereof is prohibited. If you receive
this e-mail in error, please notify us immediately and delete this mail as
well as its attachment(s) from your system. In addition, please be
informed that collection, processing, and/or use of personal data is
prohibited unless expressly permitted by personal data protection laws.
Thank you for your attention and cooperation.
Macronix International Co., Ltd.
=====================================================================
============================================================================
CONFIDENTIALITY NOTE:
This e-mail and any attachments may contain confidential information and/or personal data, which is protected by applicable laws. Please be reminded that duplication, disclosure, distribution, or use of this e-mail (and/or its attachments) or any part thereof is prohibited. If you receive this e-mail in error, please notify us immediately and delete this mail as well as its attachment(s) from your system. In addition, please be informed that collection, processing, and/or use of personal data is prohibited unless expressly permitted by personal data protection laws. Thank you for your attention and cooperation.
Macronix International Co., Ltd.
=====================================================================
Powered by blists - more mailing lists