Add nand_shutdown to wait for current nand operations to finish and prevent
further operations by changing the nand flash state to FL_SHUTDOWN.
This is addressing a problem observed during reboot tests using UBIFS
root file system: NAND erase operations that are in progress during
system reboot/shutdown are causing partial erased blocks. Although UBI should
be able to detect and recover from this error, this change will avoid
the creation of partial erased blocks on reboot in the middle of a NAND erase
operation.
Signed-off-by: Scott Branden <[email protected]>
---
drivers/mtd/nand/nand_base.c | 11 +++++++++++
include/linux/mtd/nand.h | 7 +++++++
2 files changed, 18 insertions(+)
diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
index 5b5c627..100a967 100644
--- a/drivers/mtd/nand/nand_base.c
+++ b/drivers/mtd/nand/nand_base.c
@@ -4236,6 +4236,17 @@ void nand_release(struct mtd_info *mtd)
}
EXPORT_SYMBOL_GPL(nand_release);
+/**
+ * nand_shutdown - [NAND Interface] finish the current nand operation and
+ * prevent further operations
+ * @mtd: MTD device structure
+ */
+int nand_shutdown(struct mtd_info *mtd)
+{
+ return nand_get_device(mtd, FL_SHUTDOWN);
+}
+EXPORT_SYMBOL_GPL(nand_shutdown);
+
static int __init nand_base_init(void)
{
led_trigger_register_simple("nand-disk", &nand_led_trigger);
diff --git a/include/linux/mtd/nand.h b/include/linux/mtd/nand.h
index e4d451e..80e4367 100644
--- a/include/linux/mtd/nand.h
+++ b/include/linux/mtd/nand.h
@@ -48,6 +48,13 @@ extern int nand_lock(struct mtd_info *mtd, loff_t ofs, uint64_t len);
/* unlocks specified locked blocks */
extern int nand_unlock(struct mtd_info *mtd, loff_t ofs, uint64_t len);
+/*
+ * Internal helper for board drivers which need to make sure that the current
+ * nand operation is finished and further operations are prevented before
+ * rebooting the system.
+ */
+extern int nand_shutdown(struct mtd_info *mtd);
+
/* The maximum number of NAND chips in an array */
#define NAND_MAX_CHIPS 8
--
2.1.3
On Thu, Nov 20, 2014 at 11:18:05AM -0800, Scott Branden wrote:
> Add nand_shutdown to wait for current nand operations to finish and prevent
> further operations by changing the nand flash state to FL_SHUTDOWN.
>
> This is addressing a problem observed during reboot tests using UBIFS
> root file system: NAND erase operations that are in progress during
> system reboot/shutdown are causing partial erased blocks. Although UBI should
> be able to detect and recover from this error, this change will avoid
> the creation of partial erased blocks on reboot in the middle of a NAND erase
> operation.
[...]
> diff --git a/include/linux/mtd/nand.h b/include/linux/mtd/nand.h
> index e4d451e..80e4367 100644
> --- a/include/linux/mtd/nand.h
> +++ b/include/linux/mtd/nand.h
> @@ -48,6 +48,13 @@ extern int nand_lock(struct mtd_info *mtd, loff_t ofs, uint64_t len);
> /* unlocks specified locked blocks */
> extern int nand_unlock(struct mtd_info *mtd, loff_t ofs, uint64_t len);
>
> +/*
> + * Internal helper for board drivers which need to make sure that the current
> + * nand operation is finished and further operations are prevented before
> + * rebooting the system.
> + */
> +extern int nand_shutdown(struct mtd_info *mtd);
I don't think we should push this out to the board drivers. And I
definitely don't want to merge this function without any user. It'd turn
out like our useless nand_lock() and nand_unlock() functions.
> +
> /* The maximum number of NAND chips in an array */
> #define NAND_MAX_CHIPS 8
>
So, to do this right, I think maybe we should add an (optional)
reboot_notifier field to struct mtd_info, and use it to register a
system reboot notifier on behalf of the lower layers. We can do the
registration in mtd_device_parse_register(), I think, and any
unregistration in mtd_device_unregister().
With this approach, we can actually move the cfi_cmdset_000{1,2}.c
reboot notifiers over to the same common code. They just will have to
fill out their mtd->reboot_notifier field.
How about the following two untested patches?
From: Brian Norris <[email protected]>
cfi_cmdset_000{1,2}.c already implement their own reboot notifiers, and
we're going to add one for NAND. Let's put the boilerplate in one place.
Signed-off-by: Brian Norris <[email protected]>
---
drivers/mtd/mtdcore.c | 20 ++++++++++++++++++++
include/linux/mtd/mtd.h | 1 +
2 files changed, 21 insertions(+)
diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c
index 4c611871d7e6..b80d44f9751d 100644
--- a/drivers/mtd/mtdcore.c
+++ b/drivers/mtd/mtdcore.c
@@ -37,6 +37,7 @@
#include <linux/backing-dev.h>
#include <linux/gfp.h>
#include <linux/slab.h>
+#include <linux/reboot.h>
#include <linux/mtd/mtd.h>
#include <linux/mtd/partitions.h>
@@ -365,6 +366,17 @@ static struct device_type mtd_devtype = {
.release = mtd_release,
};
+static int mtd_reboot_notifier(struct notifier_block *n, unsigned long state,
+ void *cmd)
+{
+ struct mtd_info *mtd;
+
+ mtd = container_of(n, struct mtd_info, reboot_notifier);
+ mtd->_reboot(mtd);
+
+ return NOTIFY_DONE;
+}
+
/**
* add_mtd_device - register an MTD device
* @mtd: pointer to new MTD device info structure
@@ -565,6 +577,11 @@ int mtd_device_parse_register(struct mtd_info *mtd, const char * const *types,
err = -ENODEV;
}
+ if (mtd->_reboot) {
+ mtd->reboot_notifier.notifier_call = mtd_reboot_notifier;
+ register_reboot_notifier(&mtd->reboot_notifier);
+ }
+
return err;
}
EXPORT_SYMBOL_GPL(mtd_device_parse_register);
@@ -579,6 +596,9 @@ int mtd_device_unregister(struct mtd_info *master)
{
int err;
+ if (master->_reboot)
+ unregister_reboot_notifier(&master->reboot_notifier);
+
err = del_mtd_partitions(master);
if (err)
return err;
diff --git a/include/linux/mtd/mtd.h b/include/linux/mtd/mtd.h
index 031ff3a9a0bd..c06f5373d870 100644
--- a/include/linux/mtd/mtd.h
+++ b/include/linux/mtd/mtd.h
@@ -227,6 +227,7 @@ struct mtd_info {
int (*_block_markbad) (struct mtd_info *mtd, loff_t ofs);
int (*_suspend) (struct mtd_info *mtd);
void (*_resume) (struct mtd_info *mtd);
+ void (*_reboot) (struct mtd_info *mtd);
/*
* If the driver is something smart, like UBI, it may need to maintain
* its own reference counting. The below functions are only for driver.
--
From: Scott Branden <[email protected]>
Add nand_shutdown to wait for current nand operations to finish and prevent
further operations by changing the nand flash state to FL_SHUTDOWN.
This is addressing a problem observed during reboot tests using UBIFS
root file system: NAND erase operations that are in progress during
system reboot/shutdown are causing partial erased blocks. Although UBI should
be able to detect and recover from this error, this change will avoid
the creation of partial erased blocks on reboot in the middle of a NAND erase
operation.
Signed-off-by: Scott Branden <[email protected]>
Signed-off-by: Brian Norris <[email protected]>
---
drivers/mtd/nand/nand_base.c | 11 +++++++++++
1 file changed, 11 insertions(+)
diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
index 4cbd14552d48..06ae96834063 100644
--- a/drivers/mtd/nand/nand_base.c
+++ b/drivers/mtd/nand/nand_base.c
@@ -2944,6 +2944,16 @@ static void nand_resume(struct mtd_info *mtd)
__func__);
}
+/**
+ * nand_shutdown - [MTD Interface] Finish the current NAND operation and
+ * prevent further operations
+ * @mtd: MTD device structure
+ */
+static int nand_shutdown(struct mtd_info *mtd)
+{
+ return nand_get_device(mtd, FL_SHUTDOWN);
+}
+
/* Set default functions */
static void nand_set_defaults(struct nand_chip *chip, int busw)
{
@@ -4146,6 +4156,7 @@ int nand_scan_tail(struct mtd_info *mtd)
mtd->_unlock = NULL;
mtd->_suspend = nand_suspend;
mtd->_resume = nand_resume;
+ mtd->_reboot = nand_shutdown;
mtd->_block_isreserved = nand_block_isreserved;
mtd->_block_isbad = nand_block_isbad;
mtd->_block_markbad = nand_block_markbad;
--
On 14-11-26 01:10 AM, Brian Norris wrote:
> On Thu, Nov 20, 2014 at 11:18:05AM -0800, Scott Branden wrote:
>> Add nand_shutdown to wait for current nand operations to finish and prevent
>> further operations by changing the nand flash state to FL_SHUTDOWN.
>>
>> This is addressing a problem observed during reboot tests using UBIFS
>> root file system: NAND erase operations that are in progress during
>> system reboot/shutdown are causing partial erased blocks. Although UBI should
>> be able to detect and recover from this error, this change will avoid
>> the creation of partial erased blocks on reboot in the middle of a NAND erase
>> operation.
> [...]
>> diff --git a/include/linux/mtd/nand.h b/include/linux/mtd/nand.h
>> index e4d451e..80e4367 100644
>> --- a/include/linux/mtd/nand.h
>> +++ b/include/linux/mtd/nand.h
>> @@ -48,6 +48,13 @@ extern int nand_lock(struct mtd_info *mtd, loff_t ofs, uint64_t len);
>> /* unlocks specified locked blocks */
>> extern int nand_unlock(struct mtd_info *mtd, loff_t ofs, uint64_t len);
>>
>> +/*
>> + * Internal helper for board drivers which need to make sure that the current
>> + * nand operation is finished and further operations are prevented before
>> + * rebooting the system.
>> + */
>> +extern int nand_shutdown(struct mtd_info *mtd);
>
> I don't think we should push this out to the board drivers. And I
> definitely don't want to merge this function without any user. It'd turn
> out like our useless nand_lock() and nand_unlock() functions.
>
>> +
>> /* The maximum number of NAND chips in an array */
>> #define NAND_MAX_CHIPS 8
>>
>
> So, to do this right, I think maybe we should add an (optional)
> reboot_notifier field to struct mtd_info, and use it to register a
> system reboot notifier on behalf of the lower layers. We can do the
> registration in mtd_device_parse_register(), I think, and any
> unregistration in mtd_device_unregister().
>
> With this approach, we can actually move the cfi_cmdset_000{1,2}.c
> reboot notifiers over to the same common code. They just will have to
> fill out their mtd->reboot_notifier field.
>
> How about the following two untested patches?
This is great if we can move this to common code as it should be done
for all NAND (and NOR) devices. For nand, I suggest we install a
reboot_notifier in a common location like nand_base rather than each
driver. If a particular driver wishes to override this they are free to
do so in their particular driver?
>
> From: Brian Norris <[email protected]>
>
> cfi_cmdset_000{1,2}.c already implement their own reboot notifiers, and
> we're going to add one for NAND. Let's put the boilerplate in one place.
>
> Signed-off-by: Brian Norris <[email protected]>
> ---
> drivers/mtd/mtdcore.c | 20 ++++++++++++++++++++
> include/linux/mtd/mtd.h | 1 +
> 2 files changed, 21 insertions(+)
>
> diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c
> index 4c611871d7e6..b80d44f9751d 100644
> --- a/drivers/mtd/mtdcore.c
> +++ b/drivers/mtd/mtdcore.c
> @@ -37,6 +37,7 @@
> #include <linux/backing-dev.h>
> #include <linux/gfp.h>
> #include <linux/slab.h>
> +#include <linux/reboot.h>
>
> #include <linux/mtd/mtd.h>
> #include <linux/mtd/partitions.h>
> @@ -365,6 +366,17 @@ static struct device_type mtd_devtype = {
> .release = mtd_release,
> };
>
> +static int mtd_reboot_notifier(struct notifier_block *n, unsigned long state,
> + void *cmd)
> +{
> + struct mtd_info *mtd;
> +
> + mtd = container_of(n, struct mtd_info, reboot_notifier);
> + mtd->_reboot(mtd);
> +
> + return NOTIFY_DONE;
> +}
> +
> /**
> * add_mtd_device - register an MTD device
> * @mtd: pointer to new MTD device info structure
> @@ -565,6 +577,11 @@ int mtd_device_parse_register(struct mtd_info *mtd, const char * const *types,
> err = -ENODEV;
> }
>
> + if (mtd->_reboot) {
> + mtd->reboot_notifier.notifier_call = mtd_reboot_notifier;
> + register_reboot_notifier(&mtd->reboot_notifier);
> + }
> +
> return err;
> }
> EXPORT_SYMBOL_GPL(mtd_device_parse_register);
> @@ -579,6 +596,9 @@ int mtd_device_unregister(struct mtd_info *master)
> {
> int err;
>
> + if (master->_reboot)
> + unregister_reboot_notifier(&master->reboot_notifier);
> +
> err = del_mtd_partitions(master);
> if (err)
> return err;
> diff --git a/include/linux/mtd/mtd.h b/include/linux/mtd/mtd.h
> index 031ff3a9a0bd..c06f5373d870 100644
> --- a/include/linux/mtd/mtd.h
> +++ b/include/linux/mtd/mtd.h
> @@ -227,6 +227,7 @@ struct mtd_info {
> int (*_block_markbad) (struct mtd_info *mtd, loff_t ofs);
> int (*_suspend) (struct mtd_info *mtd);
> void (*_resume) (struct mtd_info *mtd);
> + void (*_reboot) (struct mtd_info *mtd);
> /*
> * If the driver is something smart, like UBI, it may need to maintain
> * its own reference counting. The below functions are only for driver.
>
On 14-11-26 01:10 AM, Brian Norris wrote:
> + mtd->_reboot = nand_shutdown;
Ah, I see you added this here. This is great. We'll try out your
patchset on our driver and remove the reboot notifier from it.
Hi Brian,
I just sent out a new patch with your changes incorporated.
Thanks,
Scott
On 14-11-26 01:10 AM, Brian Norris wrote:
> On Thu, Nov 20, 2014 at 11:18:05AM -0800, Scott Branden wrote:
>> Add nand_shutdown to wait for current nand operations to finish and prevent
>> further operations by changing the nand flash state to FL_SHUTDOWN.
>>
>> This is addressing a problem observed during reboot tests using UBIFS
>> root file system: NAND erase operations that are in progress during
>> system reboot/shutdown are causing partial erased blocks. Although UBI should
>> be able to detect and recover from this error, this change will avoid
>> the creation of partial erased blocks on reboot in the middle of a NAND erase
>> operation.
> [...]
>> diff --git a/include/linux/mtd/nand.h b/include/linux/mtd/nand.h
>> index e4d451e..80e4367 100644
>> --- a/include/linux/mtd/nand.h
>> +++ b/include/linux/mtd/nand.h
>> @@ -48,6 +48,13 @@ extern int nand_lock(struct mtd_info *mtd, loff_t ofs, uint64_t len);
>> /* unlocks specified locked blocks */
>> extern int nand_unlock(struct mtd_info *mtd, loff_t ofs, uint64_t len);
>>
>> +/*
>> + * Internal helper for board drivers which need to make sure that the current
>> + * nand operation is finished and further operations are prevented before
>> + * rebooting the system.
>> + */
>> +extern int nand_shutdown(struct mtd_info *mtd);
>
> I don't think we should push this out to the board drivers. And I
> definitely don't want to merge this function without any user. It'd turn
> out like our useless nand_lock() and nand_unlock() functions.
>
>> +
>> /* The maximum number of NAND chips in an array */
>> #define NAND_MAX_CHIPS 8
>>
>
> So, to do this right, I think maybe we should add an (optional)
> reboot_notifier field to struct mtd_info, and use it to register a
> system reboot notifier on behalf of the lower layers. We can do the
> registration in mtd_device_parse_register(), I think, and any
> unregistration in mtd_device_unregister().
>
> With this approach, we can actually move the cfi_cmdset_000{1,2}.c
> reboot notifiers over to the same common code. They just will have to
> fill out their mtd->reboot_notifier field.
>
> How about the following two untested patches?
>
> From: Brian Norris <[email protected]>
>
> cfi_cmdset_000{1,2}.c already implement their own reboot notifiers, and
> we're going to add one for NAND. Let's put the boilerplate in one place.
>
> Signed-off-by: Brian Norris <[email protected]>
> ---
> drivers/mtd/mtdcore.c | 20 ++++++++++++++++++++
> include/linux/mtd/mtd.h | 1 +
> 2 files changed, 21 insertions(+)
>
> diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c
> index 4c611871d7e6..b80d44f9751d 100644
> --- a/drivers/mtd/mtdcore.c
> +++ b/drivers/mtd/mtdcore.c
> @@ -37,6 +37,7 @@
> #include <linux/backing-dev.h>
> #include <linux/gfp.h>
> #include <linux/slab.h>
> +#include <linux/reboot.h>
>
> #include <linux/mtd/mtd.h>
> #include <linux/mtd/partitions.h>
> @@ -365,6 +366,17 @@ static struct device_type mtd_devtype = {
> .release = mtd_release,
> };
>
> +static int mtd_reboot_notifier(struct notifier_block *n, unsigned long state,
> + void *cmd)
> +{
> + struct mtd_info *mtd;
> +
> + mtd = container_of(n, struct mtd_info, reboot_notifier);
> + mtd->_reboot(mtd);
> +
> + return NOTIFY_DONE;
> +}
> +
> /**
> * add_mtd_device - register an MTD device
> * @mtd: pointer to new MTD device info structure
> @@ -565,6 +577,11 @@ int mtd_device_parse_register(struct mtd_info *mtd, const char * const *types,
> err = -ENODEV;
> }
>
> + if (mtd->_reboot) {
> + mtd->reboot_notifier.notifier_call = mtd_reboot_notifier;
> + register_reboot_notifier(&mtd->reboot_notifier);
> + }
> +
> return err;
> }
> EXPORT_SYMBOL_GPL(mtd_device_parse_register);
> @@ -579,6 +596,9 @@ int mtd_device_unregister(struct mtd_info *master)
> {
> int err;
>
> + if (master->_reboot)
> + unregister_reboot_notifier(&master->reboot_notifier);
> +
> err = del_mtd_partitions(master);
> if (err)
> return err;
> diff --git a/include/linux/mtd/mtd.h b/include/linux/mtd/mtd.h
> index 031ff3a9a0bd..c06f5373d870 100644
> --- a/include/linux/mtd/mtd.h
> +++ b/include/linux/mtd/mtd.h
> @@ -227,6 +227,7 @@ struct mtd_info {
> int (*_block_markbad) (struct mtd_info *mtd, loff_t ofs);
> int (*_suspend) (struct mtd_info *mtd);
> void (*_resume) (struct mtd_info *mtd);
> + void (*_reboot) (struct mtd_info *mtd);
> /*
> * If the driver is something smart, like UBI, it may need to maintain
> * its own reference counting. The below functions are only for driver.
>