| Message ID | f5be75d5abc1acd236bedf36c55a860e@localhost |
|---|---|
| State | Accepted, archived |
| Headers | show |
On Tue, 2009-06-09 at 10:59 -0700, Kevin Cernekee wrote: > Terminate the UBI background thread prior to restarting the system. > > Changes from v1: > > Add ubi_sync() at the end, to make sure the underlying MTD device has > synchronized its buffers. > > Clean up comments and formatting. > > Signed-off-by: Kevin Cernekee <kpc.mtd@gmail.com> I've pushed this patch to ubi-2.6.git/master. I've tested it even. And I amended comments a little. Please, check it: http://git.infradead.org/ubi-2.6.git?a=commit;h=79c293b2629882a1db3a850ed6b84bd1d8a464a7 If you are fine with this, I'll put it to ubi-2.6.git/linux-next and merge it this merge window.
On Wed, 2009-06-10 at 12:35 +0300, Artem Bityutskiy wrote: > On Tue, 2009-06-09 at 10:59 -0700, Kevin Cernekee wrote: > > Terminate the UBI background thread prior to restarting the system. > > > > Changes from v1: > > > > Add ubi_sync() at the end, to make sure the underlying MTD device has > > synchronized its buffers. > > > > Clean up comments and formatting. > > > > Signed-off-by: Kevin Cernekee <kpc.mtd@gmail.com> > > I've pushed this patch to ubi-2.6.git/master. I've tested it > even. And I amended comments a little. Please, check it: > > http://git.infradead.org/ubi-2.6.git?a=commit;h=79c293b2629882a1db3a850ed6b84bd1d8a464a7 > > If you are fine with this, I'll put it to ubi-2.6.git/linux-next > and merge it this merge window. Ok, I've actually pushed this to ubi-2.6.git/linux-next, I just assume you should not have issues with my tweaks.
On Wed, Jun 10, 2009 at 2:35 AM, Artem Bityutskiy<dedekind@infradead.org> wrote: > I've pushed this patch to ubi-2.6.git/master. I've tested it > even. And I amended comments a little. Please, check it: > > http://git.infradead.org/ubi-2.6.git?a=commit;h=79c293b2629882a1db3a850ed6b84bd1d8a464a7 > > If you are fine with this, I'll put it to ubi-2.6.git/linux-next > and merge it this merge window. Looks good to me. Thanks.
FYI, I do not like this patch, because it is not solving the problem, it just makes the problem less likely. I.e., you are still screwed if you reboot while some other task is doing erasure. Indeed, the background thread is not the only task which may do erases. I'm not going to revert this patch, but wanted to let you know that I think it was my mistake to push it. On Tue, 2009-06-09 at 10:59 -0700, Kevin Cernekee wrote: > Terminate the UBI background thread prior to restarting the system. > > Changes from v1: > > Add ubi_sync() at the end, to make sure the underlying MTD device has > synchronized its buffers. > > Clean up comments and formatting. > > Signed-off-by: Kevin Cernekee <kpc.mtd@gmail.com> > --- > drivers/mtd/ubi/build.c | 30 ++++++++++++++++++++++++++++++ > drivers/mtd/ubi/ubi.h | 3 +++ > 2 files changed, 33 insertions(+), 0 deletions(-) > > diff --git a/drivers/mtd/ubi/build.c b/drivers/mtd/ubi/build.c > index 4048db8..18b42c9 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,29 @@ 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. > + */ > +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 > @@ -876,6 +900,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 +974,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..c8fab4a 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> > > @@ -339,6 +340,7 @@ 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 > @@ -420,6 +422,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;
Artem Bityutskiy wrote: > FYI, I do not like this patch, because it is not solving the problem, it > just makes the problem less likely. I.e., you are still screwed if you > reboot while some other task is doing erasure. Indeed, the background > thread is not the only task which may do erases. > > I'm not going to revert this patch, but wanted to let you know that I > think it was my mistake to push it. > so, how to solve this properly ? probably the only way is the reboot notifier of the MTD chip driver (for NOR flash only cmdset_0001 has one).
On Fri, 12 Feb 2010, Norbert van Bolhuis wrote:
> so, how to solve this properly ?
In hardware.
Anything we do in software will be unreliable, and by masking the problem
we do a disservice to anyone who actually tests their prototype hardware,
because we might hide the problem from them until it's too late for
them to fix it.
On Fri, 2010-02-12 at 14:45 +0100, Norbert van Bolhuis wrote: > Artem Bityutskiy wrote: > > FYI, I do not like this patch, because it is not solving the problem, it > > just makes the problem less likely. I.e., you are still screwed if you > > reboot while some other task is doing erasure. Indeed, the background > > thread is not the only task which may do erases. > > > > I'm not going to revert this patch, but wanted to let you know that I > > think it was my mistake to push it. > > > > so, how to solve this properly ? > > probably the only way is the reboot notifier of the MTD chip driver > (for NOR flash only cmdset_0001 has one). Well. UBI itself does not have problems with half-erased eraseblocks. As the comment to the patch says, this is for a strange platform where the boot loader panics when it encounters a half-erased EB. So the most proper fix is to fix the boot-loader. Remember, a power cut can introduce half-erased EBs, and you cannot address this in SW. For clean reboots, you would need to have a reboot notifier in the MTD lever, not in UBI. This notifier should: 1. Finish the current erase operation. 2. Prevent further erase operations. The UBI patch is not a good solution.
diff --git a/drivers/mtd/ubi/build.c b/drivers/mtd/ubi/build.c index 4048db8..18b42c9 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,29 @@ 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. + */ +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 @@ -876,6 +900,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 +974,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..c8fab4a 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> @@ -339,6 +340,7 @@ 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 @@ -420,6 +422,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;
Terminate the UBI background thread prior to restarting the system. Changes from v1: Add ubi_sync() at the end, to make sure the underlying MTD device has synchronized its buffers. Clean up comments and formatting. Signed-off-by: Kevin Cernekee <kpc.mtd@gmail.com> --- drivers/mtd/ubi/build.c | 30 ++++++++++++++++++++++++++++++ drivers/mtd/ubi/ubi.h | 3 +++ 2 files changed, 33 insertions(+), 0 deletions(-)