Re: Prevent Nand page writes on Power failure

[Sascha Hauer <s.hauer@xxxxxxxxxxxxxx>]

On Wed, Feb 20, 2019 at 02:58:20PM +0100, Sascha Hauer wrote:
> 
> Hi All,
> 
> I have hardware here for which the normal way to turn off is just to cut
> the power. When the powercut happens during a NAND page write then we
> get more or less completely written pages during next boot. Very rarely
> it seems to happen that such a half written page with only very few
> flipped bits is erroneously detected as empty and written again which
> then results in ECC errors when reading the data.
> 
> The Nand in question is a Micron MT29F4G08ABADAH4 and in TN2917 Micron
> clearly states:
> 
> | Power loss during NAND array operations (especially Program/Erase) is a
> | violation of the NAND voltage specifications, which is not supported and
> | should be avoided
> 
> Micron suggests to make the capacitors on the Nand chips supply input
> big enough that every started operation will be finished before the
> power goes down. Now we don't have that situation here, what I have
> though is a power good status GPIO, so my job is to wire that up to the
> Nand write operations.
> 
> Now my question is how could that be done? I assume for some people a
> power good failure means that we should write all important data away,
> rather than preventing any Nand access. Given it's a policy decision I
> assume user space should be involved, right?  An option might be to
> introcude some sysfs entry to switch mtd devices to readonly mode. Would
> that be fine? Other suggestions?
> 

FYI the attached patch is what I have ended up with for now. Not
upstreamable of course, but works for me. I have the feeling that it's
not the last time I have this problem, so an upstreamable solution is
really desired.

Sascha

-----------------------------8<--------------------------------

>From 04e76aaea00d2f92b51904e2b1611a3d64081e71 Mon Sep 17 00:00:00 2001
From: Sascha Hauer <s.hauer@xxxxxxxxxxxxxx>
Date: Thu, 21 Feb 2019 12:30:29 +0100
Subject: [PATCH] mtd: nand: raw: Allow to prevent write operations on power
 failures

When doing write operations the Nand chips supply must be within
specified ranges, otherwise written pages can be corrupted in various
ways, including cases where a page looks empty, but gets corrupted when
written to, or pages which first look good, but get corrupted over time.

This adds a power_good mutex to the nand chip which is acquired before
write/erase and released afterwards. Board code can acquire this mutex
on a power failure to prevent further write accesses to the Nand chip.

Signed-off-by: Sascha Hauer <s.hauer@xxxxxxxxxxxxxx>
---
 drivers/mtd/nand/raw/gpmi-nand/gpmi-nand.c | 50 ++++++++++++++++++++++
 drivers/mtd/nand/raw/gpmi-nand/gpmi-nand.h |  2 +
 drivers/mtd/nand/raw/nand_base.c           | 22 +++++++---
 include/linux/mtd/rawnand.h                |  2 +
 4 files changed, 71 insertions(+), 5 deletions(-)

diff --git a/drivers/mtd/nand/raw/gpmi-nand/gpmi-nand.c b/drivers/mtd/nand/raw/gpmi-nand/gpmi-nand.c
index 1c1ebbc82824..b13ddf7b7b75 100644
--- a/drivers/mtd/nand/raw/gpmi-nand/gpmi-nand.c
+++ b/drivers/mtd/nand/raw/gpmi-nand/gpmi-nand.c
@@ -13,6 +13,7 @@
 #include <linux/mtd/partitions.h>
 #include <linux/of.h>
 #include <linux/of_device.h>
+#include <linux/gpio/consumer.h>
 #include "gpmi-nand.h"
 #include "bch-regs.h"
 
@@ -628,6 +629,50 @@ err_clock:
 	return err;
 }
 
+static irqreturn_t gpmi_pwr_good_irq(int irq, void *dev_id)
+{
+	struct gpmi_nand_data *this = dev_id;
+	int level;
+
+	level = gpiod_get_value_cansleep(this->pwrgood_gpio);
+	if (level) {
+		mutex_lock(&this->nand.power_lock);
+	} else {
+		mutex_unlock(&this->nand.power_lock);
+		dev_info(this->dev, "Spurious Power Failure detected\n");
+	}
+
+	return IRQ_HANDLED;
+}
+
+static int gpmi_get_pwrgood(struct gpmi_nand_data *this)
+{
+	struct gpio_desc *desc;
+	int irq, ret;
+
+	desc = devm_gpiod_get_index_optional(this->dev, "pwrgood", 0, GPIOD_IN);
+	if (!desc)
+		return 0;
+
+	if (IS_ERR(desc))
+		return PTR_ERR(desc);
+
+	irq = gpiod_to_irq(desc);
+
+	ret = devm_request_threaded_irq(this->dev, irq,
+			NULL, gpmi_pwr_good_irq,
+			IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
+			"pwrgood", this);
+	if (ret)
+		return ret;
+
+	this->pwrgood_gpio = desc;
+
+	dev_info(this->dev, "using irq %d as power good input\n", irq);
+
+	return 0;
+}
+
 static int acquire_resources(struct gpmi_nand_data *this)
 {
 	int ret;
@@ -651,6 +696,11 @@ static int acquire_resources(struct gpmi_nand_data *this)
 	ret = gpmi_get_clks(this);
 	if (ret)
 		goto exit_clock;
+
+	ret = gpmi_get_pwrgood(this);
+	if (ret)
+		goto exit_clock;
+
 	return 0;
 
 exit_clock:
diff --git a/drivers/mtd/nand/raw/gpmi-nand/gpmi-nand.h b/drivers/mtd/nand/raw/gpmi-nand/gpmi-nand.h
index 69cd0cbde4f2..251e02bb4338 100644
--- a/drivers/mtd/nand/raw/gpmi-nand/gpmi-nand.h
+++ b/drivers/mtd/nand/raw/gpmi-nand/gpmi-nand.h
@@ -148,6 +148,8 @@ struct gpmi_nand_data {
 	void			*auxiliary_virt;
 	dma_addr_t		auxiliary_phys;
 
+	struct gpio_desc	*pwrgood_gpio;
+
 	void			*raw_buffer;
 
 	/* DMA channels */
diff --git a/drivers/mtd/nand/raw/nand_base.c b/drivers/mtd/nand/raw/nand_base.c
index d527e448ce19..d9487503bec0 100644
--- a/drivers/mtd/nand/raw/nand_base.c
+++ b/drivers/mtd/nand/raw/nand_base.c
@@ -2115,6 +2115,8 @@ int nand_erase_op(struct nand_chip *chip, unsigned int eraseblock)
 	int ret;
 	u8 status;
 
+	mutex_lock(&chip->power_lock);
+
 	if (chip->exec_op) {
 		const struct nand_sdr_timings *sdr =
 			nand_get_sdr_timings(&chip->data_interface);
@@ -2133,26 +2135,30 @@ int nand_erase_op(struct nand_chip *chip, unsigned int eraseblock)
 
 		ret = nand_exec_op(chip, &op);
 		if (ret)
-			return ret;
+			goto out;
 
 		ret = nand_status_op(chip, &status);
 		if (ret)
-			return ret;
+			goto out;
 	} else {
 		chip->cmdfunc(mtd, NAND_CMD_ERASE1, -1, page);
 		chip->cmdfunc(mtd, NAND_CMD_ERASE2, -1, -1);
 
 		ret = chip->waitfunc(mtd, chip);
 		if (ret < 0)
-			return ret;
+			goto out;
 
 		status = ret;
 	}
 
 	if (status & NAND_STATUS_FAIL)
-		return -EIO;
+		ret = -EIO;
+	else
+		ret = 0;
+out:
+	mutex_unlock(&chip->power_lock);
 
-	return 0;
+	return ret;
 }
 EXPORT_SYMBOL_GPL(nand_erase_op);
 
@@ -4330,6 +4336,8 @@ static int nand_write_page(struct mtd_info *mtd, struct nand_chip *chip,
 	else
 		subpage = 0;
 
+	mutex_lock(&chip->power_lock);
+
 	if (unlikely(raw))
 		status = chip->ecc.write_page_raw(mtd, chip, buf,
 						  oob_required, page);
@@ -4340,6 +4348,8 @@ static int nand_write_page(struct mtd_info *mtd, struct nand_chip *chip,
 		status = chip->ecc.write_page(mtd, chip, buf, oob_required,
 					      page);
 
+	mutex_unlock(&chip->power_lock);
+
 	if (status < 0)
 		return status;
 
@@ -6786,6 +6796,8 @@ int nand_scan_with_ids(struct mtd_info *mtd, int maxchips,
 	struct nand_chip *chip = mtd_to_nand(mtd);
 	int ret;
 
+	mutex_init(&chip->power_lock);
+
 	if (maxchips) {
 		ret = nand_scan_ident(mtd, maxchips, ids);
 		if (ret)
diff --git a/include/linux/mtd/rawnand.h b/include/linux/mtd/rawnand.h
index efb2345359bb..638b187eb198 100644
--- a/include/linux/mtd/rawnand.h
+++ b/include/linux/mtd/rawnand.h
@@ -1357,6 +1357,8 @@ struct nand_chip {
 
 	struct nand_bbt_descr *badblock_pattern;
 
+	struct mutex power_lock;
+
 	void *priv;
 
 	struct {
-- 
2.20.1


-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

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