Message-ID: <20210817125949.5bde9715@xps13>
Date:   Tue, 17 Aug 2021 12:59:49 +0200
From:   Miquel Raynal <miquel.raynal@...tlin.com>
To:     Evgeny Novikov <novikov@...ras.ru>
Cc:     Andy Shevchenko <andy.shevchenko@...il.com>,
        Richard Weinberger <richard@....at>,
        Vignesh Raghavendra <vigneshr@...com>,
        Kirill Shilimanov <kirill.shilimanov@...wei.com>,
        "linux-mtd@...ts.infradead.org" <linux-mtd@...ts.infradead.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "ldv-project@...uxtesting.org" <ldv-project@...uxtesting.org>,
        ycllin@...c.com.tw
Subject: 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

Powered by blists - more mailing lists