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

All of lore.kernel.org
 help / color / mirror / Atom feed

* [PATCH 1/3] mtd: implement common reboot notifier boilerplate
@ 2014-12-05  2:36 Brian Norris
  2014-12-05  2:36 ` [PATCH 2/3] mtd: nand: added nand_shutdown Brian Norris
                   ` (3 more replies)
  0 siblings, 4 replies; 16+ messages in thread
From: Brian Norris @ 2014-12-05  2:36 UTC (permalink / raw)
  To: linux-mtd; +Cc: cdoban, rjui, Brian Norris, sbranden

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

^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [PATCH 2/3] mtd: nand: added nand_shutdown
  2014-12-05  2:36 [PATCH 1/3] mtd: implement common reboot notifier boilerplate Brian Norris
@ 2014-12-05  2:36 ` Brian Norris
  2014-12-05 17:47   ` Scott Branden
  2014-12-05  2:36 ` [PATCH 3/3] mtd: cfi_cmdset_{0001, 0002}: use common MTD reboot boilerplate Brian Norris
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 16+ messages in thread
From: Brian Norris @ 2014-12-05  2:36 UTC (permalink / raw)
  To: linux-mtd; +Cc: cdoban, rjui, Brian Norris, sbranden

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

^ permalink raw reply related	[flat|nested] 16+ messages in thread

* Re: [PATCH 2/3] mtd: nand: added nand_shutdown
  2014-12-05  2:36 ` [PATCH 2/3] mtd: nand: added nand_shutdown Brian Norris
@ 2014-12-05 17:47   ` Scott Branden
  0 siblings, 0 replies; 16+ messages in thread
From: Scott Branden @ 2014-12-05 17:47 UTC (permalink / raw)
  To: Brian Norris, linux-mtd; +Cc: cdoban, rjui

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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* [PATCH 3/3] mtd: cfi_cmdset_{0001, 0002}: use common MTD reboot boilerplate
  2014-12-05  2:36 [PATCH 1/3] mtd: implement common reboot notifier boilerplate Brian Norris
  2014-12-05  2:36 ` [PATCH 2/3] mtd: nand: added nand_shutdown Brian Norris
@ 2014-12-05  2:36 ` Brian Norris
  2015-11-02 20:05   ` [3/3] " Guenter Roeck
  2014-12-05  6:48 ` [PATCH 1/3] mtd: implement common reboot notifier boilerplate Scott Branden
  2015-01-08  1:58 ` Brian Norris
  3 siblings, 1 reply; 16+ messages in thread
From: Brian Norris @ 2014-12-05  2:36 UTC (permalink / raw)
  To: linux-mtd; +Cc: cdoban, rjui, Brian Norris, sbranden

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

^ permalink raw reply related	[flat|nested] 16+ messages in thread

* Re: [3/3] mtd: cfi_cmdset_{0001, 0002}: use common MTD reboot boilerplate
  2014-12-05  2:36 ` [PATCH 3/3] mtd: cfi_cmdset_{0001, 0002}: use common MTD reboot boilerplate Brian Norris
@ 2015-11-02 20:05   ` Guenter Roeck
  2015-11-02 21:58     ` Brian Norris
  0 siblings, 1 reply; 16+ messages in thread
From: Guenter Roeck @ 2015-11-02 20:05 UTC (permalink / raw)
  To: Brian Norris; +Cc: linux-mtd, cdoban, rjui, sbranden

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);

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [3/3] mtd: cfi_cmdset_{0001, 0002}: use common MTD reboot boilerplate
  2015-11-02 20:05   ` [3/3] " Guenter Roeck
@ 2015-11-02 21:58     ` Brian Norris
  2015-11-02 22:05       ` Brian Norris
  2015-11-02 22:16       ` Guenter Roeck
  0 siblings, 2 replies; 16+ messages in thread
From: Brian Norris @ 2015-11-02 21:58 UTC (permalink / raw)
  To: Guenter Roeck; +Cc: linux-mtd, cdoban, rjui, sbranden

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);

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [3/3] mtd: cfi_cmdset_{0001, 0002}: use common MTD reboot boilerplate
  2015-11-02 21:58     ` Brian Norris
@ 2015-11-02 22:05       ` Brian Norris
  2015-11-02 22:16       ` Guenter Roeck
  1 sibling, 0 replies; 16+ messages in thread
From: Brian Norris @ 2015-11-02 22:05 UTC (permalink / raw)
  To: Guenter Roeck; +Cc: linux-mtd, cdoban, rjui, sbranden

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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [3/3] mtd: cfi_cmdset_{0001, 0002}: use common MTD reboot boilerplate
  2015-11-02 21:58     ` Brian Norris
  2015-11-02 22:05       ` Brian Norris
@ 2015-11-02 22:16       ` Guenter Roeck
  2015-11-02 23:21         ` Brian Norris
  1 sibling, 1 reply; 16+ messages in thread
From: Guenter Roeck @ 2015-11-02 22:16 UTC (permalink / raw)
  To: Brian Norris; +Cc: linux-mtd, cdoban, rjui, sbranden

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);

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [3/3] mtd: cfi_cmdset_{0001, 0002}: use common MTD reboot boilerplate
  2015-11-02 22:16       ` Guenter Roeck
@ 2015-11-02 23:21         ` Brian Norris
  2015-11-03  1:00           ` Guenter Roeck
  0 siblings, 1 reply; 16+ messages in thread
From: Brian Norris @ 2015-11-02 23:21 UTC (permalink / raw)
  To: Guenter Roeck; +Cc: linux-mtd, cdoban, rjui, sbranden

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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [3/3] mtd: cfi_cmdset_{0001, 0002}: use common MTD reboot boilerplate
  2015-11-02 23:21         ` Brian Norris
@ 2015-11-03  1:00           ` Guenter Roeck
  0 siblings, 0 replies; 16+ messages in thread
From: Guenter Roeck @ 2015-11-03  1:00 UTC (permalink / raw)
  To: Brian Norris; +Cc: linux-mtd, cdoban, rjui, sbranden

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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 1/3] mtd: implement common reboot notifier boilerplate
  2014-12-05  2:36 [PATCH 1/3] mtd: implement common reboot notifier boilerplate Brian Norris
  2014-12-05  2:36 ` [PATCH 2/3] mtd: nand: added nand_shutdown Brian Norris
  2014-12-05  2:36 ` [PATCH 3/3] mtd: cfi_cmdset_{0001, 0002}: use common MTD reboot boilerplate Brian Norris
@ 2014-12-05  6:48 ` Scott Branden
  2014-12-05 18:07   ` Brian Norris
  2015-01-08  1:58 ` Brian Norris
  3 siblings, 1 reply; 16+ messages in thread
From: Scott Branden @ 2014-12-05  6:48 UTC (permalink / raw)
  To: Brian Norris, linux-mtd; +Cc: cdoban, rjui

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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 1/3] mtd: implement common reboot notifier boilerplate
  2014-12-05  6:48 ` [PATCH 1/3] mtd: implement common reboot notifier boilerplate Scott Branden
@ 2014-12-05 18:07   ` Brian Norris
  2014-12-10  0:02     ` Scott Branden
  0 siblings, 1 reply; 16+ messages in thread
From: Brian Norris @ 2014-12-05 18:07 UTC (permalink / raw)
  To: Scott Branden
  Cc: ricard.wanderlof, Artem Bityutskiy, cdoban, rjui,
	richard.weinberger, linux-mtd, David Woodhouse

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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 1/3] mtd: implement common reboot notifier boilerplate
  2014-12-05 18:07   ` Brian Norris
@ 2014-12-10  0:02     ` Scott Branden
  2014-12-10  0:07       ` Richard Weinberger
  0 siblings, 1 reply; 16+ messages in thread
From: Scott Branden @ 2014-12-10  0:02 UTC (permalink / raw)
  To: Brian Norris
  Cc: ricard.wanderlof, Artem Bityutskiy, cdoban, rjui,
	richard.weinberger, linux-mtd, David Woodhouse

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
>

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 1/3] mtd: implement common reboot notifier boilerplate
  2014-12-10  0:02     ` Scott Branden
@ 2014-12-10  0:07       ` Richard Weinberger
  0 siblings, 0 replies; 16+ messages in thread
From: Richard Weinberger @ 2014-12-10  0:07 UTC (permalink / raw)
  To: Scott Branden, Brian Norris
  Cc: ricard.wanderlof, Artem Bityutskiy, cdoban, rjui,
	richard.weinberger, linux-mtd, David Woodhouse

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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 1/3] mtd: implement common reboot notifier boilerplate
  2014-12-05  2:36 [PATCH 1/3] mtd: implement common reboot notifier boilerplate Brian Norris
                   ` (2 preceding siblings ...)
  2014-12-05  6:48 ` [PATCH 1/3] mtd: implement common reboot notifier boilerplate Scott Branden
@ 2015-01-08  1:58 ` Brian Norris
  2015-01-14  0:43   ` Scott Branden
  3 siblings, 1 reply; 16+ messages in thread
From: Brian Norris @ 2015-01-08  1:58 UTC (permalink / raw)
  To: linux-mtd; +Cc: cdoban, rjui, sbranden

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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 1/3] mtd: implement common reboot notifier boilerplate
  2015-01-08  1:58 ` Brian Norris
@ 2015-01-14  0:43   ` Scott Branden
  0 siblings, 0 replies; 16+ messages in thread
From: Scott Branden @ 2015-01-14  0:43 UTC (permalink / raw)
  To: Brian Norris, linux-mtd; +Cc: Corneliu Doban, Ray Jui

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
>

^ permalink raw reply	[flat|nested] 16+ messages in thread

end of thread, other threads:[~2015-11-03  1:01 UTC | newest]

Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-12-05  2:36 [PATCH 1/3] mtd: implement common reboot notifier boilerplate Brian Norris
2014-12-05  2:36 ` [PATCH 2/3] mtd: nand: added nand_shutdown Brian Norris
2014-12-05 17:47   ` Scott Branden
2014-12-05  2:36 ` [PATCH 3/3] mtd: cfi_cmdset_{0001, 0002}: use common MTD reboot boilerplate Brian Norris
2015-11-02 20:05   ` [3/3] " Guenter Roeck
2015-11-02 21:58     ` Brian Norris
2015-11-02 22:05       ` Brian Norris
2015-11-02 22:16       ` Guenter Roeck
2015-11-02 23:21         ` Brian Norris
2015-11-03  1:00           ` Guenter Roeck
2014-12-05  6:48 ` [PATCH 1/3] mtd: implement common reboot notifier boilerplate Scott Branden
2014-12-05 18:07   ` Brian Norris
2014-12-10  0:02     ` Scott Branden
2014-12-10  0:07       ` Richard Weinberger
2015-01-08  1:58 ` Brian Norris
2015-01-14  0:43   ` Scott Branden

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.