[PATCH 0/2] MTD: Move reboot notifier from UBI to cfi_cmdset_0002

[PATCH 0/2] MTD: Move reboot notifier from UBI to cfi_cmdset_0002

[Kevin Cernekee]

During the last filesystem sync prior to rebooting, UBI/UBIFS typically
schedules a flash erase operation.  It is important to ensure that this
operation completes before restarting the system, for the following
reasons:

1) Some systems do not toggle the flash reset pin upon rebooting, so if
the flash is stuck in erase/program mode the bootloader will not come
back up.  This is especially likely on NOR flash devices since the erase
time can be 1-2 seconds.

2) While UBI is able to recover gracefully from partial erasures, that
doesn't mean it is a good idea to intentionally allow them to happen on
a regular basis.  At the minimum it will cause another unnecessary
erasure when the system comes back up.

3) It's not very polite to leave the hardware in an unclean state.

A reboot notifier was previously added to UBI to handle this condition,
but it caused problems with hibernation and does not handle the general
case of "flash operation in progress during reboot."  Some background
information can be found here:

http://lists.infradead.org/pipermail/linux-mtd/2010-March/029477.html

http://lkml.org/lkml/2009/6/9/16

http://lkml.org/lkml/2010/2/12/114

http://lkml.org/lkml/2010/2/12/144

Note that the bootloader cannot even begin execution if the NOR flash is
not in read array mode.  There is only one NOR flash chip on this system
and it contains both the bootloader and the Linux filesystems.  This is
not a bug in the bootloader.  Similar problems have been seen on other
boards:

http://www.mail-archive.com/davinci-linux-open-source@linux.davincidsp.com/msg14071.html

http://www.mail-archive.com/bk-commits-head@vger.kernel.org/msg06294.html

This set of patches adds a new reboot notifier to the AMD flash driver,
then removes the UBI reboot notifier.  I have verified that the new
scheme is equally effective in solving the original problem.

[PATCH 1/2] MTD: cfi_cmdset_0002: Add reboot notifier for AMD flashes

[Kevin Cernekee]

Ensure that the flash device is in a quiescent state before rebooting.
The implementation is closely modeled after the cfe_cmdset_0001 reboot
notifier, commit 963a6fb0a0d336d0513083b7e4b5c3ff9d6d2061 .

Signed-off-by: Kevin Cernekee <cernekee@gmail.com>
---
 drivers/mtd/chips/cfi_cmdset_0002.c |   50 +++++++++++++++++++++++++++++++++++
 1 files changed, 50 insertions(+), 0 deletions(-)

diff --git a/drivers/mtd/chips/cfi_cmdset_0002.c b/drivers/mtd/chips/cfi_cmdset_0002.c
index ea2a7f6..4b10072 100644
--- a/drivers/mtd/chips/cfi_cmdset_0002.c
+++ b/drivers/mtd/chips/cfi_cmdset_0002.c
@@ -32,6 +32,7 @@
 #include <linux/slab.h>
 #include <linux/delay.h>
 #include <linux/interrupt.h>
+#include <linux/reboot.h>
 #include <linux/mtd/compatmac.h>
 #include <linux/mtd/map.h>
 #include <linux/mtd/mtd.h>
@@ -56,6 +57,7 @@ static int cfi_amdstd_erase_varsize(struct mtd_info *, struct erase_info *);
 static void cfi_amdstd_sync (struct mtd_info *);
 static int cfi_amdstd_suspend (struct mtd_info *);
 static void cfi_amdstd_resume (struct mtd_info *);
+static int cfi_amdstd_reboot(struct notifier_block *, unsigned long, void *);
 static int cfi_amdstd_secsi_read (struct mtd_info *, loff_t, size_t, size_t *, u_char *);
 
 static void cfi_amdstd_destroy(struct mtd_info *);
@@ -351,6 +353,8 @@ struct mtd_info *cfi_cmdset_0002(struct map_info *map, int primary)
 	mtd->name    = map->name;
 	mtd->writesize = 1;
 
+	mtd->reboot_notifier.notifier_call = cfi_amdstd_reboot;
+
 	if (cfi->cfi_mode==CFI_MODE_CFI){
 		unsigned char bootloc;
 		/*
@@ -487,6 +491,7 @@ static struct mtd_info *cfi_amdstd_setup(struct mtd_info *mtd)
 #endif
 
 	__module_get(THIS_MODULE);
+	register_reboot_notifier(&mtd->reboot_notifier);
 	return mtd;
 
  setup_err:
@@ -628,6 +633,10 @@ static int get_chip(struct map_info *map, struct flchip *chip, unsigned long adr
 		chip->state = FL_READY;
 		return 0;
 
+	case FL_SHUTDOWN:
+		/* The machine is rebooting */
+		return -EIO;
+
 	case FL_POINT:
 		/* Only if there's no operation suspended... */
 		if (mode == FL_READY && chip->oldstate == FL_READY)
@@ -1918,11 +1927,52 @@ static void cfi_amdstd_resume(struct mtd_info *mtd)
 	}
 }
 
+
+static int cfi_amdstd_reset(struct mtd_info *mtd)
+{
+	struct map_info *map = mtd->priv;
+	struct cfi_private *cfi = map->fldrv_priv;
+	int i, ret;
+	struct flchip *chip;
+
+	for (i = 0; i < cfi->numchips; i++) {
+
+		chip = &cfi->chips[i];
+
+		spin_lock(chip->mutex);
+
+		ret = get_chip(map, chip, chip->start, FL_SHUTDOWN);
+		if (!ret) {
+			map_write(map, CMD(0xF0), chip->start);
+			chip->state = FL_SHUTDOWN;
+			put_chip(map, chip, chip->start);
+		}
+
+		spin_unlock(chip->mutex);
+	}
+
+	return 0;
+}
+
+
+static int cfi_amdstd_reboot(struct notifier_block *nb, unsigned long val,
+			       void *v)
+{
+	struct mtd_info *mtd;
+
+	mtd = container_of(nb, struct mtd_info, reboot_notifier);
+	cfi_amdstd_reset(mtd);
+	return NOTIFY_DONE;
+}
+
+
 static void cfi_amdstd_destroy(struct mtd_info *mtd)
 {
 	struct map_info *map = mtd->priv;
 	struct cfi_private *cfi = map->fldrv_priv;
 
+	cfi_amdstd_reset(mtd);
+	unregister_reboot_notifier(&mtd->reboot_notifier);
 	kfree(cfi->cmdset_priv);
 	kfree(cfi->cfiq);
 	kfree(cfi);
-- 
1.6.3.1

Re: [PATCH 1/2] MTD: cfi_cmdset_0002: Add reboot notifier for AMD flashes

[Artem Bityutskiy]

On Tue, 2010-04-13 at 13:30 -0700, Kevin Cernekee wrote:
> +static int cfi_amdstd_reset(struct mtd_info *mtd)
> +{
> +	struct map_info *map = mtd->priv;
> +	struct cfi_private *cfi = map->fldrv_priv;
> +	int i, ret;
> +	struct flchip *chip;
> +
> +	for (i = 0; i < cfi->numchips; i++) {
> +
> +		chip = &cfi->chips[i];
> +
> +		spin_lock(chip->mutex);
> +
> +		ret = get_chip(map, chip, chip->start, FL_SHUTDOWN);
> +		if (!ret) {
> +			map_write(map, CMD(0xF0), chip->start);
> +			chip->state = FL_SHUTDOWN;
> +			put_chip(map, chip, chip->start);
> +		}
> +
> +		spin_unlock(chip->mutex);
> +	}
> +
> +	return 0;
> +}

Kevin, I'd suggest to document why you do this in the code, just for the
next generation, or for the archaeologists who will dig MTD code in the
future. 

-- 
Best Regards,
Artem Bityutskiy (Артём Битюцкий)

Re: [PATCH 1/2] MTD: cfi_cmdset_0002: Add reboot notifier for AMD flashes

[Fabio Giovagnini]

Exactly!!!!
Why this is requested?
Is it necessary for the best proper woring mode?

Thanks a lot


In data mercoledì 14 aprile 2010 06:57:58, Artem Bityutskiy ha scritto:
: > On Tue, 2010-04-13 at 13:30 -0700, Kevin Cernekee wrote:
> > +static int cfi_amdstd_reset(struct mtd_info *mtd)
> > +{
> > +	struct map_info *map = mtd->priv;
> > +	struct cfi_private *cfi = map->fldrv_priv;
> > +	int i, ret;
> > +	struct flchip *chip;
> > +
> > +	for (i = 0; i < cfi->numchips; i++) {
> > +
> > +		chip = &cfi->chips[i];
> > +
> > +		spin_lock(chip->mutex);
> > +
> > +		ret = get_chip(map, chip, chip->start, FL_SHUTDOWN);
> > +		if (!ret) {
> > +			map_write(map, CMD(0xF0), chip->start);
> > +			chip->state = FL_SHUTDOWN;
> > +			put_chip(map, chip, chip->start);
> > +		}
> > +
> > +		spin_unlock(chip->mutex);
> > +	}
> > +
> > +	return 0;
> > +}
> 
> Kevin, I'd suggest to document why you do this in the code, just for the
> next generation, or for the archaeologists who will dig MTD code in the
> future.
> 

-- 
Fabio Giovagnini

Aurion s.r.l.
P.I e C.F.
00885711200
Tel. +39.051.594.78.24
Cell. +39.335.83.50.919

[PATCHv2 1/2] MTD: cfi_cmdset_0002: Add reboot notifier for AMD flashes

[Kevin Cernekee]

Ensure that the flash device is in a quiescent state before rebooting.
The implementation is closely modeled after the cfe_cmdset_0001 reboot
notifier, commit 963a6fb0a0d336d0513083b7e4b5c3ff9d6d2061 .

Signed-off-by: Kevin Cernekee <cernekee@gmail.com>
---
 drivers/mtd/chips/cfi_cmdset_0002.c |   56 +++++++++++++++++++++++++++++++++++
 1 files changed, 56 insertions(+), 0 deletions(-)

diff --git a/drivers/mtd/chips/cfi_cmdset_0002.c b/drivers/mtd/chips/cfi_cmdset_0002.c
index ea2a7f6..f40d308 100644
--- a/drivers/mtd/chips/cfi_cmdset_0002.c
+++ b/drivers/mtd/chips/cfi_cmdset_0002.c
@@ -32,6 +32,7 @@
 #include <linux/slab.h>
 #include <linux/delay.h>
 #include <linux/interrupt.h>
+#include <linux/reboot.h>
 #include <linux/mtd/compatmac.h>
 #include <linux/mtd/map.h>
 #include <linux/mtd/mtd.h>
@@ -56,6 +57,7 @@ static int cfi_amdstd_erase_varsize(struct mtd_info *, struct erase_info *);
 static void cfi_amdstd_sync (struct mtd_info *);
 static int cfi_amdstd_suspend (struct mtd_info *);
 static void cfi_amdstd_resume (struct mtd_info *);
+static int cfi_amdstd_reboot(struct notifier_block *, unsigned long, void *);
 static int cfi_amdstd_secsi_read (struct mtd_info *, loff_t, size_t, size_t *, u_char *);
 
 static void cfi_amdstd_destroy(struct mtd_info *);
@@ -351,6 +353,8 @@ struct mtd_info *cfi_cmdset_0002(struct map_info *map, int primary)
 	mtd->name    = map->name;
 	mtd->writesize = 1;
 
+	mtd->reboot_notifier.notifier_call = cfi_amdstd_reboot;
+
 	if (cfi->cfi_mode==CFI_MODE_CFI){
 		unsigned char bootloc;
 		/*
@@ -487,6 +491,7 @@ static struct mtd_info *cfi_amdstd_setup(struct mtd_info *mtd)
 #endif
 
 	__module_get(THIS_MODULE);
+	register_reboot_notifier(&mtd->reboot_notifier);
 	return mtd;
 
  setup_err:
@@ -628,6 +633,10 @@ static int get_chip(struct map_info *map, struct flchip *chip, unsigned long adr
 		chip->state = FL_READY;
 		return 0;
 
+	case FL_SHUTDOWN:
+		/* The machine is rebooting */
+		return -EIO;
+
 	case FL_POINT:
 		/* Only if there's no operation suspended... */
 		if (mode == FL_READY && chip->oldstate == FL_READY)
@@ -1918,11 +1927,58 @@ static void cfi_amdstd_resume(struct mtd_info *mtd)
 	}
 }
 
+
+/*
+ * Ensure that the flash device is put back into read array mode before
+ * unloading the driver or rebooting.  On some systems, rebooting while
+ * the flash is in query/program/erase mode will prevent the CPU from
+ * fetching the bootloader code, requiring a hard reset or power cycle.
+ */
+static int cfi_amdstd_reset(struct mtd_info *mtd)
+{
+	struct map_info *map = mtd->priv;
+	struct cfi_private *cfi = map->fldrv_priv;
+	int i, ret;
+	struct flchip *chip;
+
+	for (i = 0; i < cfi->numchips; i++) {
+
+		chip = &cfi->chips[i];
+
+		spin_lock(chip->mutex);
+
+		ret = get_chip(map, chip, chip->start, FL_SHUTDOWN);
+		if (!ret) {
+			map_write(map, CMD(0xF0), chip->start);
+			chip->state = FL_SHUTDOWN;
+			put_chip(map, chip, chip->start);
+		}
+
+		spin_unlock(chip->mutex);
+	}
+
+	return 0;
+}
+
+
+static int cfi_amdstd_reboot(struct notifier_block *nb, unsigned long val,
+			       void *v)
+{
+	struct mtd_info *mtd;
+
+	mtd = container_of(nb, struct mtd_info, reboot_notifier);
+	cfi_amdstd_reset(mtd);
+	return NOTIFY_DONE;
+}
+
+
 static void cfi_amdstd_destroy(struct mtd_info *mtd)
 {
 	struct map_info *map = mtd->priv;
 	struct cfi_private *cfi = map->fldrv_priv;
 
+	cfi_amdstd_reset(mtd);
+	unregister_reboot_notifier(&mtd->reboot_notifier);
 	kfree(cfi->cmdset_priv);
 	kfree(cfi->cfiq);
 	kfree(cfi);
-- 
1.6.3.1

Re: [PATCHv2 1/2] MTD: cfi_cmdset_0002: Add reboot notifier for AMD flashes

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 789 bytes --]

On Wed, Apr 14, 2010 at 05:57:33PM -0700, Kevin Cernekee wrote:
> Ensure that the flash device is in a quiescent state before rebooting.
> The implementation is closely modeled after the cfe_cmdset_0001 reboot
> notifier, commit 963a6fb0a0d336d0513083b7e4b5c3ff9d6d2061 .
> 
> Signed-off-by: Kevin Cernekee <cernekee@gmail.com>

Yes, if this is present for Intel, it should be present for AMD, IMHO.

Acked-by: Wolfram Sang <w.sang@pengutronix.de>

Still, the correct solution is to wire the correct reset-line to the flashes.
Above patch will not help if a watchdog kicks in. Been there :(

Regards,

   Wolfram

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

Re: [PATCHv2 1/2] MTD: cfi_cmdset_0002: Add reboot notifier for AMD flashes

[Jamie Lokier]

Wolfram Sang wrote:
> On Wed, Apr 14, 2010 at 05:57:33PM -0700, Kevin Cernekee wrote:
> > Ensure that the flash device is in a quiescent state before rebooting.
> > The implementation is closely modeled after the cfe_cmdset_0001 reboot
> > notifier, commit 963a6fb0a0d336d0513083b7e4b5c3ff9d6d2061 .
> > 
> > Signed-off-by: Kevin Cernekee <cernekee@gmail.com>
> 
> Yes, if this is present for Intel, it should be present for AMD, IMHO.
> 
> Acked-by: Wolfram Sang <w.sang@pengutronix.de>
> 
> Still, the correct solution is to wire the correct reset-line to the flashes.
> Above patch will not help if a watchdog kicks in. Been there :(

Isn't the patch intended to handle _software_ resets where the hard reset
line isn't toggled?

-- Jamie

Re: [PATCHv2 1/2] MTD: cfi_cmdset_0002: Add reboot notifier for AMD flashes

[Artem Bityutskiy]

On Wed, 2010-04-14 at 17:57 -0700, Kevin Cernekee wrote:
> Ensure that the flash device is in a quiescent state before rebooting.
> The implementation is closely modeled after the cfe_cmdset_0001 reboot
> notifier, commit 963a6fb0a0d336d0513083b7e4b5c3ff9d6d2061 .
> 
> Signed-off-by: Kevin Cernekee <cernekee@gmail.com>
> ---
>  drivers/mtd/chips/cfi_cmdset_0002.c |   56 +++++++++++++++++++++++++++++++++++
>  1 files changed, 56 insertions(+), 0 deletions(-)

This patch conflicts with the patch from Stefani, which is sitting in my
l2-mtd-2.6.git tree
(http://git.infradead.org/users/dedekind/l2-mtd-2.6.git). Could you
please amend it and re-send?

The conflicting patch is:

Author: Stefani Seibold <stefani@seibold.net>
Date:   Sun Apr 18 22:46:44 2010 +0200

    mtd: fix a huge latency problem in the MTD CFI and LPDDR flash drivers.
    
    The use of a memcpy() during a spinlock operation will cause very long
    thread context switch delays if the flash chip bandwidth is low and the
    data to be copied large, because a spinlock will disable preemption.
    
    For example: A flash with 6,5 MB/s bandwidth will cause under ubifs,
    which request sometimes 128 KB (the flash erase size), a preemption delay of
    20 milliseconds. High priority threads will not be served during this
    time, regardless whether this threads access the flash or not. This behavior
    breaks real time.
    
    The patch changes all the use of spin_lock operations for xxxx->mutex
    into mutex operations, which is exact what the name says and means.
    
    I have checked the code of the drivers and there is no use of atomic
    pathes like interrupt or timers. The mtdoops facility will also not be used
    by this drivers. So it is dave to replace the spin_lock against mutex.
    
    There is no performance regression since the mutex is normally not
    acquired.
    
    Changelog:
     06.03.2010 First release
     26.03.2010 Fix mutex[1] issue and tested it for compile failure
    
    Signed-off-by: Stefani Seibold <stefani@seibold.net>
    Signed-off-by: Artem Bityutskiy <Artem.Bityutskiy@nokia.com>


-- 
Best Regards,
Artem Bityutskiy (Артём Битюцкий)

[PATCH 2/2] UBI: Remove reboot notifier

[Kevin Cernekee]

The UBI reboot notifier causes problems with hibernation.  Move this
functionality into the low-level MTD driver instead.

Signed-off-by: Kevin Cernekee <cernekee@gmail.com>
---
 drivers/mtd/ubi/build.c |   35 -----------------------------------
 drivers/mtd/ubi/ubi.h   |    2 --
 2 files changed, 0 insertions(+), 37 deletions(-)

diff --git a/drivers/mtd/ubi/build.c b/drivers/mtd/ubi/build.c
index fad40aa..a6a5d1a 100644
--- a/drivers/mtd/ubi/build.c
+++ b/drivers/mtd/ubi/build.c
@@ -42,7 +42,6 @@
 #include <linux/miscdevice.h>
 #include <linux/log2.h>
 #include <linux/kthread.h>
-#include <linux/reboot.h>
 #include <linux/kernel.h>
 #include "ubi.h"
 
@@ -831,34 +830,6 @@ static int autoresize(struct ubi_device *ubi, int vol_id)
 }
 
 /**
- * ubi_reboot_notifier - halt UBI transactions immediately prior to a reboot.
- * @n: reboot notifier object
- * @state: SYS_RESTART, SYS_HALT, or SYS_POWER_OFF
- * @cmd: pointer to command string for RESTART2
- *
- * This function stops the UBI background thread so that the flash device
- * remains quiescent when Linux restarts the system. Any queued work will be
- * discarded, but this function will block until do_work() finishes if an
- * operation is already in progress.
- *
- * This function solves a real-life problem observed on NOR flashes when an
- * PEB erase operation starts, then the system is rebooted before the erase is
- * finishes, and the boot loader gets confused and dies. So we prefer to finish
- * the ongoing operation before rebooting.
- */
-static int ubi_reboot_notifier(struct notifier_block *n, unsigned long state,
-			       void *cmd)
-{
-	struct ubi_device *ubi;
-
-	ubi = container_of(n, struct ubi_device, reboot_notifier);
-	if (ubi->bgt_thread)
-		kthread_stop(ubi->bgt_thread);
-	ubi_sync(ubi->ubi_num);
-	return NOTIFY_DONE;
-}
-
-/**
  * ubi_attach_mtd_dev - attach an MTD device.
  * @mtd: MTD device description object
  * @ubi_num: number to assign to the new UBI device
@@ -1015,11 +986,6 @@ int ubi_attach_mtd_dev(struct mtd_info *mtd, int ubi_num, int vid_hdr_offset)
 	wake_up_process(ubi->bgt_thread);
 	spin_unlock(&ubi->wl_lock);
 
-	/* Flash device priority is 0 - UBI needs to shut down first */
-	ubi->reboot_notifier.priority = 1;
-	ubi->reboot_notifier.notifier_call = ubi_reboot_notifier;
-	register_reboot_notifier(&ubi->reboot_notifier);
-
 	ubi_devices[ubi_num] = ubi;
 	ubi_notify_all(ubi, UBI_VOLUME_ADDED, NULL);
 	return ubi_num;
@@ -1090,7 +1056,6 @@ int ubi_detach_mtd_dev(int ubi_num, int anyway)
 	 * Before freeing anything, we have to stop the background thread to
 	 * prevent it from doing anything on this device while we are freeing.
 	 */
-	unregister_reboot_notifier(&ubi->reboot_notifier);
 	if (ubi->bgt_thread)
 		kthread_stop(ubi->bgt_thread);
 
diff --git a/drivers/mtd/ubi/ubi.h b/drivers/mtd/ubi/ubi.h
index 1af0817..797c2cb 100644
--- a/drivers/mtd/ubi/ubi.h
+++ b/drivers/mtd/ubi/ubi.h
@@ -349,7 +349,6 @@ struct ubi_wl_entry;
  * @bgt_thread: background thread description object
  * @thread_enabled: if the background thread is enabled
  * @bgt_name: background thread name
- * @reboot_notifier: notifier to terminate background thread before rebooting
  *
  * @flash_size: underlying MTD device size (in bytes)
  * @peb_count: count of physical eraseblocks on the MTD device
@@ -435,7 +434,6 @@ struct ubi_device {
 	struct task_struct *bgt_thread;
 	int thread_enabled;
 	char bgt_name[sizeof(UBI_BGT_NAME_PATTERN)+2];
-	struct notifier_block reboot_notifier;
 
 	/* I/O sub-system's stuff */
 	long long flash_size;
-- 
1.6.3.1

Re: [PATCH 2/2] UBI: Remove reboot notifier

[Artem Bityutskiy]

On Tue, 2010-04-13 at 13:30 -0700, Kevin Cernekee wrote:
> The UBI reboot notifier causes problems with hibernation.  Move this
> functionality into the low-level MTD driver instead.
> 
> Signed-off-by: Kevin Cernekee <cernekee@gmail.com>
> ---
>  drivers/mtd/ubi/build.c |   35 -----------------------------------
>  drivers/mtd/ubi/ubi.h   |    2 --
>  2 files changed, 0 insertions(+), 37 deletions(-)
> 

Pushed this patch to the ubi-2.6.git tree, thanks.

-- 
Best Regards,
Artem Bityutskiy (Артём Битюцкий)