Re: [PATCH] mtd: nand: added nand_shutdown

[Scott Branden]

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 <computersforpeace@xxxxxxxxx>
>
> 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 <computersforpeace@xxxxxxxxx>
> ---
>   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.

--
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/