Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
References: <1556733121-20133-1-git-send-email-kdasu.kdev@gmail.com> <20190502102504.32b45247@xps13>
In-Reply-To: <20190502102504.32b45247@xps13>
From:   Kamal Dasu <kdasu.kdev@gmail.com>
Date:   Thu, 2 May 2019 10:09:59 -0400
Message-ID: <CAC=U0a2O8V9O8b-dZhn7DRptP3fg1BBCbxhxKXVcsWHykQovaA@mail.gmail.com>
Subject: Re: [PATCH] mtd: nand: raw: brcmnand: When oops in progress use pio
 and interrupt polling
To:     Miquel Raynal <miquel.raynal@bootlin.com>
Cc:     MTD Maling List <linux-mtd@lists.infradead.org>,
        bcm-kernel-feedback-list@broadcom.com,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        Brian Norris <computersforpeace@gmail.com>,
        Boris Brezillon <bbrezillon@kernel.org>,
        Richard Weinberger <richard@nod.at>,
        David Woodhouse <dwmw2@infradead.org>,
        Marek Vasut <marek.vasut@gmail.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Thu, May 2, 2019 at 4:25 AM Miquel Raynal <miquel.raynal@bootlin.com> wr=
ote:
>
> Hi Kamal,
>
> Kamal Dasu <kdasu.kdev@gmail.com> wrote on Wed,  1 May 2019 13:46:15
> -0400:
>
> > If mtd_oops is in progress switch to polling for nand command completio=
n
>
> s/nand/NAND/

Will change this.

>
> > interrupts and use PIO mode wihtout DMA so that the mtd_oops buffer can
> > be completely written in the assinged nand partition.
>
> What about:
>
> "If mtd_oops is in progress, switch to polling during NAND command
> completion instead of relying on DMA/interrupts so that the mtd_oops
> buffer can be completely written in the assigned NAND partition."
>

Will make this change as well

> > This is needed in
> > cases where the panic does not happen on cpu0 and there is only one onl=
ine
> > CPU and the panic is not on cpu0.
>
> "This is needed in case the panic does not happen on CPU0 and there is
> only one online CPU."
>
> I am not sure to understand the problem or how this can happen (and
> be a problem). Have you met such issue already or is this purely
> speculative?

We have seen this issue and tested it on multi core SoCs.

>
> >
> > Signed-off-by: Kamal Dasu <kdasu.kdev@gmail.com>
> > ---
> >  drivers/mtd/nand/raw/brcmnand/brcmnand.c | 55 ++++++++++++++++++++++++=
++++++--
> >  1 file changed, 52 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/mtd/nand/raw/brcmnand/brcmnand.c b/drivers/mtd/nan=
d/raw/brcmnand/brcmnand.c
> > index 482c6f0..cfbe51a 100644
> > --- a/drivers/mtd/nand/raw/brcmnand/brcmnand.c
> > +++ b/drivers/mtd/nand/raw/brcmnand/brcmnand.c
> > @@ -823,6 +823,12 @@ static inline bool has_flash_dma(struct brcmnand_c=
ontroller *ctrl)
> >       return ctrl->flash_dma_base;
> >  }
> >
> > +static inline void disable_flash_dma_xfer(struct brcmnand_controller *=
ctrl)
> > +{
> > +     if (has_flash_dma(ctrl))
> > +             ctrl->flash_dma_base =3D 0;
> > +}
> > +
> >  static inline bool flash_dma_buf_ok(const void *buf)
> >  {
> >       return buf && !is_vmalloc_addr(buf) &&
> > @@ -1237,15 +1243,58 @@ static void brcmnand_cmd_ctrl(struct nand_chip =
*chip, int dat,
> >       /* intentionally left blank */
> >  }
> >
> > +static bool is_mtd_oops_in_progress(void)
> > +{
> > +     int i =3D 0;
> > +
> > +#ifdef CONFIG_MTD_OOPS
> > +     if (oops_in_progress && smp_processor_id()) {
> > +             int cpu =3D 0;
> > +
> > +             for_each_online_cpu(cpu)
> > +                     ++i;
> > +     }
> > +#endif
> > +     return i =3D=3D 1 ? true : false;
>
> I suppose return (i =3D=3D 1); is enough
>

Ok will make the change.

> > +}
> > +
> > +static bool brcmstb_nand_wait_for_completion(struct nand_chip *chip)
> > +{
> > +     struct brcmnand_host *host =3D nand_get_controller_data(chip);
> > +     struct brcmnand_controller *ctrl =3D host->ctrl;
> > +     bool err =3D false;
> > +     int sts;
> > +
> > +     if (is_mtd_oops_in_progress()) {
> > +             /* Switch to interrupt polling and PIO mode */
> > +             disable_flash_dma_xfer(ctrl);
> > +             sts =3D bcmnand_ctrl_poll_status(ctrl, NAND_CTRL_RDY |
> > +                                            NAND_STATUS_READY,
> > +                                            NAND_CTRL_RDY |
> > +                                            NAND_STATUS_READY, 0);
> > +             err =3D (sts < 0) ? true : false;
> > +     } else {
> > +             unsigned long timeo =3D msecs_to_jiffies(
> > +                                             NAND_POLL_STATUS_TIMEOUT_=
MS);
> > +             /* wait for completion interrupt */
> > +             sts =3D wait_for_completion_timeout(&ctrl->done, timeo);
> > +             err =3D (sts <=3D 0) ? true : false;
> > +     }
> > +
> > +     return err;
> > +}
> > +
> >  static int brcmnand_waitfunc(struct nand_chip *chip)
> >  {
> >       struct brcmnand_host *host =3D nand_get_controller_data(chip);
> >       struct brcmnand_controller *ctrl =3D host->ctrl;
> > -     unsigned long timeo =3D msecs_to_jiffies(100);
> > +     bool err =3D false;
> >
> >       dev_dbg(ctrl->dev, "wait on native cmd %d\n", ctrl->cmd_pending);
> > -     if (ctrl->cmd_pending &&
> > -                     wait_for_completion_timeout(&ctrl->done, timeo) <=
=3D 0) {
>
> What about the wait_for_completion_timeout() call in brcmnand_write()?
>

brcmnand_write() too calls brcmnand_waitfunc(), will poll if it needs
to for completion.

> > +     if (ctrl->cmd_pending)
> > +             err =3D brcmstb_nand_wait_for_completion(chip);
> > +
> > +     if (err) {
> >               u32 cmd =3D brcmnand_read_reg(ctrl, BRCMNAND_CMD_START)
> >                                       >> brcmnand_cmd_shift(ctrl);
> >
>
>
> Thanks,
> Miqu=C3=A8l

Thanks
Kamal