Skip to contentSkip to conversation

mtd: Add flag to indicate panic_write

PATCH Applied Kamal Dasu 3 participants 10 messages May 16, 2019 – Jul 1, 2019

Subsystems (3)
Memory technology devices (MTD) NAND flash Broadcom stb NAND flash driver

v3 of 4 Applied Latest is v4 →

lore
Share & tools
lore

Kamal Dasu

author
#0 lore raw

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

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 <[email protected]>
---
 drivers/mtd/mtdcore.c   | 3 +++
 include/linux/mtd/mtd.h | 6 ++++++
 2 files changed, 9 insertions(+)
drivers/mtd/mtdcore.c +3 -0
index 76b4264..a83decd 100644
@@ -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);
include/linux/mtd/mtd.h +6 -0
index 677768b..791c34d 100644
@@ -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 */
Signature · 1 line
1.9.0.138.g2de3478
author
#1 lore raw

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

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 <[email protected]>
---
 drivers/mtd/nand/raw/brcmnand/brcmnand.c | 48 ++++++++++++++++++++++++++++++--
 1 file changed, 45 insertions(+), 3 deletions(-)
drivers/mtd/nand/raw/brcmnand/brcmnand.c +45 -3
index ce0b8ff..dca8eb8 100644
@@ -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);
Signature · 1 line
1.9.0.138.g2de3478
#2 lore raw

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

Quoted 4 lines · 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.

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.

Signature · 2 lines
Thanks,
//richard
author
#3 lore raw

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

Quoted 10 lines · Richard Weinberger
Quoted 4 lines · 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.

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.

Quoted 3 lines

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.

Quoted 3 lines

--
Thanks,
//richard

Thanks
Kamal

author
#4 lore raw

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

Richard,

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

Thanks
Kamal

Quoted 30 lines · Kamal Dasu
Quoted 10 lines · Richard Weinberger
Quoted 4 lines · 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.

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.

Quoted 3 lines

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.

Quoted 3 lines

--
Thanks,
//richard

Thanks
Kamal

#5 lore raw

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

Quoted 5 lines · Kamal Dasu

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 :-(

Signature · 2 lines
Thanks,
//richard
maintainer
#6 lore raw

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

Hi Richard,

Richard Weinberger <[email protected]> wrote on Tue, 11 Jun 2019 22:16:36 +0200:

Quoted 10 lines · Kamal Dasu
Quoted 5 lines

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

#7 lore raw
In reply to Kamal Dasu
Quoted 16 lines · 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 <[email protected]>
---
 drivers/mtd/mtdcore.c   | 3 +++
 include/linux/mtd/mtd.h | 6 ++++++
 2 files changed, 9 insertions(+)
drivers/mtd/mtdcore.c +0 -0
index 76b4264..a83decd 100644

drivers/mtd/mtdcore.c int mtd_panic_write(struct mtd_info *mtd, loff_t to, size_t len, size_t *retlen, · line 1143

Quoted 6 lines
drivers/mtd/mtdcore.c +3 -0
@@ -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.

Quoted 8 lines
drivers/mtd/mtdcore.c +0 -0
        return mtd->_panic_write(mtd, to, len, retlen, buf);
 }
 EXPORT_SYMBOL_GPL(mtd_panic_write);
include/linux/mtd/mtd.h +0 -0
index 677768b..791c34d 100644

include/linux/mtd/mtd.h struct mtd_info { · line 338

Quoted 9 lines
include/linux/mtd/mtd.h +6 -0
@@ -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?

Signature · 2 lines
Thanks,
//richard
#8 lore raw
Quoted 39 lines · Richard Weinberger
Quoted 16 lines · 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 <[email protected]>
---
 drivers/mtd/mtdcore.c   | 3 +++
 include/linux/mtd/mtd.h | 6 ++++++
 2 files changed, 9 insertions(+)
drivers/mtd/mtdcore.c +0 -0
index 76b4264..a83decd 100644

drivers/mtd/mtdcore.c int mtd_panic_write(struct mtd_info *mtd, loff_t to, size_t len, size_t *retlen, · line 1143

Quoted 6 lines
drivers/mtd/mtdcore.c +3 -0
@@ -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.

Quoted 11 lines
drivers/mtd/mtdcore.c +0 -0
        return mtd->_panic_write(mtd, to, len, retlen, buf);
 }
 EXPORT_SYMBOL_GPL(mtd_panic_write);
include/linux/mtd/mtd.h +0 -0
index 677768b..791c34d 100644
@@ -330,6 +330,12 @@ struct mtd_info {
        int (*_get_device) (struct mtd_info *mtd);
        void (*_put_device) (struct mtd_info *mtd);
Quoted 9 lines

include/linux/mtd/mtd.h struct mtd_info { · line 338

Quoted 6 lines
include/linux/mtd/mtd.h +6 -0
+       /*
+        * 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? :-)

Signature · 2 lines
Thanks,
//richard