[PATCH v3 1/2] mtd: Add flag to indicate panic_write

[PATCH v3 1/2] mtd: Add flag to indicate panic_write

[Kamal Dasu]

Added a flag to indicate a panic_write so that low level drivers can
use it to take required action where applicable, to ensure oops data
gets written to assigned mtd device.

Signed-off-by: Kamal Dasu <kdasu.kdev@gmail.com>
---
 drivers/mtd/mtdcore.c   | 3 +++
 include/linux/mtd/mtd.h | 6 ++++++
 2 files changed, 9 insertions(+)

diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c
index 76b4264..a83decd 100644
--- a/drivers/mtd/mtdcore.c
+++ b/drivers/mtd/mtdcore.c
@@ -1138,6 +1138,9 @@ int mtd_panic_write(struct mtd_info *mtd, loff_t to, size_t len, size_t *retlen,
 		return -EROFS;
 	if (!len)
 		return 0;
+	if (!mtd->oops_panic_write)
+		mtd->oops_panic_write = true;
+
 	return mtd->_panic_write(mtd, to, len, retlen, buf);
 }
 EXPORT_SYMBOL_GPL(mtd_panic_write);
diff --git a/include/linux/mtd/mtd.h b/include/linux/mtd/mtd.h
index 677768b..791c34d 100644
--- a/include/linux/mtd/mtd.h
+++ b/include/linux/mtd/mtd.h
@@ -330,6 +330,12 @@ struct mtd_info {
 	int (*_get_device) (struct mtd_info *mtd);
 	void (*_put_device) (struct mtd_info *mtd);
 
+	/*
+	 * flag indicates a panic write, low level drivers can take appropriate
+	 * action if required to ensure writes go through
+	 */
+	bool oops_panic_write;
+
 	struct notifier_block reboot_notifier;  /* default mode before reboot */
 
 	/* ECC status information */
-- 
1.9.0.138.g2de3478

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

[Kamal Dasu]

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.

Signed-off-by: Kamal Dasu <kdasu.kdev@gmail.com>
---
 drivers/mtd/nand/raw/brcmnand/brcmnand.c | 48 ++++++++++++++++++++++++++++++--
 1 file changed, 45 insertions(+), 3 deletions(-)

diff --git a/drivers/mtd/nand/raw/brcmnand/brcmnand.c b/drivers/mtd/nand/raw/brcmnand/brcmnand.c
index ce0b8ff..dca8eb8 100644
--- a/drivers/mtd/nand/raw/brcmnand/brcmnand.c
+++ b/drivers/mtd/nand/raw/brcmnand/brcmnand.c
@@ -159,6 +159,7 @@ struct brcmnand_controller {
 	u32			nand_cs_nand_xor;
 	u32			corr_stat_threshold;
 	u32			flash_dma_mode;
+	bool			pio_poll_mode;
 };
 
 struct brcmnand_cfg {
@@ -823,6 +824,20 @@ static inline bool has_flash_dma(struct brcmnand_controller *ctrl)
 	return ctrl->flash_dma_base;
 }
 
+static inline void disable_ctrl_irqs(struct brcmnand_controller *ctrl)
+{
+	if (ctrl->pio_poll_mode)
+		return;
+
+	if (has_flash_dma(ctrl)) {
+		ctrl->flash_dma_base = 0;
+		disable_irq(ctrl->dma_irq);
+	}
+
+	disable_irq(ctrl->irq);
+	ctrl->pio_poll_mode = true;
+}
+
 static inline bool flash_dma_buf_ok(const void *buf)
 {
 	return buf && !is_vmalloc_addr(buf) &&
@@ -1237,15 +1252,42 @@ static void brcmnand_cmd_ctrl(struct nand_chip *chip, int dat,
 	/* intentionally left blank */
 }
 
+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;
+	struct mtd_info *mtd = nand_to_mtd(chip);
+	bool err = false;
+	int sts;
+
+	if (mtd->oops_panic_write) {
+		/* switch to interrupt polling and PIO mode */
+		disable_ctrl_irqs(ctrl);
+		sts = bcmnand_ctrl_poll_status(ctrl, NAND_CTRL_RDY,
+					       NAND_CTRL_RDY, 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) {
+	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);
 
-- 
1.9.0.138.g2de3478

Re: [PATCH v3 1/2] mtd: Add flag to indicate panic_write

[Richard Weinberger]

On Thu, May 16, 2019 at 6:42 PM Kamal Dasu <kdasu.kdev@gmail.com> wrote:
>
> Added a flag to indicate a panic_write so that low level drivers can
> use it to take required action where applicable, to ensure oops data
> gets written to assigned mtd device.
>
> Signed-off-by: Kamal Dasu <kdasu.kdev@gmail.com>
> ---
>  drivers/mtd/mtdcore.c   | 3 +++
>  include/linux/mtd/mtd.h | 6 ++++++
>  2 files changed, 9 insertions(+)
>
> diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c
> index 76b4264..a83decd 100644
> --- a/drivers/mtd/mtdcore.c
> +++ b/drivers/mtd/mtdcore.c
> @@ -1138,6 +1138,9 @@ int mtd_panic_write(struct mtd_info *mtd, loff_t to, size_t len, size_t *retlen,
>                 return -EROFS;
>         if (!len)
>                 return 0;
> +       if (!mtd->oops_panic_write)
> +               mtd->oops_panic_write = true;
> +

You can set the flag unconditionally.
If it is set, it will stay so, and setting it again, won't hurt.

>         return mtd->_panic_write(mtd, to, len, retlen, buf);
>  }
>  EXPORT_SYMBOL_GPL(mtd_panic_write);
> diff --git a/include/linux/mtd/mtd.h b/include/linux/mtd/mtd.h
> index 677768b..791c34d 100644
> --- a/include/linux/mtd/mtd.h
> +++ b/include/linux/mtd/mtd.h
> @@ -330,6 +330,12 @@ struct mtd_info {
>         int (*_get_device) (struct mtd_info *mtd);
>         void (*_put_device) (struct mtd_info *mtd);
>
> +       /*
> +        * flag indicates a panic write, low level drivers can take appropriate
> +        * action if required to ensure writes go through
> +        */
> +       bool oops_panic_write;
> +

Maybe we find a better name for it.
panic_write_triggered?

-- 
Thanks,
//richard

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

[Richard Weinberger]

On Thu, May 16, 2019 at 6:42 PM Kamal Dasu <kdasu.kdev@gmail.com> wrote:
>
> 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.

With the new flag the semantics change, as soon a panic write happened,
the flag will stay and *all* future operates will take the polling/pio path.

IMHO this is fine since the kernel cannot recover from an oops.
But just to make sure we all get this. :-)
An alternative would be to block all further non-panic writes.

-- 
Thanks,
//richard

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

[Kamal Dasu]

On Fri, May 17, 2019 at 4:12 AM Richard Weinberger
<richard.weinberger@gmail.com> wrote:
>
> On Thu, May 16, 2019 at 6:42 PM Kamal Dasu <kdasu.kdev@gmail.com> wrote:
> >
> > 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.
>
> With the new flag the semantics change, as soon a panic write happened,
> the flag will stay and *all* future operates will take the polling/pio path.
>

Yes that is true.

> IMHO this is fine since the kernel cannot recover from an oops.
> But just to make sure we all get this. :-)
> An alternative would be to block all further non-panic writes.

Capturing the panic writes into an mtd device reliably is what the low
level driver is trying to do.If there are non panic writes they will
use pio and interrupt  polling  as well in this case.

> --
> Thanks,
> //richard

Thanks
Kamal

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

[Kamal Dasu]

Richard,

You have any other review comments/concerns with this patch, if not
can you please sign off on it.

Thanks
Kamal

On Fri, May 17, 2019 at 7:56 AM Kamal Dasu <kdasu.kdev@gmail.com> wrote:
>
> On Fri, May 17, 2019 at 4:12 AM Richard Weinberger
> <richard.weinberger@gmail.com> wrote:
> >
> > On Thu, May 16, 2019 at 6:42 PM Kamal Dasu <kdasu.kdev@gmail.com> wrote:
> > >
> > > 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.
> >
> > With the new flag the semantics change, as soon a panic write happened,
> > the flag will stay and *all* future operates will take the polling/pio path.
> >
>
> Yes that is true.
>
> > IMHO this is fine since the kernel cannot recover from an oops.
> > But just to make sure we all get this. :-)
> > An alternative would be to block all further non-panic writes.
>
> Capturing the panic writes into an mtd device reliably is what the low
> level driver is trying to do.If there are non panic writes they will
> use pio and interrupt  polling  as well in this case.
>
> > --
> > Thanks,
> > //richard
>
> Thanks
> Kamal

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

[Richard Weinberger]

On Tue, Jun 11, 2019 at 10:03 PM Kamal Dasu <kdasu.kdev@gmail.com> wrote:
>
> Richard,
>
> You have any other review comments/concerns with this patch, if not
> can you please sign off on it.

I'm fine with that approach.
I hoped to get some input from other MTD folks too :-(

-- 
Thanks,
//richard

Re: [PATCH v3 1/2] mtd: Add flag to indicate panic_write

[Richard Weinberger]

On Fri, May 17, 2019 at 10:08 AM Richard Weinberger
<richard.weinberger@gmail.com> wrote:
>
> On Thu, May 16, 2019 at 6:42 PM Kamal Dasu <kdasu.kdev@gmail.com> wrote:
> >
> > Added a flag to indicate a panic_write so that low level drivers can
> > use it to take required action where applicable, to ensure oops data
> > gets written to assigned mtd device.
> >
> > Signed-off-by: Kamal Dasu <kdasu.kdev@gmail.com>
> > ---
> >  drivers/mtd/mtdcore.c   | 3 +++
> >  include/linux/mtd/mtd.h | 6 ++++++
> >  2 files changed, 9 insertions(+)
> >
> > diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c
> > index 76b4264..a83decd 100644
> > --- a/drivers/mtd/mtdcore.c
> > +++ b/drivers/mtd/mtdcore.c
> > @@ -1138,6 +1138,9 @@ int mtd_panic_write(struct mtd_info *mtd, loff_t to, size_t len, size_t *retlen,
> >                 return -EROFS;
> >         if (!len)
> >                 return 0;
> > +       if (!mtd->oops_panic_write)
> > +               mtd->oops_panic_write = true;
> > +
>
> You can set the flag unconditionally.
> If it is set, it will stay so, and setting it again, won't hurt.
>
> >         return mtd->_panic_write(mtd, to, len, retlen, buf);
> >  }
> >  EXPORT_SYMBOL_GPL(mtd_panic_write);
> > diff --git a/include/linux/mtd/mtd.h b/include/linux/mtd/mtd.h
> > index 677768b..791c34d 100644
> > --- a/include/linux/mtd/mtd.h
> > +++ b/include/linux/mtd/mtd.h
> > @@ -330,6 +330,12 @@ struct mtd_info {
> >         int (*_get_device) (struct mtd_info *mtd);
> >         void (*_put_device) (struct mtd_info *mtd);
> >
> > +       /*
> > +        * flag indicates a panic write, low level drivers can take appropriate
> > +        * action if required to ensure writes go through
> > +        */
> > +       bool oops_panic_write;
> > +
>
> Maybe we find a better name for it.
> panic_write_triggered?

ping?
I'm happy with the overall approach.
So let's target the upcoming merge window.
Can you please sort my two comments out? :-)

-- 
Thanks,
//richard

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

[Miquel Raynal]

Hi Richard,

Richard Weinberger <richard.weinberger@gmail.com> wrote on Tue, 11 Jun
2019 22:16:36 +0200:

> On Tue, Jun 11, 2019 at 10:03 PM Kamal Dasu <kdasu.kdev@gmail.com> wrote:
> >
> > Richard,
> >
> > You have any other review comments/concerns with this patch, if not
> > can you please sign off on it.  
> 
> I'm fine with that approach.
> I hoped to get some input from other MTD folks too :-(
> 

Sorry for my late answer but yes, I am totally fine with this approach.
I'll carry this through the nand branch.


Thanks,
Miquèl

Re: [PATCH v3 1/2] mtd: Add flag to indicate panic_write

[Miquel Raynal]

Hi Kamal,

Kamal Dasu <kdasu.kdev@gmail.com> wrote on Thu, 16 May 2019 12:41:46
-0400:

> Added a flag to indicate a panic_write so that low level drivers can
> use it to take required action where applicable, to ensure oops data
> gets written to assigned mtd device.
> 
> Signed-off-by: Kamal Dasu <kdasu.kdev@gmail.com>
> ---

Applied to nand/next, thanks.

Miquèl