[PATCH v2 0/3] mtd: rawnand: vf610_nfc: fix the OOB of written pages and command timeouts

[PATCH v2 0/3] mtd: rawnand: vf610_nfc: fix the OOB of written pages and command timeouts

[Mehmet Fide]

From: Mehmet Fide <mehmet.fide@screeningeagle.com>

Three fixes for the vf610_nfc driver. All three were pointed out by
Sashiko, on the large-OOB series [1] and on v1 of this one [2].

1/3: the ECC page write transfers the spare area out of the controller
SRAM but nothing ever puts chip->oob_poi there, so every written page
gets whatever the previous transfer left behind as its OOB.

2/3: a command that timed out can still finish later, and then the next
command returns before the controller is done.

3/3: a timeout is only logged, the callers report success.

Tested on a Colibri VF50 (MX30LF1G18AC) with v7.4-rc2, PROVE_LOCKING and
DEBUG_ATOMIC_SLEEP. A debug patch skipped the wait of one command, so
that the command finished after the status flags had been cleared, and
counted the commands that returned while the controller was still busy:

  unpatched:                       250 after 100 forced timeouts
  completion reinitialized (v1):    41 after 100 forced timeouts
  this series:                       0 after 200 forced timeouts

With the series each forced timeout came back as a read error and the
reads after it returned the right data. An 8 MiB UBIFS write read back
identical after dropping the caches, with no ECC corrections or
failures and no lockdep or atomic sleep report.

Changes in v2:
- rebased on v7.4-rc2. The large-OOB fix that patch 1 builds on
  (vf610_nfc_spare_size()) is merged, which was the build failure
  Sashiko reported on v1 1/2
- 1/3: comment dropped, commit message shortened, no code change
- 2/3: rewritten. Sashiko was right that reinitializing the completion
  is not enough, see the numbers above. The idle interrupt is now
  disabled on a timeout and the status flags are cleared before a
  command starts
- 3/3: new

[1] https://lore.kernel.org/linux-mtd/20260818114208.2780311-1-mehmet.fide@gmail.com/
[2] https://lore.kernel.org/linux-mtd/20260828085340.3916239-1-mehmet.fide@gmail.com/

Mehmet Fide (3):
  mtd: rawnand: vf610_nfc: write the OOB bytes on page writes
  mtd: rawnand: vf610_nfc: keep a timed-out command from completing the
    next one
  mtd: rawnand: vf610_nfc: return an error when a command times out

 drivers/mtd/nand/raw/vf610_nfc.c | 41 +++++++++++++++++++++++++++++-----------
 1 file changed, 30 insertions(+), 11 deletions(-)


base-commit: 7475ea8b58616b33c04afb976cf0fbaad9dd63fb
-- 
2.56.0

[PATCH v2 1/3] mtd: rawnand: vf610_nfc: write the OOB bytes on page writes

[Mehmet Fide]

From: Mehmet Fide <mehmet.fide@screeningeagle.com>

The ECC page write transfers the page and the spare area from the
controller SRAM, but only the page data is copied there: the conversion
to ->exec_op() dropped the vf610_nfc_write_buf() call that copied
chip->oob_poi. Every written page gets the leftovers of the previous
transfer as its OOB, and OOB data given by the caller is lost.

Copy chip->oob_poi into the SRAM buffer as well. The NAND core fills it
with 0xff when the caller writes no OOB, and the ECC engine overwrites
the parity region on its way to the flash.

Fixes: 1cbe30b0ddc7 ("mtd: rawnand: vf610_nfc: make use of ->exec_op()")
Cc: stable@vger.kernel.org
Signed-off-by: Mehmet Fide <mehmet.fide@screeningeagle.com>
---
 drivers/mtd/nand/raw/vf610_nfc.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/mtd/nand/raw/vf610_nfc.c b/drivers/mtd/nand/raw/vf610_nfc.c
index 6ab3c440eac7..acc9ff282083 100644
--- a/drivers/mtd/nand/raw/vf610_nfc.c
+++ b/drivers/mtd/nand/raw/vf610_nfc.c
@@ -637,6 +637,8 @@ static int vf610_nfc_write_page(struct nand_chip *chip, const uint8_t *buf,
 	 */
 	vf610_nfc_wr_to_sram(nfc->regs + NFC_MAIN_AREA(0), buf,
 			     mtd->writesize, false);
+	vf610_nfc_wr_to_sram(nfc->regs + NFC_MAIN_AREA(0) + mtd->writesize,
+			     chip->oob_poi, vf610_nfc_spare_size(mtd), false);
 
 	code |= COMMAND_RB_HANDSHAKE;
 	cmd2 |= code << CMD_CODE_SHIFT;

base-commit: 7475ea8b58616b33c04afb976cf0fbaad9dd63fb
-- 
2.56.0

[PATCH v2 2/3] mtd: rawnand: vf610_nfc: keep a timed-out command from completing the next one

[Mehmet Fide]

From: Mehmet Fide <mehmet.fide@screeningeagle.com>

A command that timed out can still finish later. Its interrupt then
completes cmd_done and leaves NFC_ISR[IDLE] set, so the next command
returns before the controller is done: the completion is already done
or, if only the completion is reinitialized, enabling the idle interrupt
raises it again at once.

On a timeout, disable the idle interrupt and wait for a running handler
to return. Before a command is started, clear the status flags and
reinitialize the completion.

Fixes: 456930d80a2d ("mtd: nand: vf610_nfc: Freescale NFC for VF610, MPC5125 and others")
Cc: stable@vger.kernel.org
Signed-off-by: Mehmet Fide <mehmet.fide@screeningeagle.com>
---
Applies as is down to 6.6.y. 6.1.y and older differ in the probe context
around platform_get_irq(), I can send a backport for those.

 drivers/mtd/nand/raw/vf610_nfc.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/drivers/mtd/nand/raw/vf610_nfc.c b/drivers/mtd/nand/raw/vf610_nfc.c
index acc9ff282083..547eb1513893 100644
--- a/drivers/mtd/nand/raw/vf610_nfc.c
+++ b/drivers/mtd/nand/raw/vf610_nfc.c
@@ -154,6 +154,7 @@ struct vf610_nfc {
 	struct device *dev;
 	void __iomem *regs;
 	struct completion cmd_done;
+	int irq;
 	/* Status and ID are in alternate locations. */
 	enum vf610_nfc_variant variant;
 	struct clk *clk;
@@ -291,6 +292,9 @@ static void vf610_nfc_done(struct vf610_nfc *nfc)
 {
 	unsigned long timeout = msecs_to_jiffies(100);
 
+	vf610_nfc_clear_status(nfc);
+	reinit_completion(&nfc->cmd_done);
+
 	/*
 	 * Barrier is needed after this write. This write need
 	 * to be done before reading the next register the first
@@ -301,8 +305,11 @@ static void vf610_nfc_done(struct vf610_nfc *nfc)
 	vf610_nfc_set(nfc, NFC_IRQ_STATUS, IDLE_EN_BIT);
 	vf610_nfc_set(nfc, NFC_FLASH_CMD2, START_BIT);
 
-	if (!wait_for_completion_timeout(&nfc->cmd_done, timeout))
+	if (!wait_for_completion_timeout(&nfc->cmd_done, timeout)) {
 		dev_warn(nfc->dev, "Timeout while waiting for BUSY.\n");
+		vf610_nfc_clear(nfc, NFC_IRQ_STATUS, IDLE_EN_BIT);
+		synchronize_irq(nfc->irq);
+	}
 
 	vf610_nfc_clear_status(nfc);
 }
@@ -885,6 +892,7 @@ static int vf610_nfc_probe(struct platform_device *pdev)
 	irq = platform_get_irq(pdev, 0);
 	if (irq < 0)
 		return irq;
+	nfc->irq = irq;
 
 	nfc->regs = devm_platform_ioremap_resource(pdev, 0);
 	if (IS_ERR(nfc->regs))
-- 
2.56.0

[PATCH v2 3/3] mtd: rawnand: vf610_nfc: return an error when a command times out

[Mehmet Fide]

From: Mehmet Fide <mehmet.fide@screeningeagle.com>

A command that times out only prints a warning. The callers carry on,
copy whatever the SRAM buffer holds and report success.

Return -ETIMEDOUT from vf610_nfc_done() and pass it up through
->exec_op() and the page read and write hooks.

Fixes: 456930d80a2d ("mtd: nand: vf610_nfc: Freescale NFC for VF610, MPC5125 and others")
Signed-off-by: Mehmet Fide <mehmet.fide@screeningeagle.com>
---
 drivers/mtd/nand/raw/vf610_nfc.c | 29 +++++++++++++++++++----------
 1 file changed, 19 insertions(+), 10 deletions(-)

diff --git a/drivers/mtd/nand/raw/vf610_nfc.c b/drivers/mtd/nand/raw/vf610_nfc.c
index 547eb1513893..d6d4a18432ce 100644
--- a/drivers/mtd/nand/raw/vf610_nfc.c
+++ b/drivers/mtd/nand/raw/vf610_nfc.c
@@ -288,9 +288,10 @@ static inline void vf610_nfc_clear_status(struct vf610_nfc *nfc)
 	vf610_nfc_write(nfc, NFC_IRQ_STATUS, tmp);
 }
 
-static void vf610_nfc_done(struct vf610_nfc *nfc)
+static int vf610_nfc_done(struct vf610_nfc *nfc)
 {
 	unsigned long timeout = msecs_to_jiffies(100);
+	int ret = 0;
 
 	vf610_nfc_clear_status(nfc);
 	reinit_completion(&nfc->cmd_done);
@@ -309,9 +310,12 @@ static void vf610_nfc_done(struct vf610_nfc *nfc)
 		dev_warn(nfc->dev, "Timeout while waiting for BUSY.\n");
 		vf610_nfc_clear(nfc, NFC_IRQ_STATUS, IDLE_EN_BIT);
 		synchronize_irq(nfc->irq);
+		ret = -ETIMEDOUT;
 	}
 
 	vf610_nfc_clear_status(nfc);
+
+	return ret;
 }
 
 static irqreturn_t vf610_nfc_irq(int irq, void *data)
@@ -331,8 +335,8 @@ static inline void vf610_nfc_ecc_mode(struct vf610_nfc *nfc, int ecc_mode)
 			    CONFIG_ECC_MODE_SHIFT, ecc_mode);
 }
 
-static inline void vf610_nfc_run(struct vf610_nfc *nfc, u32 col, u32 row,
-				 u32 cmd1, u32 cmd2, u32 trfr_sz)
+static inline int vf610_nfc_run(struct vf610_nfc *nfc, u32 col, u32 row,
+				u32 cmd1, u32 cmd2, u32 trfr_sz)
 {
 	vf610_nfc_set_field(nfc, NFC_COL_ADDR, COL_ADDR_MASK,
 			    COL_ADDR_SHIFT, col);
@@ -348,7 +352,7 @@ static inline void vf610_nfc_run(struct vf610_nfc *nfc, u32 col, u32 row,
 		"col 0x%04x, row 0x%08x, cmd1 0x%08x, cmd2 0x%08x, len %d\n",
 		col, row, cmd1, cmd2, trfr_sz);
 
-	vf610_nfc_done(nfc);
+	return vf610_nfc_done(nfc);
 }
 
 static inline const struct nand_op_instr *
@@ -370,6 +374,7 @@ static int vf610_nfc_cmd(struct nand_chip *chip,
 	int op_id = -1, trfr_sz = 0, offset = 0;
 	u32 col = 0, row = 0, cmd1 = 0, cmd2 = 0, code = 0;
 	bool force8bit = false;
+	int ret;
 
 	/*
 	 * Some ops are optional, but the hardware requires the operations
@@ -448,9 +453,9 @@ static int vf610_nfc_cmd(struct nand_chip *chip,
 
 	cmd2 |= code << CMD_CODE_SHIFT;
 
-	vf610_nfc_run(nfc, col, row, cmd1, cmd2, trfr_sz);
+	ret = vf610_nfc_run(nfc, col, row, cmd1, cmd2, trfr_sz);
 
-	if (instr && instr->type == NAND_OP_DATA_IN_INSTR) {
+	if (!ret && instr && instr->type == NAND_OP_DATA_IN_INSTR) {
 		/*
 		 * Don't fix endianness on page access for historical reasons.
 		 * See comment in vf610_nfc_rd_from_sram
@@ -463,7 +468,7 @@ static int vf610_nfc_cmd(struct nand_chip *chip,
 	if (force8bit && (chip->options & NAND_BUSWIDTH_16))
 		vf610_nfc_set(nfc, NFC_FLASH_CONFIG, CONFIG_16BIT);
 
-	return 0;
+	return ret;
 }
 
 static const struct nand_op_parser vf610_nfc_op_parser = NAND_OP_PARSER(
@@ -572,7 +577,7 @@ static int vf610_nfc_read_page(struct nand_chip *chip, uint8_t *buf,
 	struct mtd_info *mtd = nand_to_mtd(chip);
 	int trfr_sz = mtd->writesize + vf610_nfc_spare_size(mtd);
 	u32 row = 0, cmd1 = 0, cmd2 = 0, code = 0;
-	int stat;
+	int stat, ret;
 
 	vf610_nfc_select_target(chip, chip->cur_cs);
 
@@ -587,8 +592,10 @@ static int vf610_nfc_read_page(struct nand_chip *chip, uint8_t *buf,
 	cmd2 |= code << CMD_CODE_SHIFT;
 
 	vf610_nfc_ecc_mode(nfc, nfc->ecc_mode);
-	vf610_nfc_run(nfc, 0, row, cmd1, cmd2, trfr_sz);
+	ret = vf610_nfc_run(nfc, 0, row, cmd1, cmd2, trfr_sz);
 	vf610_nfc_ecc_mode(nfc, ECC_BYPASS);
+	if (ret)
+		return ret;
 
 	/*
 	 * Don't fix endianness on page access for historical reasons.
@@ -651,8 +658,10 @@ static int vf610_nfc_write_page(struct nand_chip *chip, const uint8_t *buf,
 	cmd2 |= code << CMD_CODE_SHIFT;
 
 	vf610_nfc_ecc_mode(nfc, nfc->ecc_mode);
-	vf610_nfc_run(nfc, 0, row, cmd1, cmd2, trfr_sz);
+	ret = vf610_nfc_run(nfc, 0, row, cmd1, cmd2, trfr_sz);
 	vf610_nfc_ecc_mode(nfc, ECC_BYPASS);
+	if (ret)
+		return ret;
 
 	ret = nand_status_op(chip, &status);
 	if (ret)
-- 
2.56.0

Re: [PATCH v2 3/3] mtd: rawnand: vf610_nfc: return an error when a command times out

[Mehmet Fide]

Hi Miquel,

Please do not apply this patch as it is. Sashiko is right: when the
timeout hits the raw re-read in vf610_nfc_correct_data(),
vf610_nfc_read_page() counts it as an ECC failure and returns 0 instead
of the error. I could reproduce it by forcing the timeout there.

v3 will return it. Patches 1/3 and 2/3 are not affected.

Thanks,
Mehmet