Re: [PATCH] MTD: Add UBI reboot notifier

[Artem Bityutskiy]

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?

-- 
Best regards,
Artem Bityutskiy (ÐÐÑÑÑÐÐÐ ÐÑÑÑÐ)

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/