Patchwork MTD: Add UBI reboot notifier

login
register
mail settings
Submitter Kevin Cernekee
Date June 9, 2009, 5:14 a.m.
Message ID <018abcd2d98090bda0f75d3c95c9ec85@localhost>
Download mbox | patch
Permalink /patch/28269/
State New
Headers show

Comments

Kevin Cernekee - June 9, 2009, 5:14 a.m.
ubifs_sync_fs() may queue up a new UBI erase transaction, which is
processed in the background:

bash# sync
ubifs_sync_fs: enter
schedule_erase: enter
schedule_erase: exit
ubi_sync: enter
ubi_sync: exit
ubifs_sync_fs: exit
cfi_amdstd_erase_varsize: enter
bash# cfi_amdstd_erase_varsize: exit

Normally this is not a big deal.  However, during the final sync before
rebooting, it initiates an erase operation that is potentially still
active when Linux restarts the machine:

bash# reboot -f
ubifs_sync_fs: enter
schedule_erase: enter
schedule_erase: exit
ubi_sync: enter
ubi_sync: exit
ubifs_sync_fs: exit
cfi_amdstd_erase_varsize: enter
Restarting system.
<Flash is stuck in FL_ERASE mode - system hangs>

This is easiest to observe on a NOR flash.  One factor is the long erase
time.  The other reason is because getting a NOR flash stuck in FL_ERASE
mode will prevent the bootloader from running, unless the board provides
a way for the processor to automatically reset the flash.  In my
experience, many boards do not.

My proposal is to add a reboot notifier to let the UBI background thread
terminate gracefully.  The new ordering looks like this:

bash# reboot -f
ubifs_sync_fs: enter
schedule_erase: enter
schedule_erase: exit
ubifs_sync_fs: exit
cfi_amdstd_erase_varsize: enter
ubi_reboot_notifier: enter
cfi_amdstd_erase_varsize: exit
ubi_reboot_notifier: exit
cfi_amdstd_reboot: enter
cfi_amdstd_reboot: exit

cfi_amdstd_reboot doesn't really exist, but I added a dummy notifier to
make sure that the ordering would be correct when using drivers that do
have this feature.

Signed-off-by: Kevin Cernekee <kpc.mtd@gmail.com>
---
 drivers/mtd/ubi/build.c |   24 ++++++++++++++++++++++++
 drivers/mtd/ubi/ubi.h   |    2 ++
 2 files changed, 26 insertions(+), 0 deletions(-)
Artem Bityutskiy - June 9, 2009, 12:13 p.m.
On Mon, 2009-06-08 at 22:14 -0700, Kevin Cernekee wrote:
> ubifs_sync_fs() may queue up a new UBI erase transaction, which is
> processed in the background:
> 
> bash# sync
> ubifs_sync_fs: enter
> schedule_erase: enter
> schedule_erase: exit
> ubi_sync: enter
> ubi_sync: exit
> ubifs_sync_fs: exit
> cfi_amdstd_erase_varsize: enter
> bash# cfi_amdstd_erase_varsize: exit
> 
> Normally this is not a big deal.  However, during the final sync before
> rebooting, it initiates an erase operation that is potentially still
> active when Linux restarts the machine:
> 
> bash# reboot -f
> ubifs_sync_fs: enter
> schedule_erase: enter
> schedule_erase: exit
> ubi_sync: enter
> ubi_sync: exit
> ubifs_sync_fs: exit
> cfi_amdstd_erase_varsize: enter
> Restarting system.
> <Flash is stuck in FL_ERASE mode - system hangs>
> 
> This is easiest to observe on a NOR flash.  One factor is the long erase
> time.  The other reason is because getting a NOR flash stuck in FL_ERASE
> mode will prevent the bootloader from running, unless the board provides
> a way for the processor to automatically reset the flash.  In my
> experience, many boards do not.
> 
> My proposal is to add a reboot notifier to let the UBI background thread
> terminate gracefully.  The new ordering looks like this:
> 
> bash# reboot -f
> ubifs_sync_fs: enter
> schedule_erase: enter
> schedule_erase: exit
> ubifs_sync_fs: exit
> cfi_amdstd_erase_varsize: enter
> ubi_reboot_notifier: enter
> cfi_amdstd_erase_varsize: exit
> ubi_reboot_notifier: exit
> cfi_amdstd_reboot: enter
> cfi_amdstd_reboot: exit
> 
> cfi_amdstd_reboot doesn't really exist, but I added a dummy notifier to
> make sure that the ordering would be correct when using drivers that do
> have this feature.

The idea looks OK. So you basically make sure that if UBI bgt thread
started an erase operation, it finishes it (and only it) before we
reboot. Do I read the patch correctly?
Artem Bityutskiy - June 9, 2009, 12:32 p.m.
Looks good to me. I'm though wandering if UBIFS should also
register a reboot notifier an sync write-buffers in it...
But this is not related to your patch.

Some nit-picking.

On Mon, 2009-06-08 at 22:14 -0700, Kevin Cernekee wrote:
>  /**
> + * ubi_reboot_notifier - halt UBI transactions immediately prior to a reboot
> + * @n: notifier_block struct (inside our struct ubi_device)
> + * @val: unused
> + * @v: unused
> + */
> +static int ubi_reboot_notifier(struct notifier_block *n, unsigned long val,
> +	void *v)
> +{
> +	struct ubi_device *ubi = container_of(n, struct ubi_device,
> +		reboot_notifier);
> +
> +	if (ubi->bgt_thread)
> +		kthread_stop(ubi->bgt_thread);
> +	return NOTIFY_DONE;
> +}

Several small things in comments and indentations. Take a look how
other UBI funcs do things, I'd be happier to keep the code consistent.

> +/**
>   * ubi_attach_mtd_dev - attach an MTD device.
>   * @mtd: MTD device description object
>   * @ubi_num: number to assign to the new UBI device
> @@ -876,6 +894,11 @@ int ubi_attach_mtd_dev(struct mtd_info *mtd, int ubi_num, int vid_hdr_offset)
>  		ubi->thread_enabled = 1;
>  	wake_up_process(ubi->bgt_thread);
>  
> +	/* 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;
>  	return ubi_num;
>  
> @@ -945,6 +968,7 @@ 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 c055511..44a45b9 100644
> --- a/drivers/mtd/ubi/ubi.h
> +++ b/drivers/mtd/ubi/ubi.h
> @@ -36,6 +36,7 @@
>  #include <linux/device.h>
>  #include <linux/string.h>
>  #include <linux/vmalloc.h>
> +#include <linux/notifier.h>
>  #include <linux/mtd/mtd.h>
>  #include <linux/mtd/ubi.h>
>  
> @@ -420,6 +421,7 @@ 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;

How about commenting this field above the struct ubi_device
definition?

Patch

diff --git a/drivers/mtd/ubi/build.c b/drivers/mtd/ubi/build.c
index 4048db8..ca88059 100644
--- a/drivers/mtd/ubi/build.c
+++ b/drivers/mtd/ubi/build.c
@@ -41,6 +41,7 @@ 
 #include <linux/miscdevice.h>
 #include <linux/log2.h>
 #include <linux/kthread.h>
+#include <linux/reboot.h>
 #include "ubi.h"
 
 /* Maximum length of the 'mtd=' parameter */
@@ -726,6 +727,23 @@  static int autoresize(struct ubi_device *ubi, int vol_id)
 }
 
 /**
+ * ubi_reboot_notifier - halt UBI transactions immediately prior to a reboot
+ * @n: notifier_block struct (inside our struct ubi_device)
+ * @val: unused
+ * @v: unused
+ */
+static int ubi_reboot_notifier(struct notifier_block *n, unsigned long val,
+	void *v)
+{
+	struct ubi_device *ubi = container_of(n, struct ubi_device,
+		reboot_notifier);
+
+	if (ubi->bgt_thread)
+		kthread_stop(ubi->bgt_thread);
+	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
@@ -876,6 +894,11 @@  int ubi_attach_mtd_dev(struct mtd_info *mtd, int ubi_num, int vid_hdr_offset)
 		ubi->thread_enabled = 1;
 	wake_up_process(ubi->bgt_thread);
 
+	/* 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;
 	return ubi_num;
 
@@ -945,6 +968,7 @@  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 c055511..44a45b9 100644
--- a/drivers/mtd/ubi/ubi.h
+++ b/drivers/mtd/ubi/ubi.h
@@ -36,6 +36,7 @@ 
 #include <linux/device.h>
 #include <linux/string.h>
 #include <linux/vmalloc.h>
+#include <linux/notifier.h>
 #include <linux/mtd/mtd.h>
 #include <linux/mtd/ubi.h>
 
@@ -420,6 +421,7 @@  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;