[PATCH 1/3] mtd: implement common reboot notifier boilerplate

[PATCH 1/3] mtd: implement common reboot notifier boilerplate

[Brian Norris]

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@gmail.com>
---
 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.
-- 
1.9.1

[PATCH 2/3] mtd: nand: added nand_shutdown

[Brian Norris]

From: Scott Branden <sbranden@broadcom.com>

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.

Signed-off-by: Scott Branden <sbranden@broadcom.com>
Signed-off-by: Brian Norris <computersforpeace@gmail.com>
---
Modified to use the new mtd->_reboot hook

Not tested yet

 drivers/mtd/nand/nand_base.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
index 41585dfb206f..382354b5547b 100644
--- a/drivers/mtd/nand/nand_base.c
+++ b/drivers/mtd/nand/nand_base.c
@@ -2944,6 +2944,16 @@ static void nand_resume(struct mtd_info *mtd)
 			__func__);
 }
 
+/**
+ * nand_shutdown - [MTD Interface] Finish the current NAND operation and
+ *                 prevent further operations
+ * @mtd: MTD device structure
+ */
+static void nand_shutdown(struct mtd_info *mtd)
+{
+	nand_get_device(mtd, FL_SHUTDOWN);
+}
+
 /* Set default functions */
 static void nand_set_defaults(struct nand_chip *chip, int busw)
 {
@@ -4146,6 +4156,7 @@ int nand_scan_tail(struct mtd_info *mtd)
 	mtd->_unlock = NULL;
 	mtd->_suspend = nand_suspend;
 	mtd->_resume = nand_resume;
+	mtd->_reboot = nand_shutdown;
 	mtd->_block_isreserved = nand_block_isreserved;
 	mtd->_block_isbad = nand_block_isbad;
 	mtd->_block_markbad = nand_block_markbad;
-- 
1.9.1

[PATCH 3/3] mtd: cfi_cmdset_{0001, 0002}: use common MTD reboot boilerplate

[Brian Norris]

We don't have to implement this glue code in the chip driver any more.

Signed-off-by: Brian Norris <computersforpeace@gmail.com>
---
Not tested yet

 drivers/mtd/chips/cfi_cmdset_0001.c | 22 +++-------------------
 drivers/mtd/chips/cfi_cmdset_0002.c | 22 +++-------------------
 2 files changed, 6 insertions(+), 38 deletions(-)

diff --git a/drivers/mtd/chips/cfi_cmdset_0001.c b/drivers/mtd/chips/cfi_cmdset_0001.c
index 286b97a304cf..3df9744496b2 100644
--- a/drivers/mtd/chips/cfi_cmdset_0001.c
+++ b/drivers/mtd/chips/cfi_cmdset_0001.c
@@ -28,7 +28,6 @@
 #include <linux/slab.h>
 #include <linux/delay.h>
 #include <linux/interrupt.h>
-#include <linux/reboot.h>
 #include <linux/bitmap.h>
 #include <linux/mtd/xip.h>
 #include <linux/mtd/map.h>
@@ -80,7 +79,7 @@ static int cfi_intelext_get_user_prot_info(struct mtd_info *, size_t,
 #endif
 static int cfi_intelext_suspend (struct mtd_info *);
 static void cfi_intelext_resume (struct mtd_info *);
-static int cfi_intelext_reboot (struct notifier_block *, unsigned long, void *);
+static void cfi_intelext_reset(struct mtd_info *);
 
 static void cfi_intelext_destroy(struct mtd_info *);
 
@@ -486,13 +485,12 @@ struct mtd_info *cfi_cmdset_0001(struct map_info *map, int primary)
 	mtd->_is_locked = cfi_intelext_is_locked;
 	mtd->_suspend = cfi_intelext_suspend;
 	mtd->_resume  = cfi_intelext_resume;
+	mtd->_reboot  = cfi_intelext_reset;
 	mtd->flags   = MTD_CAP_NORFLASH;
 	mtd->name    = map->name;
 	mtd->writesize = 1;
 	mtd->writebufsize = cfi_interleave(cfi) << cfi->cfiq->MaxBufWriteSize;
 
-	mtd->reboot_notifier.notifier_call = cfi_intelext_reboot;
-
 	if (cfi->cfi_mode == CFI_MODE_CFI) {
 		/*
 		 * It's a real CFI chip, not one for which the probe
@@ -646,7 +644,6 @@ static struct mtd_info *cfi_intelext_setup(struct mtd_info *mtd)
 		goto setup_err;
 
 	__module_get(THIS_MODULE);
-	register_reboot_notifier(&mtd->reboot_notifier);
 	return mtd;
 
  setup_err:
@@ -2605,7 +2602,7 @@ static void cfi_intelext_resume(struct mtd_info *mtd)
 		cfi_intelext_restore_locks(mtd);
 }
 
-static int cfi_intelext_reset(struct mtd_info *mtd)
+static void cfi_intelext_reset(struct mtd_info *mtd)
 {
 	struct map_info *map = mtd->priv;
 	struct cfi_private *cfi = map->fldrv_priv;
@@ -2626,18 +2623,6 @@ static int cfi_intelext_reset(struct mtd_info *mtd)
 		}
 		mutex_unlock(&chip->mutex);
 	}
-
-	return 0;
-}
-
-static int cfi_intelext_reboot(struct notifier_block *nb, unsigned long val,
-			       void *v)
-{
-	struct mtd_info *mtd;
-
-	mtd = container_of(nb, struct mtd_info, reboot_notifier);
-	cfi_intelext_reset(mtd);
-	return NOTIFY_DONE;
 }
 
 static void cfi_intelext_destroy(struct mtd_info *mtd)
@@ -2647,7 +2632,6 @@ static void cfi_intelext_destroy(struct mtd_info *mtd)
 	struct mtd_erase_region_info *region;
 	int i;
 	cfi_intelext_reset(mtd);
-	unregister_reboot_notifier(&mtd->reboot_notifier);
 	kfree(cfi->cmdset_priv);
 	kfree(cfi->cfiq);
 	kfree(cfi->chips[0].priv);
diff --git a/drivers/mtd/chips/cfi_cmdset_0002.c b/drivers/mtd/chips/cfi_cmdset_0002.c
index c50d8cf0f60d..c4f63482cf96 100644
--- a/drivers/mtd/chips/cfi_cmdset_0002.c
+++ b/drivers/mtd/chips/cfi_cmdset_0002.c
@@ -31,7 +31,6 @@
 #include <linux/slab.h>
 #include <linux/delay.h>
 #include <linux/interrupt.h>
-#include <linux/reboot.h>
 #include <linux/of.h>
 #include <linux/of_platform.h>
 #include <linux/mtd/map.h>
@@ -57,7 +56,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 void cfi_amdstd_reset(struct mtd_info *);
 static int cfi_amdstd_get_fact_prot_info(struct mtd_info *, size_t,
 					 size_t *, struct otp_info *);
 static int cfi_amdstd_get_user_prot_info(struct mtd_info *, size_t,
@@ -529,6 +528,7 @@ struct mtd_info *cfi_cmdset_0002(struct map_info *map, int primary)
 	mtd->_sync    = cfi_amdstd_sync;
 	mtd->_suspend = cfi_amdstd_suspend;
 	mtd->_resume  = cfi_amdstd_resume;
+	mtd->_reboot  = cfi_amdstd_reset;
 	mtd->_read_user_prot_reg = cfi_amdstd_read_user_prot_reg;
 	mtd->_read_fact_prot_reg = cfi_amdstd_read_fact_prot_reg;
 	mtd->_get_fact_prot_info = cfi_amdstd_get_fact_prot_info;
@@ -544,7 +544,6 @@ struct mtd_info *cfi_cmdset_0002(struct map_info *map, int primary)
 			mtd->writebufsize);
 
 	mtd->_panic_write = cfi_amdstd_panic_write;
-	mtd->reboot_notifier.notifier_call = cfi_amdstd_reboot;
 
 	if (cfi->cfi_mode==CFI_MODE_CFI){
 		unsigned char bootloc;
@@ -717,7 +716,6 @@ static struct mtd_info *cfi_amdstd_setup(struct mtd_info *mtd)
 	}
 
 	__module_get(THIS_MODULE);
-	register_reboot_notifier(&mtd->reboot_notifier);
 	return mtd;
 
  setup_err:
@@ -2871,7 +2869,7 @@ static void cfi_amdstd_resume(struct mtd_info *mtd)
  * 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)
+static void cfi_amdstd_reset(struct mtd_info *mtd)
 {
 	struct map_info *map = mtd->priv;
 	struct cfi_private *cfi = map->fldrv_priv;
@@ -2893,19 +2891,6 @@ static int cfi_amdstd_reset(struct mtd_info *mtd)
 
 		mutex_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;
 }
 
 
@@ -2915,7 +2900,6 @@ static void cfi_amdstd_destroy(struct mtd_info *mtd)
 	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.9.1

Re: [PATCH 1/3] mtd: implement common reboot notifier boilerplate

[Scott Branden]

You need to change some spaces to tabs in the patch.

Tested-by: Scott Branden <sbranden@broadcom.com>

On 14-12-04 06:36 PM, Brian Norris wrote:
> 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@gmail.com>
> ---
>   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;
You need tabs here instead of spaces to pass checkpatch
> +
> +       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);
> +	}
You need tabs here instead of spaces to pass checkpatch
> +
>   	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.
>

Re: [PATCH 2/3] mtd: nand: added nand_shutdown

[Scott Branden]

On 14-12-04 06:36 PM, Brian Norris wrote:
> From: Scott Branden <sbranden@broadcom.com>
>
> 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.
>
> Signed-off-by: Scott Branden <sbranden@broadcom.com>
> Signed-off-by: Brian Norris <computersforpeace@gmail.com>
> ---
> Modified to use the new mtd->_reboot hook
>
> Not tested yet
Tested by: Scott Branden <sbranden@broadcom.com>
>
>   drivers/mtd/nand/nand_base.c | 11 +++++++++++
>   1 file changed, 11 insertions(+)
>
> diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
> index 41585dfb206f..382354b5547b 100644
> --- a/drivers/mtd/nand/nand_base.c
> +++ b/drivers/mtd/nand/nand_base.c
> @@ -2944,6 +2944,16 @@ static void nand_resume(struct mtd_info *mtd)
>   			__func__);
>   }
>
> +/**
> + * nand_shutdown - [MTD Interface] Finish the current NAND operation and
> + *                 prevent further operations
> + * @mtd: MTD device structure
> + */
> +static void nand_shutdown(struct mtd_info *mtd)
> +{
> +	nand_get_device(mtd, FL_SHUTDOWN);
> +}
> +
>   /* Set default functions */
>   static void nand_set_defaults(struct nand_chip *chip, int busw)
>   {
> @@ -4146,6 +4156,7 @@ int nand_scan_tail(struct mtd_info *mtd)
>   	mtd->_unlock = NULL;
>   	mtd->_suspend = nand_suspend;
>   	mtd->_resume = nand_resume;
> +	mtd->_reboot = nand_shutdown;
>   	mtd->_block_isreserved = nand_block_isreserved;
>   	mtd->_block_isbad = nand_block_isbad;
>   	mtd->_block_markbad = nand_block_markbad;
>

Re: [PATCH 1/3] mtd: implement common reboot notifier boilerplate

[Brian Norris]

On Thu, Dec 04, 2014 at 10:48:02PM -0800, Scott Branden wrote:
> You need to change some spaces to tabs in the patch.

Wow, sorry. I'm being very dense. I'll be sure to fix these up before
applying.

> Tested-by: Scott Branden <sbranden@broadcom.com>

Thanks! I'll leave this for others to comment on, since this was
seemingly controversial. Plus, I have plenty of other things to review
for (maybe) 3.19 that have been around a lot longer than this, so these
may delay until 3.20, given the nearness of the 3.19 merge window.

Regards,
Brian

Re: [PATCH 1/3] mtd: implement common reboot notifier boilerplate

[Scott Branden]

Hi Brian,

I guess I'll leave this in your hands now unless there's anything else 
you need help will.

I don't see how this is controversial.  It is simply adding a reboot 
notifier so the MTD devices are shut down gracefully.  You already added 
it to NOR.  We're now just adding it generically for NAND and NOR.

Thanks,
  Scott

On 14-12-05 10:07 AM, Brian Norris wrote:
> On Thu, Dec 04, 2014 at 10:48:02PM -0800, Scott Branden wrote:
>> You need to change some spaces to tabs in the patch.
>
> Wow, sorry. I'm being very dense. I'll be sure to fix these up before
> applying.
>
>> Tested-by: Scott Branden <sbranden@broadcom.com>
>
> Thanks! I'll leave this for others to comment on, since this was
> seemingly controversial. Plus, I have plenty of other things to review
> for (maybe) 3.19 that have been around a lot longer than this, so these
> may delay until 3.20, given the nearness of the 3.19 merge window.
>
> Regards,
> Brian
>

Re: [PATCH 1/3] mtd: implement common reboot notifier boilerplate

[Richard Weinberger]

Am 10.12.2014 um 01:02 schrieb Scott Branden:
> Hi Brian,
> 
> I guess I'll leave this in your hands now unless there's anything else you need help will.
> 
> I don't see how this is controversial.  It is simply adding a reboot notifier so the MTD devices are shut down gracefully.  You already added it to NOR.  We're now just adding it
> generically for NAND and NOR.

FWIW, I'll not block this. If it can be done in a sane way in MTD core and the MTD maintainers are fine with it I'm fine too. :-)
My concern was that David was against this a few yeas ago and I tend to agree with him.

Thanks,
//richard

Re: [PATCH 1/3] mtd: implement common reboot notifier boilerplate

[Brian Norris]

On Thu, Dec 04, 2014 at 06:36:06PM -0800, Brian Norris wrote:
> 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@gmail.com>

Pushed the first 2 (with a whitespace fixup) to l2-mtd.git. The third
patch could use some testing, but I don't have an applicable system on
me at the moment.

Brian

Re: [PATCH 1/3] mtd: implement common reboot notifier boilerplate

[Scott Branden]

Thanks Brian.  We only use NAND right now so don't have system to test 
NOR either.

Regards,
  Scott

On 15-01-07 05:58 PM, Brian Norris wrote:
> On Thu, Dec 04, 2014 at 06:36:06PM -0800, Brian Norris wrote:
>> 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@gmail.com>
>
> Pushed the first 2 (with a whitespace fixup) to l2-mtd.git. The third
> patch could use some testing, but I don't have an applicable system on
> me at the moment.
>
> Brian
>

Re: [3/3] mtd: cfi_cmdset_{0001, 0002}: use common MTD reboot boilerplate

[Guenter Roeck]

On Thu, Dec 04, 2014 at 06:36:08PM -0800, Brian Norris wrote:
> We don't have to implement this glue code in the chip driver any more.
> 
> Signed-off-by: Brian Norris <computersforpeace@gmail.com>
> ---
> Not tested yet
> 
Tested-by: Guenter Roeck <linux@roeck-us.net>

>  drivers/mtd/chips/cfi_cmdset_0001.c | 22 +++-------------------
>  drivers/mtd/chips/cfi_cmdset_0002.c | 22 +++-------------------
>  2 files changed, 6 insertions(+), 38 deletions(-)
> 
> diff --git a/drivers/mtd/chips/cfi_cmdset_0001.c b/drivers/mtd/chips/cfi_cmdset_0001.c
> index 286b97a304cf..3df9744496b2 100644
> --- a/drivers/mtd/chips/cfi_cmdset_0001.c
> +++ b/drivers/mtd/chips/cfi_cmdset_0001.c
> @@ -28,7 +28,6 @@
>  #include <linux/slab.h>
>  #include <linux/delay.h>
>  #include <linux/interrupt.h>
> -#include <linux/reboot.h>
>  #include <linux/bitmap.h>
>  #include <linux/mtd/xip.h>
>  #include <linux/mtd/map.h>
> @@ -80,7 +79,7 @@ static int cfi_intelext_get_user_prot_info(struct mtd_info *, size_t,
>  #endif
>  static int cfi_intelext_suspend (struct mtd_info *);
>  static void cfi_intelext_resume (struct mtd_info *);
> -static int cfi_intelext_reboot (struct notifier_block *, unsigned long, void *);
> +static void cfi_intelext_reset(struct mtd_info *);
>  
>  static void cfi_intelext_destroy(struct mtd_info *);
>  
> @@ -486,13 +485,12 @@ struct mtd_info *cfi_cmdset_0001(struct map_info *map, int primary)
>  	mtd->_is_locked = cfi_intelext_is_locked;
>  	mtd->_suspend = cfi_intelext_suspend;
>  	mtd->_resume  = cfi_intelext_resume;
> +	mtd->_reboot  = cfi_intelext_reset;
>  	mtd->flags   = MTD_CAP_NORFLASH;
>  	mtd->name    = map->name;
>  	mtd->writesize = 1;
>  	mtd->writebufsize = cfi_interleave(cfi) << cfi->cfiq->MaxBufWriteSize;
>  
> -	mtd->reboot_notifier.notifier_call = cfi_intelext_reboot;
> -
>  	if (cfi->cfi_mode == CFI_MODE_CFI) {
>  		/*
>  		 * It's a real CFI chip, not one for which the probe
> @@ -646,7 +644,6 @@ static struct mtd_info *cfi_intelext_setup(struct mtd_info *mtd)
>  		goto setup_err;
>  
>  	__module_get(THIS_MODULE);
> -	register_reboot_notifier(&mtd->reboot_notifier);
>  	return mtd;
>  
>   setup_err:
> @@ -2605,7 +2602,7 @@ static void cfi_intelext_resume(struct mtd_info *mtd)
>  		cfi_intelext_restore_locks(mtd);
>  }
>  
> -static int cfi_intelext_reset(struct mtd_info *mtd)
> +static void cfi_intelext_reset(struct mtd_info *mtd)
>  {
>  	struct map_info *map = mtd->priv;
>  	struct cfi_private *cfi = map->fldrv_priv;
> @@ -2626,18 +2623,6 @@ static int cfi_intelext_reset(struct mtd_info *mtd)
>  		}
>  		mutex_unlock(&chip->mutex);
>  	}
> -
> -	return 0;
> -}
> -
> -static int cfi_intelext_reboot(struct notifier_block *nb, unsigned long val,
> -			       void *v)
> -{
> -	struct mtd_info *mtd;
> -
> -	mtd = container_of(nb, struct mtd_info, reboot_notifier);
> -	cfi_intelext_reset(mtd);
> -	return NOTIFY_DONE;
>  }
>  
>  static void cfi_intelext_destroy(struct mtd_info *mtd)
> @@ -2647,7 +2632,6 @@ static void cfi_intelext_destroy(struct mtd_info *mtd)
>  	struct mtd_erase_region_info *region;
>  	int i;
>  	cfi_intelext_reset(mtd);
> -	unregister_reboot_notifier(&mtd->reboot_notifier);
>  	kfree(cfi->cmdset_priv);
>  	kfree(cfi->cfiq);
>  	kfree(cfi->chips[0].priv);
> diff --git a/drivers/mtd/chips/cfi_cmdset_0002.c b/drivers/mtd/chips/cfi_cmdset_0002.c
> index c50d8cf0f60d..c4f63482cf96 100644
> --- a/drivers/mtd/chips/cfi_cmdset_0002.c
> +++ b/drivers/mtd/chips/cfi_cmdset_0002.c
> @@ -31,7 +31,6 @@
>  #include <linux/slab.h>
>  #include <linux/delay.h>
>  #include <linux/interrupt.h>
> -#include <linux/reboot.h>
>  #include <linux/of.h>
>  #include <linux/of_platform.h>
>  #include <linux/mtd/map.h>
> @@ -57,7 +56,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 void cfi_amdstd_reset(struct mtd_info *);
>  static int cfi_amdstd_get_fact_prot_info(struct mtd_info *, size_t,
>  					 size_t *, struct otp_info *);
>  static int cfi_amdstd_get_user_prot_info(struct mtd_info *, size_t,
> @@ -529,6 +528,7 @@ struct mtd_info *cfi_cmdset_0002(struct map_info *map, int primary)
>  	mtd->_sync    = cfi_amdstd_sync;
>  	mtd->_suspend = cfi_amdstd_suspend;
>  	mtd->_resume  = cfi_amdstd_resume;
> +	mtd->_reboot  = cfi_amdstd_reset;
>  	mtd->_read_user_prot_reg = cfi_amdstd_read_user_prot_reg;
>  	mtd->_read_fact_prot_reg = cfi_amdstd_read_fact_prot_reg;
>  	mtd->_get_fact_prot_info = cfi_amdstd_get_fact_prot_info;
> @@ -544,7 +544,6 @@ struct mtd_info *cfi_cmdset_0002(struct map_info *map, int primary)
>  			mtd->writebufsize);
>  
>  	mtd->_panic_write = cfi_amdstd_panic_write;
> -	mtd->reboot_notifier.notifier_call = cfi_amdstd_reboot;
>  
>  	if (cfi->cfi_mode==CFI_MODE_CFI){
>  		unsigned char bootloc;
> @@ -717,7 +716,6 @@ static struct mtd_info *cfi_amdstd_setup(struct mtd_info *mtd)
>  	}
>  
>  	__module_get(THIS_MODULE);
> -	register_reboot_notifier(&mtd->reboot_notifier);
>  	return mtd;
>  
>   setup_err:
> @@ -2871,7 +2869,7 @@ static void cfi_amdstd_resume(struct mtd_info *mtd)
>   * 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)
> +static void cfi_amdstd_reset(struct mtd_info *mtd)
>  {
>  	struct map_info *map = mtd->priv;
>  	struct cfi_private *cfi = map->fldrv_priv;
> @@ -2893,19 +2891,6 @@ static int cfi_amdstd_reset(struct mtd_info *mtd)
>  
>  		mutex_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;
>  }
>  
>  
> @@ -2915,7 +2900,6 @@ static void cfi_amdstd_destroy(struct mtd_info *mtd)
>  	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);

Re: [3/3] mtd: cfi_cmdset_{0001, 0002}: use common MTD reboot boilerplate

[Brian Norris]

On Mon, Nov 02, 2015 at 12:05:13PM -0800, Guenter Roeck wrote:
> On Thu, Dec 04, 2014 at 06:36:08PM -0800, Brian Norris wrote:
> > We don't have to implement this glue code in the chip driver any more.
> > 
> > Signed-off-by: Brian Norris <computersforpeace@gmail.com>
> > ---
> > Not tested yet
> > 
> Tested-by: Guenter Roeck <linux@roeck-us.net>

Thanks! Did you verify that your reboot notifier callback (e.g.,
cfi_intelext_reset()) is actually called on reboot?

And actually, I do see a problem with this patch now, so I'm less likely
to take it as-is; we never guarantee that this mtd_info struct actually
gets registered via mtd_device_parse_register(), so even though we
stick the right callback in mtd->_reboot(), we haven't actually ensured
it will ever get called.

See, for instance, the calling path in (speaking of the devil)
arch/cris/arch-v10/drivers/axisflashmap.c:

static struct mtd_info *flash_probe(void)
{
...
	mtd_cse0 = probe_cs(&map_cse0);
	mtd_cse1 = probe_cs(&map_cse1);
...
        if (mtd_cse0 && mtd_cse1) {
                struct mtd_info *mtds[] = { mtd_cse0, mtd_cse1 };

...
                mtd_cse = mtd_concat_create(mtds, ARRAY_SIZE(mtds),
                                            "cse0+cse1");


So, either of map_cse0 or map_cse1 could be a CFI-based map, and so only the
concatenation would be registered, and therefore the reboot() notifier will
just be left assigned to the sub-device, but never registered with the core.

Now, it looks like this is one of very few cases that we'd have to worry
about... right now I've only found physmap_of.c that does the same
mtdconcat stuff. So maybe if we fixed mtdconcat, we could solve this...

MTD is too hairy :(

Anyway, I'll have to NAK my own patch still, and we should probalby go
with your other solution to your current problems from here:

http://lists.infradead.org/pipermail/linux-mtd/2015-November/063022.html

Regards,
Brian

> >  drivers/mtd/chips/cfi_cmdset_0001.c | 22 +++-------------------
> >  drivers/mtd/chips/cfi_cmdset_0002.c | 22 +++-------------------
> >  2 files changed, 6 insertions(+), 38 deletions(-)
> > 
> > diff --git a/drivers/mtd/chips/cfi_cmdset_0001.c b/drivers/mtd/chips/cfi_cmdset_0001.c
> > index 286b97a304cf..3df9744496b2 100644
> > --- a/drivers/mtd/chips/cfi_cmdset_0001.c
> > +++ b/drivers/mtd/chips/cfi_cmdset_0001.c
> > @@ -28,7 +28,6 @@
> >  #include <linux/slab.h>
> >  #include <linux/delay.h>
> >  #include <linux/interrupt.h>
> > -#include <linux/reboot.h>
> >  #include <linux/bitmap.h>
> >  #include <linux/mtd/xip.h>
> >  #include <linux/mtd/map.h>
> > @@ -80,7 +79,7 @@ static int cfi_intelext_get_user_prot_info(struct mtd_info *, size_t,
> >  #endif
> >  static int cfi_intelext_suspend (struct mtd_info *);
> >  static void cfi_intelext_resume (struct mtd_info *);
> > -static int cfi_intelext_reboot (struct notifier_block *, unsigned long, void *);
> > +static void cfi_intelext_reset(struct mtd_info *);
> >  
> >  static void cfi_intelext_destroy(struct mtd_info *);
> >  
> > @@ -486,13 +485,12 @@ struct mtd_info *cfi_cmdset_0001(struct map_info *map, int primary)
> >  	mtd->_is_locked = cfi_intelext_is_locked;
> >  	mtd->_suspend = cfi_intelext_suspend;
> >  	mtd->_resume  = cfi_intelext_resume;
> > +	mtd->_reboot  = cfi_intelext_reset;
> >  	mtd->flags   = MTD_CAP_NORFLASH;
> >  	mtd->name    = map->name;
> >  	mtd->writesize = 1;
> >  	mtd->writebufsize = cfi_interleave(cfi) << cfi->cfiq->MaxBufWriteSize;
> >  
> > -	mtd->reboot_notifier.notifier_call = cfi_intelext_reboot;
> > -
> >  	if (cfi->cfi_mode == CFI_MODE_CFI) {
> >  		/*
> >  		 * It's a real CFI chip, not one for which the probe
> > @@ -646,7 +644,6 @@ static struct mtd_info *cfi_intelext_setup(struct mtd_info *mtd)
> >  		goto setup_err;
> >  
> >  	__module_get(THIS_MODULE);
> > -	register_reboot_notifier(&mtd->reboot_notifier);
> >  	return mtd;
> >  
> >   setup_err:
> > @@ -2605,7 +2602,7 @@ static void cfi_intelext_resume(struct mtd_info *mtd)
> >  		cfi_intelext_restore_locks(mtd);
> >  }
> >  
> > -static int cfi_intelext_reset(struct mtd_info *mtd)
> > +static void cfi_intelext_reset(struct mtd_info *mtd)
> >  {
> >  	struct map_info *map = mtd->priv;
> >  	struct cfi_private *cfi = map->fldrv_priv;
> > @@ -2626,18 +2623,6 @@ static int cfi_intelext_reset(struct mtd_info *mtd)
> >  		}
> >  		mutex_unlock(&chip->mutex);
> >  	}
> > -
> > -	return 0;
> > -}
> > -
> > -static int cfi_intelext_reboot(struct notifier_block *nb, unsigned long val,
> > -			       void *v)
> > -{
> > -	struct mtd_info *mtd;
> > -
> > -	mtd = container_of(nb, struct mtd_info, reboot_notifier);
> > -	cfi_intelext_reset(mtd);
> > -	return NOTIFY_DONE;
> >  }
> >  
> >  static void cfi_intelext_destroy(struct mtd_info *mtd)
> > @@ -2647,7 +2632,6 @@ static void cfi_intelext_destroy(struct mtd_info *mtd)
> >  	struct mtd_erase_region_info *region;
> >  	int i;
> >  	cfi_intelext_reset(mtd);
> > -	unregister_reboot_notifier(&mtd->reboot_notifier);
> >  	kfree(cfi->cmdset_priv);
> >  	kfree(cfi->cfiq);
> >  	kfree(cfi->chips[0].priv);
> > diff --git a/drivers/mtd/chips/cfi_cmdset_0002.c b/drivers/mtd/chips/cfi_cmdset_0002.c
> > index c50d8cf0f60d..c4f63482cf96 100644
> > --- a/drivers/mtd/chips/cfi_cmdset_0002.c
> > +++ b/drivers/mtd/chips/cfi_cmdset_0002.c
> > @@ -31,7 +31,6 @@
> >  #include <linux/slab.h>
> >  #include <linux/delay.h>
> >  #include <linux/interrupt.h>
> > -#include <linux/reboot.h>
> >  #include <linux/of.h>
> >  #include <linux/of_platform.h>
> >  #include <linux/mtd/map.h>
> > @@ -57,7 +56,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 void cfi_amdstd_reset(struct mtd_info *);
> >  static int cfi_amdstd_get_fact_prot_info(struct mtd_info *, size_t,
> >  					 size_t *, struct otp_info *);
> >  static int cfi_amdstd_get_user_prot_info(struct mtd_info *, size_t,
> > @@ -529,6 +528,7 @@ struct mtd_info *cfi_cmdset_0002(struct map_info *map, int primary)
> >  	mtd->_sync    = cfi_amdstd_sync;
> >  	mtd->_suspend = cfi_amdstd_suspend;
> >  	mtd->_resume  = cfi_amdstd_resume;
> > +	mtd->_reboot  = cfi_amdstd_reset;
> >  	mtd->_read_user_prot_reg = cfi_amdstd_read_user_prot_reg;
> >  	mtd->_read_fact_prot_reg = cfi_amdstd_read_fact_prot_reg;
> >  	mtd->_get_fact_prot_info = cfi_amdstd_get_fact_prot_info;
> > @@ -544,7 +544,6 @@ struct mtd_info *cfi_cmdset_0002(struct map_info *map, int primary)
> >  			mtd->writebufsize);
> >  
> >  	mtd->_panic_write = cfi_amdstd_panic_write;
> > -	mtd->reboot_notifier.notifier_call = cfi_amdstd_reboot;
> >  
> >  	if (cfi->cfi_mode==CFI_MODE_CFI){
> >  		unsigned char bootloc;
> > @@ -717,7 +716,6 @@ static struct mtd_info *cfi_amdstd_setup(struct mtd_info *mtd)
> >  	}
> >  
> >  	__module_get(THIS_MODULE);
> > -	register_reboot_notifier(&mtd->reboot_notifier);
> >  	return mtd;
> >  
> >   setup_err:
> > @@ -2871,7 +2869,7 @@ static void cfi_amdstd_resume(struct mtd_info *mtd)
> >   * 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)
> > +static void cfi_amdstd_reset(struct mtd_info *mtd)
> >  {
> >  	struct map_info *map = mtd->priv;
> >  	struct cfi_private *cfi = map->fldrv_priv;
> > @@ -2893,19 +2891,6 @@ static int cfi_amdstd_reset(struct mtd_info *mtd)
> >  
> >  		mutex_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;
> >  }
> >  
> >  
> > @@ -2915,7 +2900,6 @@ static void cfi_amdstd_destroy(struct mtd_info *mtd)
> >  	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);

Re: [3/3] mtd: cfi_cmdset_{0001, 0002}: use common MTD reboot boilerplate

[Brian Norris]

On Mon, Nov 02, 2015 at 01:58:18PM -0800, Brian Norris wrote:
> Now, it looks like this is one of very few cases that we'd have to worry
> about... right now I've only found physmap_of.c that does the same
> mtdconcat stuff. So maybe if we fixed mtdconcat, we could solve this...

There's also physmap.c and drivers/mtd/maps/sc520cdp.c. But the problem
only lies when these drivers have to concatenate MTDs, so the original
mtd_info's never get registered on their own.

Brian

Re: [3/3] mtd: cfi_cmdset_{0001, 0002}: use common MTD reboot boilerplate

[Guenter Roeck]

On Mon, Nov 02, 2015 at 01:58:18PM -0800, Brian Norris wrote:
> On Mon, Nov 02, 2015 at 12:05:13PM -0800, Guenter Roeck wrote:
> > On Thu, Dec 04, 2014 at 06:36:08PM -0800, Brian Norris wrote:
> > > We don't have to implement this glue code in the chip driver any more.
> > > 
> > > Signed-off-by: Brian Norris <computersforpeace@gmail.com>
> > > ---
> > > Not tested yet
> > > 
> > Tested-by: Guenter Roeck <linux@roeck-us.net>
> 
> Thanks! Did you verify that your reboot notifier callback (e.g.,
> cfi_intelext_reset()) is actually called on reboot?
> 
No, I just verified that it boots and shuts down correctly.

Do you want me to verify if the notifier is called, given the
problems you discovered with the patch ?

Thanks,
Guenter

> And actually, I do see a problem with this patch now, so I'm less likely
> to take it as-is; we never guarantee that this mtd_info struct actually
> gets registered via mtd_device_parse_register(), so even though we
> stick the right callback in mtd->_reboot(), we haven't actually ensured
> it will ever get called.
> 
> See, for instance, the calling path in (speaking of the devil)
> arch/cris/arch-v10/drivers/axisflashmap.c:
> 
> static struct mtd_info *flash_probe(void)
> {
> ...
> 	mtd_cse0 = probe_cs(&map_cse0);
> 	mtd_cse1 = probe_cs(&map_cse1);
> ...
>         if (mtd_cse0 && mtd_cse1) {
>                 struct mtd_info *mtds[] = { mtd_cse0, mtd_cse1 };
> 
> ...
>                 mtd_cse = mtd_concat_create(mtds, ARRAY_SIZE(mtds),
>                                             "cse0+cse1");
> 
> 
> So, either of map_cse0 or map_cse1 could be a CFI-based map, and so only the
> concatenation would be registered, and therefore the reboot() notifier will
> just be left assigned to the sub-device, but never registered with the core.
> 
> Now, it looks like this is one of very few cases that we'd have to worry
> about... right now I've only found physmap_of.c that does the same
> mtdconcat stuff. So maybe if we fixed mtdconcat, we could solve this...
> 
> MTD is too hairy :(
> 
> Anyway, I'll have to NAK my own patch still, and we should probalby go
> with your other solution to your current problems from here:
> 
> http://lists.infradead.org/pipermail/linux-mtd/2015-November/063022.html
> 
> Regards,
> Brian
> 
> > >  drivers/mtd/chips/cfi_cmdset_0001.c | 22 +++-------------------
> > >  drivers/mtd/chips/cfi_cmdset_0002.c | 22 +++-------------------
> > >  2 files changed, 6 insertions(+), 38 deletions(-)
> > > 
> > > diff --git a/drivers/mtd/chips/cfi_cmdset_0001.c b/drivers/mtd/chips/cfi_cmdset_0001.c
> > > index 286b97a304cf..3df9744496b2 100644
> > > --- a/drivers/mtd/chips/cfi_cmdset_0001.c
> > > +++ b/drivers/mtd/chips/cfi_cmdset_0001.c
> > > @@ -28,7 +28,6 @@
> > >  #include <linux/slab.h>
> > >  #include <linux/delay.h>
> > >  #include <linux/interrupt.h>
> > > -#include <linux/reboot.h>
> > >  #include <linux/bitmap.h>
> > >  #include <linux/mtd/xip.h>
> > >  #include <linux/mtd/map.h>
> > > @@ -80,7 +79,7 @@ static int cfi_intelext_get_user_prot_info(struct mtd_info *, size_t,
> > >  #endif
> > >  static int cfi_intelext_suspend (struct mtd_info *);
> > >  static void cfi_intelext_resume (struct mtd_info *);
> > > -static int cfi_intelext_reboot (struct notifier_block *, unsigned long, void *);
> > > +static void cfi_intelext_reset(struct mtd_info *);
> > >  
> > >  static void cfi_intelext_destroy(struct mtd_info *);
> > >  
> > > @@ -486,13 +485,12 @@ struct mtd_info *cfi_cmdset_0001(struct map_info *map, int primary)
> > >  	mtd->_is_locked = cfi_intelext_is_locked;
> > >  	mtd->_suspend = cfi_intelext_suspend;
> > >  	mtd->_resume  = cfi_intelext_resume;
> > > +	mtd->_reboot  = cfi_intelext_reset;
> > >  	mtd->flags   = MTD_CAP_NORFLASH;
> > >  	mtd->name    = map->name;
> > >  	mtd->writesize = 1;
> > >  	mtd->writebufsize = cfi_interleave(cfi) << cfi->cfiq->MaxBufWriteSize;
> > >  
> > > -	mtd->reboot_notifier.notifier_call = cfi_intelext_reboot;
> > > -
> > >  	if (cfi->cfi_mode == CFI_MODE_CFI) {
> > >  		/*
> > >  		 * It's a real CFI chip, not one for which the probe
> > > @@ -646,7 +644,6 @@ static struct mtd_info *cfi_intelext_setup(struct mtd_info *mtd)
> > >  		goto setup_err;
> > >  
> > >  	__module_get(THIS_MODULE);
> > > -	register_reboot_notifier(&mtd->reboot_notifier);
> > >  	return mtd;
> > >  
> > >   setup_err:
> > > @@ -2605,7 +2602,7 @@ static void cfi_intelext_resume(struct mtd_info *mtd)
> > >  		cfi_intelext_restore_locks(mtd);
> > >  }
> > >  
> > > -static int cfi_intelext_reset(struct mtd_info *mtd)
> > > +static void cfi_intelext_reset(struct mtd_info *mtd)
> > >  {
> > >  	struct map_info *map = mtd->priv;
> > >  	struct cfi_private *cfi = map->fldrv_priv;
> > > @@ -2626,18 +2623,6 @@ static int cfi_intelext_reset(struct mtd_info *mtd)
> > >  		}
> > >  		mutex_unlock(&chip->mutex);
> > >  	}
> > > -
> > > -	return 0;
> > > -}
> > > -
> > > -static int cfi_intelext_reboot(struct notifier_block *nb, unsigned long val,
> > > -			       void *v)
> > > -{
> > > -	struct mtd_info *mtd;
> > > -
> > > -	mtd = container_of(nb, struct mtd_info, reboot_notifier);
> > > -	cfi_intelext_reset(mtd);
> > > -	return NOTIFY_DONE;
> > >  }
> > >  
> > >  static void cfi_intelext_destroy(struct mtd_info *mtd)
> > > @@ -2647,7 +2632,6 @@ static void cfi_intelext_destroy(struct mtd_info *mtd)
> > >  	struct mtd_erase_region_info *region;
> > >  	int i;
> > >  	cfi_intelext_reset(mtd);
> > > -	unregister_reboot_notifier(&mtd->reboot_notifier);
> > >  	kfree(cfi->cmdset_priv);
> > >  	kfree(cfi->cfiq);
> > >  	kfree(cfi->chips[0].priv);
> > > diff --git a/drivers/mtd/chips/cfi_cmdset_0002.c b/drivers/mtd/chips/cfi_cmdset_0002.c
> > > index c50d8cf0f60d..c4f63482cf96 100644
> > > --- a/drivers/mtd/chips/cfi_cmdset_0002.c
> > > +++ b/drivers/mtd/chips/cfi_cmdset_0002.c
> > > @@ -31,7 +31,6 @@
> > >  #include <linux/slab.h>
> > >  #include <linux/delay.h>
> > >  #include <linux/interrupt.h>
> > > -#include <linux/reboot.h>
> > >  #include <linux/of.h>
> > >  #include <linux/of_platform.h>
> > >  #include <linux/mtd/map.h>
> > > @@ -57,7 +56,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 void cfi_amdstd_reset(struct mtd_info *);
> > >  static int cfi_amdstd_get_fact_prot_info(struct mtd_info *, size_t,
> > >  					 size_t *, struct otp_info *);
> > >  static int cfi_amdstd_get_user_prot_info(struct mtd_info *, size_t,
> > > @@ -529,6 +528,7 @@ struct mtd_info *cfi_cmdset_0002(struct map_info *map, int primary)
> > >  	mtd->_sync    = cfi_amdstd_sync;
> > >  	mtd->_suspend = cfi_amdstd_suspend;
> > >  	mtd->_resume  = cfi_amdstd_resume;
> > > +	mtd->_reboot  = cfi_amdstd_reset;
> > >  	mtd->_read_user_prot_reg = cfi_amdstd_read_user_prot_reg;
> > >  	mtd->_read_fact_prot_reg = cfi_amdstd_read_fact_prot_reg;
> > >  	mtd->_get_fact_prot_info = cfi_amdstd_get_fact_prot_info;
> > > @@ -544,7 +544,6 @@ struct mtd_info *cfi_cmdset_0002(struct map_info *map, int primary)
> > >  			mtd->writebufsize);
> > >  
> > >  	mtd->_panic_write = cfi_amdstd_panic_write;
> > > -	mtd->reboot_notifier.notifier_call = cfi_amdstd_reboot;
> > >  
> > >  	if (cfi->cfi_mode==CFI_MODE_CFI){
> > >  		unsigned char bootloc;
> > > @@ -717,7 +716,6 @@ static struct mtd_info *cfi_amdstd_setup(struct mtd_info *mtd)
> > >  	}
> > >  
> > >  	__module_get(THIS_MODULE);
> > > -	register_reboot_notifier(&mtd->reboot_notifier);
> > >  	return mtd;
> > >  
> > >   setup_err:
> > > @@ -2871,7 +2869,7 @@ static void cfi_amdstd_resume(struct mtd_info *mtd)
> > >   * 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)
> > > +static void cfi_amdstd_reset(struct mtd_info *mtd)
> > >  {
> > >  	struct map_info *map = mtd->priv;
> > >  	struct cfi_private *cfi = map->fldrv_priv;
> > > @@ -2893,19 +2891,6 @@ static int cfi_amdstd_reset(struct mtd_info *mtd)
> > >  
> > >  		mutex_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;
> > >  }
> > >  
> > >  
> > > @@ -2915,7 +2900,6 @@ static void cfi_amdstd_destroy(struct mtd_info *mtd)
> > >  	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);

Re: [3/3] mtd: cfi_cmdset_{0001, 0002}: use common MTD reboot boilerplate

[Brian Norris]

On Mon, Nov 02, 2015 at 02:16:33PM -0800, Guenter Roeck wrote:
> On Mon, Nov 02, 2015 at 01:58:18PM -0800, Brian Norris wrote:
> > On Mon, Nov 02, 2015 at 12:05:13PM -0800, Guenter Roeck wrote:
> > > On Thu, Dec 04, 2014 at 06:36:08PM -0800, Brian Norris wrote:
> > > > We don't have to implement this glue code in the chip driver any more.
> > > > 
> > > > Signed-off-by: Brian Norris <computersforpeace@gmail.com>
> > > > ---
> > > > Not tested yet
> > > > 
> > > Tested-by: Guenter Roeck <linux@roeck-us.net>
> > 
> > Thanks! Did you verify that your reboot notifier callback (e.g.,
> > cfi_intelext_reset()) is actually called on reboot?
> > 
> No, I just verified that it boots and shuts down correctly.
> 
> Do you want me to verify if the notifier is called, given the
> problems you discovered with the patch ?

Up to you. It'd be nice to know, but even with that info, we'd have more
work to do before we can take this, since I'm pretty sure there are
platforms out there where the notifier won't be called.

(And sorry, my emails can be a little evolutionary as they progress,
when I start to type before I've finished investigating everything.)

BTW, do you want to send out the patch for your suggestion in the other
thread, or should I?

Thanks,
Brian

Re: [3/3] mtd: cfi_cmdset_{0001, 0002}: use common MTD reboot boilerplate

[Guenter Roeck]

On 11/02/2015 03:21 PM, Brian Norris wrote:
> On Mon, Nov 02, 2015 at 02:16:33PM -0800, Guenter Roeck wrote:
>> On Mon, Nov 02, 2015 at 01:58:18PM -0800, Brian Norris wrote:
>>> On Mon, Nov 02, 2015 at 12:05:13PM -0800, Guenter Roeck wrote:
>>>> On Thu, Dec 04, 2014 at 06:36:08PM -0800, Brian Norris wrote:
>>>>> We don't have to implement this glue code in the chip driver any more.
>>>>>
>>>>> Signed-off-by: Brian Norris <computersforpeace@gmail.com>
>>>>> ---
>>>>> Not tested yet
>>>>>
>>>> Tested-by: Guenter Roeck <linux@roeck-us.net>
>>>
>>> Thanks! Did you verify that your reboot notifier callback (e.g.,
>>> cfi_intelext_reset()) is actually called on reboot?
>>>
>> No, I just verified that it boots and shuts down correctly.
>>
>> Do you want me to verify if the notifier is called, given the
>> problems you discovered with the patch ?
>
> Up to you. It'd be nice to know, but even with that info, we'd have more
> work to do before we can take this, since I'm pretty sure there are
> platforms out there where the notifier won't be called.
>

I added a WARN() into cfi_intelext_reset() and got:

WARNING: CPU: 0 PID: 504 at drivers/mtd/chips/cfi_cmdset_0001.c:2611 cfi_intelext_reset+0x44/0x130()
cfi_intelext_reset called

which we can take as hint that it was called.

> (And sorry, my emails can be a little evolutionary as they progress,
> when I start to type before I've finished investigating everything.)
>
> BTW, do you want to send out the patch for your suggestion in the other
> thread, or should I?
>
Please go ahead and do it. I can test it if you Cc: me on it.

Thanks,
Guenter