Re: [PATCH] mtd: nand: raw: brcmnand: When oops in progress use pio and interrupt polling

[Kamal Dasu <kdasu.kdev@xxxxxxxxx>]

On Thu, May 2, 2019 at 4:25 AM Miquel Raynal <miquel.raynal@xxxxxxxxxxx> wrote:
>
> Hi Kamal,
>
> Kamal Dasu <kdasu.kdev@xxxxxxxxx> wrote on Wed,  1 May 2019 13:46:15
> -0400:
>
> > If mtd_oops is in progress switch to polling for nand command completion
>
> 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 online
> > 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@xxxxxxxxx>
> > ---
> >  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/nand/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_controller *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 = 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 = 0;
> > +
> > +#ifdef CONFIG_MTD_OOPS
> > +     if (oops_in_progress && smp_processor_id()) {
> > +             int cpu = 0;
> > +
> > +             for_each_online_cpu(cpu)
> > +                     ++i;
> > +     }
> > +#endif
> > +     return i == 1 ? true : false;
>
> I suppose return (i == 1); is enough
>

Ok will make the change.

> > +}
> > +
> > +static bool brcmstb_nand_wait_for_completion(struct nand_chip *chip)
> > +{
> > +     struct brcmnand_host *host = nand_get_controller_data(chip);
> > +     struct brcmnand_controller *ctrl = host->ctrl;
> > +     bool err = false;
> > +     int sts;
> > +
> > +     if (is_mtd_oops_in_progress()) {
> > +             /* Switch to interrupt polling and PIO mode */
> > +             disable_flash_dma_xfer(ctrl);
> > +             sts = bcmnand_ctrl_poll_status(ctrl, NAND_CTRL_RDY |
> > +                                            NAND_STATUS_READY,
> > +                                            NAND_CTRL_RDY |
> > +                                            NAND_STATUS_READY, 0);
> > +             err = (sts < 0) ? true : false;
> > +     } else {
> > +             unsigned long timeo = msecs_to_jiffies(
> > +                                             NAND_POLL_STATUS_TIMEOUT_MS);
> > +             /* wait for completion interrupt */
> > +             sts = wait_for_completion_timeout(&ctrl->done, timeo);
> > +             err = (sts <= 0) ? true : false;
> > +     }
> > +
> > +     return err;
> > +}
> > +
> >  static int brcmnand_waitfunc(struct nand_chip *chip)
> >  {
> >       struct brcmnand_host *host = nand_get_controller_data(chip);
> >       struct brcmnand_controller *ctrl = host->ctrl;
> > -     unsigned long timeo = msecs_to_jiffies(100);
> > +     bool err = 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) <= 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 = brcmstb_nand_wait_for_completion(chip);
> > +
> > +     if (err) {
> >               u32 cmd = brcmnand_read_reg(ctrl, BRCMNAND_CMD_START)
> >                                       >> brcmnand_cmd_shift(ctrl);
> >
>
>
> Thanks,
> Miquèl

Thanks
Kamal

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/