[v3,2/2] mtd: nand: raw: brcmnand: When oops in progress usepio and interrupt polling
diff mbox series

Message ID 1558024913-26502-2-git-send-email-kdasu.kdev@gmail.com
State Changes Requested
Delegated to: Miquel Raynal
Headers show
Series
  • [v3,1/2] mtd: Add flag to indicate panic_write
Related show

Commit Message

Kamal Dasu May 16, 2019, 4:41 p.m. UTC
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(-)

Comments

Richard Weinberger May 17, 2019, 8:12 a.m. UTC | #1
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.
Kamal Dasu May 17, 2019, 11:56 a.m. UTC | #2
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
Kamal Dasu June 11, 2019, 8:03 p.m. UTC | #3
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
Richard Weinberger June 11, 2019, 8:16 p.m. UTC | #4
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 :-(
Miquel Raynal June 27, 2019, 2:54 p.m. UTC | #5
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

Patch
diff mbox series

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);