MTD life cycle fixes

MTD life cycle fixes

[Richard Weinberger]

While working on a nandsim feature to add/remove NAND devices
on the fly I noticed some odds. This series intends to fix them.
My initial plan was submitting them as part of my nandsim series
but it turned out to be five patches and I fear they need a good
review as they're all touching MTD core code.

Thanks,
//richard

[PATCH 1/5] mtdpart: Propagate _get/put_device()

[Richard Weinberger]

If the master device has callbacks for _get/put_device()
and this MTD has slaves a get_mtd_device() call on paritions
will never issue the registered callbacks.
Fix this by propagating _get/put_device() down.

Reviewed-by: Boris Brezillon <boris.brezillon@free-electrons.com>
Signed-off-by: Richard Weinberger <richard@nod.at>
---
 drivers/mtd/mtdpart.c | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/drivers/mtd/mtdpart.c b/drivers/mtd/mtdpart.c
index 1f13e32..ec852fa 100644
--- a/drivers/mtd/mtdpart.c
+++ b/drivers/mtd/mtdpart.c
@@ -317,6 +317,18 @@ static int part_block_markbad(struct mtd_info *mtd, loff_t ofs)
 	return res;
 }
 
+static int part_get_device(struct mtd_info *mtd)
+{
+	struct mtd_part *part = mtd_to_part(mtd);
+	return part->master->_get_device(part->master);
+}
+
+static void part_put_device(struct mtd_info *mtd)
+{
+	struct mtd_part *part = mtd_to_part(mtd);
+	part->master->_put_device(part->master);
+}
+
 static int part_ooblayout_ecc(struct mtd_info *mtd, int section,
 			      struct mtd_oob_region *oobregion)
 {
@@ -463,6 +475,12 @@ static struct mtd_part *allocate_partition(struct mtd_info *master,
 		slave->mtd._block_isbad = part_block_isbad;
 	if (master->_block_markbad)
 		slave->mtd._block_markbad = part_block_markbad;
+
+	if (master->_get_device)
+		slave->mtd._get_device = part_get_device;
+	if (master->_put_device)
+		slave->mtd._put_device = part_put_device;
+
 	slave->mtd._erase = part_erase;
 	slave->master = master;
 	slave->offset = part->offset;
-- 
2.7.3

[PATCH 2/5] mtd: nand: Propagate mtd_device_unregister() return value in tear down

[Richard Weinberger]

Provide a __nand_release() function for drivers which can deal
with a failing nand release operation.
Most drivers should be safe since they rely on module refcounting.
To catch outliers implement a nand_release() with a WARN_ON().

Signed-off-by: Richard Weinberger <richard@nod.at>
---
 drivers/mtd/nand/nand_base.c | 15 ++++++++++-----
 include/linux/mtd/nand.h     | 12 +++++++++++-
 2 files changed, 21 insertions(+), 6 deletions(-)

diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
index 0b0dc29..5b9b65b 100644
--- a/drivers/mtd/nand/nand_base.c
+++ b/drivers/mtd/nand/nand_base.c
@@ -4601,19 +4601,22 @@ int nand_scan(struct mtd_info *mtd, int maxchips)
 EXPORT_SYMBOL(nand_scan);
 
 /**
- * nand_release - [NAND Interface] Free resources held by the NAND device
+ * __nand_release - [NAND Interface] Free resources held by the NAND device
  * @mtd: MTD device structure
  */
-void nand_release(struct mtd_info *mtd)
+int __nand_release(struct mtd_info *mtd)
 {
+	int ret;
 	struct nand_chip *chip = mtd_to_nand(mtd);
 
+	ret = mtd_device_unregister(mtd);
+	if (ret)
+		return ret;
+
 	if (chip->ecc.mode == NAND_ECC_SOFT &&
 	    chip->ecc.algo == NAND_ECC_BCH)
 		nand_bch_free((struct nand_bch_control *)chip->ecc.priv);
 
-	mtd_device_unregister(mtd);
-
 	/* Free bad block table memory */
 	kfree(chip->bbt);
 	if (!(chip->options & NAND_OWN_BUFFERS))
@@ -4623,8 +4626,10 @@ void nand_release(struct mtd_info *mtd)
 	if (chip->badblock_pattern && chip->badblock_pattern->options
 			& NAND_BBT_DYNAMICSTRUCT)
 		kfree(chip->badblock_pattern);
+
+	return 0;
 }
-EXPORT_SYMBOL_GPL(nand_release);
+EXPORT_SYMBOL_GPL(__nand_release);
 
 MODULE_LICENSE("GPL");
 MODULE_AUTHOR("Steven J. Hill <sjhill@realitydiluted.com>");
diff --git a/include/linux/mtd/nand.h b/include/linux/mtd/nand.h
index fbe8e16..b3554ec 100644
--- a/include/linux/mtd/nand.h
+++ b/include/linux/mtd/nand.h
@@ -39,7 +39,7 @@ extern int nand_scan_ident(struct mtd_info *mtd, int max_chips,
 extern int nand_scan_tail(struct mtd_info *mtd);
 
 /* Free resources held by the NAND device */
-extern void nand_release(struct mtd_info *mtd);
+extern int __nand_release(struct mtd_info *mtd);
 
 /* Internal helper for board drivers which need to override command function */
 extern void nand_wait_ready(struct mtd_info *mtd);
@@ -1022,6 +1022,16 @@ static inline int jedec_feature(struct nand_chip *chip)
 		: 0;
 }
 
+static inline void nand_release(struct mtd_info *mtd)
+{
+	/*
+	 * If you face this warning your driver is doing something bad.
+	 * Don't issue nand_release() when your MTD is in use.
+	 * Use __nand_release() and handle the error correctly.
+	 */
+	WARN_ON(__nand_release(mtd) != 0);
+}
+
 /*
  * struct nand_sdr_timings - SDR NAND chip timings
  *
-- 
2.7.3

[PATCH 3/5] mtd: Don't unconditionally unregister reboot notifier

[Richard Weinberger]

del_mtd_device() is allowed to fail.
i.e. when the MTD is busy.
Unregister the reboot notifier only when we're really
about to delete the MTD.

Signed-off-by: Richard Weinberger <richard@nod.at>
---
 drivers/mtd/mtdcore.c | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c
index e3936b8..36e5fb0 100644
--- a/drivers/mtd/mtdcore.c
+++ b/drivers/mtd/mtdcore.c
@@ -654,17 +654,22 @@ 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;
 
 	if (!device_is_registered(&master->dev))
-		return 0;
+		goto unregister;
 
-	return del_mtd_device(master);
+	err = del_mtd_device(master);
+	if (err)
+		return err;
+
+unregister:
+	if (master->_reboot)
+		unregister_reboot_notifier(&master->reboot_notifier);
+
+	return 0;
 }
 EXPORT_SYMBOL_GPL(mtd_device_unregister);
 
-- 
2.7.3

[PATCH 4/5] mtd: Don't unconditionally execute remove notifiers

[Richard Weinberger]

Only call them when we're really removing the MTD.

Signed-off-by: Richard Weinberger <richard@nod.at>
---
 drivers/mtd/mtdcore.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c
index 36e5fb0..f49e103 100644
--- a/drivers/mtd/mtdcore.c
+++ b/drivers/mtd/mtdcore.c
@@ -499,16 +499,16 @@ int del_mtd_device(struct mtd_info *mtd)
 		goto out_error;
 	}
 
-	/* No need to get a refcount on the module containing
-		the notifier, since we hold the mtd_table_mutex */
-	list_for_each_entry(not, &mtd_notifiers, list)
-		not->remove(mtd);
-
 	if (mtd->usecount) {
 		printk(KERN_NOTICE "Removing MTD device #%d (%s) with use count %d\n",
 		       mtd->index, mtd->name, mtd->usecount);
 		ret = -EBUSY;
 	} else {
+		/* No need to get a refcount on the module containing
+			the notifier, since we hold the mtd_table_mutex */
+		list_for_each_entry(not, &mtd_notifiers, list)
+			not->remove(mtd);
+
 		device_unregister(&mtd->dev);
 
 		idr_remove(&mtd_idr, mtd->index);
-- 
2.7.3

[PATCH 5/5] mtd: Don't print a scary message when trying to remove a busy MTD

[Richard Weinberger]

Just return -EBUSY and everything is fine.

Signed-off-by: Richard Weinberger <richard@nod.at>
---
 drivers/mtd/mtdcore.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c
index f49e103..f2dea88 100644
--- a/drivers/mtd/mtdcore.c
+++ b/drivers/mtd/mtdcore.c
@@ -499,11 +499,9 @@ int del_mtd_device(struct mtd_info *mtd)
 		goto out_error;
 	}
 
-	if (mtd->usecount) {
-		printk(KERN_NOTICE "Removing MTD device #%d (%s) with use count %d\n",
-		       mtd->index, mtd->name, mtd->usecount);
+	if (mtd->usecount)
 		ret = -EBUSY;
-	} else {
+	else {
 		/* No need to get a refcount on the module containing
 			the notifier, since we hold the mtd_table_mutex */
 		list_for_each_entry(not, &mtd_notifiers, list)
-- 
2.7.3

Re: [PATCH 2/5] mtd: nand: Propagate mtd_device_unregister() return value in tear down

[Boris Brezillon]

On Tue,  5 Jul 2016 00:06:20 +0200
Richard Weinberger <richard@nod.at> wrote:

> Provide a __nand_release() function for drivers which can deal
> with a failing nand release operation.
> Most drivers should be safe since they rely on module refcounting.
> To catch outliers implement a nand_release() with a WARN_ON().
> 
> Signed-off-by: Richard Weinberger <richard@nod.at>
> ---
>  drivers/mtd/nand/nand_base.c | 15 ++++++++++-----
>  include/linux/mtd/nand.h     | 12 +++++++++++-
>  2 files changed, 21 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
> index 0b0dc29..5b9b65b 100644
> --- a/drivers/mtd/nand/nand_base.c
> +++ b/drivers/mtd/nand/nand_base.c
> @@ -4601,19 +4601,22 @@ int nand_scan(struct mtd_info *mtd, int maxchips)
>  EXPORT_SYMBOL(nand_scan);
>  
>  /**
> - * nand_release - [NAND Interface] Free resources held by the NAND device
> + * __nand_release - [NAND Interface] Free resources held by the NAND device
>   * @mtd: MTD device structure
>   */
> -void nand_release(struct mtd_info *mtd)
> +int __nand_release(struct mtd_info *mtd)

Can we find a better name? nand_release_safe()?

>  {
> +	int ret;
>  	struct nand_chip *chip = mtd_to_nand(mtd);
>  
> +	ret = mtd_device_unregister(mtd);
> +	if (ret)
> +		return ret;
> +

The question is, should we unregister the MTD device in nand_release().
It feels a bit odd to have nand_scan_xxx() functions only doing the
nand_chip initialization and letting the NAND controller driver
register the MTD device, and have nand_release() unregister the MTD
device for us.

Maybe we should export a nand_cleanup() function that would just
release nand_chip resources and let NAND controller drivers that
really care about mtd_device_unregister() return code call it
themselves before calling nand_cleanup() (instead of calling
nand_release()).

>  	if (chip->ecc.mode == NAND_ECC_SOFT &&
>  	    chip->ecc.algo == NAND_ECC_BCH)
>  		nand_bch_free((struct nand_bch_control *)chip->ecc.priv);
>  
> -	mtd_device_unregister(mtd);
> -
>  	/* Free bad block table memory */
>  	kfree(chip->bbt);
>  	if (!(chip->options & NAND_OWN_BUFFERS))
> @@ -4623,8 +4626,10 @@ void nand_release(struct mtd_info *mtd)
>  	if (chip->badblock_pattern && chip->badblock_pattern->options
>  			& NAND_BBT_DYNAMICSTRUCT)
>  		kfree(chip->badblock_pattern);
> +
> +	return 0;
>  }
> -EXPORT_SYMBOL_GPL(nand_release);
> +EXPORT_SYMBOL_GPL(__nand_release);
>  
>  MODULE_LICENSE("GPL");
>  MODULE_AUTHOR("Steven J. Hill <sjhill@realitydiluted.com>");
> diff --git a/include/linux/mtd/nand.h b/include/linux/mtd/nand.h
> index fbe8e16..b3554ec 100644
> --- a/include/linux/mtd/nand.h
> +++ b/include/linux/mtd/nand.h
> @@ -39,7 +39,7 @@ extern int nand_scan_ident(struct mtd_info *mtd, int max_chips,
>  extern int nand_scan_tail(struct mtd_info *mtd);
>  
>  /* Free resources held by the NAND device */
> -extern void nand_release(struct mtd_info *mtd);
> +extern int __nand_release(struct mtd_info *mtd);
>  
>  /* Internal helper for board drivers which need to override command function */
>  extern void nand_wait_ready(struct mtd_info *mtd);
> @@ -1022,6 +1022,16 @@ static inline int jedec_feature(struct nand_chip *chip)
>  		: 0;
>  }
>  
> +static inline void nand_release(struct mtd_info *mtd)
> +{
> +	/*
> +	 * If you face this warning your driver is doing something bad.
> +	 * Don't issue nand_release() when your MTD is in use.
> +	 * Use __nand_release() and handle the error correctly.
> +	 */
> +	WARN_ON(__nand_release(mtd) != 0);
> +}
> +
>  /*
>   * struct nand_sdr_timings - SDR NAND chip timings
>   *

Re: [PATCH 5/5] mtd: Don't print a scary message when trying to remove a busy MTD

[Boris Brezillon]

On Tue,  5 Jul 2016 00:06:23 +0200
Richard Weinberger <richard@nod.at> wrote:

> Just return -EBUSY and everything is fine.
> 
> Signed-off-by: Richard Weinberger <richard@nod.at>
> ---
>  drivers/mtd/mtdcore.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c
> index f49e103..f2dea88 100644
> --- a/drivers/mtd/mtdcore.c
> +++ b/drivers/mtd/mtdcore.c
> @@ -499,11 +499,9 @@ int del_mtd_device(struct mtd_info *mtd)
>  		goto out_error;
>  	}
>  
> -	if (mtd->usecount) {
> -		printk(KERN_NOTICE "Removing MTD device #%d (%s) with use count %d\n",
> -		       mtd->index, mtd->name, mtd->usecount);
> +	if (mtd->usecount)
>  		ret = -EBUSY;
> -	} else {
> +	else {

Nit: can you keep the brackets around the if (mtd->usecount) block (we
usually only drop them when both the 'if' and 'else' blocks contain
a single instruction)?

>  		/* No need to get a refcount on the module containing
>  			the notifier, since we hold the mtd_table_mutex */
>  		list_for_each_entry(not, &mtd_notifiers, list)

Re: [PATCH 4/5] mtd: Don't unconditionally execute remove notifiers

[Boris Brezillon]

On Tue,  5 Jul 2016 00:06:22 +0200
Richard Weinberger <richard@nod.at> wrote:

> Only call them when we're really removing the MTD.
> 
> Signed-off-by: Richard Weinberger <richard@nod.at>

Reviewed-by: Boris Brezillon <boris.brezillon@free-electrons.com>

> ---
>  drivers/mtd/mtdcore.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c
> index 36e5fb0..f49e103 100644
> --- a/drivers/mtd/mtdcore.c
> +++ b/drivers/mtd/mtdcore.c
> @@ -499,16 +499,16 @@ int del_mtd_device(struct mtd_info *mtd)
>  		goto out_error;
>  	}
>  
> -	/* No need to get a refcount on the module containing
> -		the notifier, since we hold the mtd_table_mutex */
> -	list_for_each_entry(not, &mtd_notifiers, list)
> -		not->remove(mtd);
> -
>  	if (mtd->usecount) {
>  		printk(KERN_NOTICE "Removing MTD device #%d (%s) with use count %d\n",
>  		       mtd->index, mtd->name, mtd->usecount);
>  		ret = -EBUSY;
>  	} else {
> +		/* No need to get a refcount on the module containing
> +			the notifier, since we hold the mtd_table_mutex */
> +		list_for_each_entry(not, &mtd_notifiers, list)
> +			not->remove(mtd);
> +
>  		device_unregister(&mtd->dev);
>  
>  		idr_remove(&mtd_idr, mtd->index);

Re: [PATCH 2/5] mtd: nand: Propagate mtd_device_unregister() return value in tear down

[Richard Weinberger]

Boris,

Am 06.07.2016 um 15:34 schrieb Boris Brezillon:
>>  /**
>> - * nand_release - [NAND Interface] Free resources held by the NAND device
>> + * __nand_release - [NAND Interface] Free resources held by the NAND device
>>   * @mtd: MTD device structure
>>   */
>> -void nand_release(struct mtd_info *mtd)
>> +int __nand_release(struct mtd_info *mtd)
> 
> Can we find a better name? nand_release_safe()?

Sure. Let's pick nand_release_safe().

>>  {
>> +	int ret;
>>  	struct nand_chip *chip = mtd_to_nand(mtd);
>>  
>> +	ret = mtd_device_unregister(mtd);
>> +	if (ret)
>> +		return ret;
>> +
> 
> The question is, should we unregister the MTD device in nand_release().
> It feels a bit odd to have nand_scan_xxx() functions only doing the
> nand_chip initialization and letting the NAND controller driver
> register the MTD device, and have nand_release() unregister the MTD
> device for us.
> 
> Maybe we should export a nand_cleanup() function that would just
> release nand_chip resources and let NAND controller drivers that
> really care about mtd_device_unregister() return code call it
> themselves before calling nand_cleanup() (instead of calling
> nand_release()).

You mean renaming nand_release() to nand_cleanup() and the driver
has to issue mtd_device_unregister() itself before it is allowed to
do nand_cleanup(). Yes, that is also an option.
The only downside is that we have to touch a lot of drivers then.
But the conversion should be almost trivial.

Thanks,
//richard

Re: [PATCH 5/5] mtd: Don't print a scary message when trying to remove a busy MTD

[Richard Weinberger]

Boris,

Am 07.07.2016 um 16:58 schrieb Boris Brezillon:
> On Tue,  5 Jul 2016 00:06:23 +0200
> Richard Weinberger <richard@nod.at> wrote:
> 
>> Just return -EBUSY and everything is fine.
>>
>> Signed-off-by: Richard Weinberger <richard@nod.at>
>> ---
>>  drivers/mtd/mtdcore.c | 6 ++----
>>  1 file changed, 2 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c
>> index f49e103..f2dea88 100644
>> --- a/drivers/mtd/mtdcore.c
>> +++ b/drivers/mtd/mtdcore.c
>> @@ -499,11 +499,9 @@ int del_mtd_device(struct mtd_info *mtd)
>>  		goto out_error;
>>  	}
>>  
>> -	if (mtd->usecount) {
>> -		printk(KERN_NOTICE "Removing MTD device #%d (%s) with use count %d\n",
>> -		       mtd->index, mtd->name, mtd->usecount);
>> +	if (mtd->usecount)
>>  		ret = -EBUSY;
>> -	} else {
>> +	else {
> 
> Nit: can you keep the brackets around the if (mtd->usecount) block (we
> usually only drop them when both the 'if' and 'else' blocks contain
> a single instruction)?
> 

As discussed on IRC, the current kernel coding style is fine with that.
And checkpatch.pl is happy too.

Thanks,
//richard

Re: [PATCH 5/5] mtd: Don't print a scary message when trying to remove a busy MTD

[Boris Brezillon]

On Thu, 7 Jul 2016 20:48:40 +0200
Richard Weinberger <richard@nod.at> wrote:

> Boris,
> 
> Am 07.07.2016 um 16:58 schrieb Boris Brezillon:
> > On Tue,  5 Jul 2016 00:06:23 +0200
> > Richard Weinberger <richard@nod.at> wrote:
> >   
> >> Just return -EBUSY and everything is fine.
> >>
> >> Signed-off-by: Richard Weinberger <richard@nod.at>
> >> ---
> >>  drivers/mtd/mtdcore.c | 6 ++----
> >>  1 file changed, 2 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c
> >> index f49e103..f2dea88 100644
> >> --- a/drivers/mtd/mtdcore.c
> >> +++ b/drivers/mtd/mtdcore.c
> >> @@ -499,11 +499,9 @@ int del_mtd_device(struct mtd_info *mtd)
> >>  		goto out_error;
> >>  	}
> >>  
> >> -	if (mtd->usecount) {
> >> -		printk(KERN_NOTICE "Removing MTD device #%d (%s) with use count %d\n",
> >> -		       mtd->index, mtd->name, mtd->usecount);
> >> +	if (mtd->usecount)
> >>  		ret = -EBUSY;
> >> -	} else {
> >> +	else {  
> > 
> > Nit: can you keep the brackets around the if (mtd->usecount) block (we
> > usually only drop them when both the 'if' and 'else' blocks contain
> > a single instruction)?
> >   
> 
> As discussed on IRC, the current kernel coding style is fine with that.

That's not what's described in Documentation/CodingStyle [1], but for
reason checkpatch is not complaining (I thought it was, but maybe this
rule has been relaxed).

I still find it clearer to have brackets in this case, but I'll let
Brian decide.

[1]http://lxr.free-electrons.com/source/Documentation/CodingStyle#L168

Re: [PATCH 5/5] mtd: Don't print a scary message when trying to remove a busy MTD

[Richard Weinberger]

Boris,

Am 07.07.2016 um 20:53 schrieb Boris Brezillon:
>>> Nit: can you keep the brackets around the if (mtd->usecount) block (we
>>> usually only drop them when both the 'if' and 'else' blocks contain
>>> a single instruction)?
>>>   
>>
>> As discussed on IRC, the current kernel coding style is fine with that.
> 
> That's not what's described in Documentation/CodingStyle [1], but for
> reason checkpatch is not complaining (I thought it was, but maybe this
> rule has been relaxed).
> 
> I still find it clearer to have brackets in this case, but I'll let
> Brian decide.
> 
> [1]http://lxr.free-electrons.com/source/Documentation/CodingStyle#L168

I don't care much, if you prefer it the other way I'll happily change it.
So, please await v2. :-)

Thanks,
//richard

Re: [PATCH 2/5] mtd: nand: Propagate mtd_device_unregister() return value in tear down

[Boris Brezillon]

On Thu, 7 Jul 2016 20:29:24 +0200
Richard Weinberger <richard@nod.at> wrote:

> Boris,
> 
> Am 06.07.2016 um 15:34 schrieb Boris Brezillon:
> >>  /**
> >> - * nand_release - [NAND Interface] Free resources held by the NAND device
> >> + * __nand_release - [NAND Interface] Free resources held by the NAND device
> >>   * @mtd: MTD device structure
> >>   */
> >> -void nand_release(struct mtd_info *mtd)
> >> +int __nand_release(struct mtd_info *mtd)  
> > 
> > Can we find a better name? nand_release_safe()?  
> 
> Sure. Let's pick nand_release_safe().
> 
> >>  {
> >> +	int ret;
> >>  	struct nand_chip *chip = mtd_to_nand(mtd);
> >>  
> >> +	ret = mtd_device_unregister(mtd);
> >> +	if (ret)
> >> +		return ret;
> >> +  
> > 
> > The question is, should we unregister the MTD device in nand_release().
> > It feels a bit odd to have nand_scan_xxx() functions only doing the
> > nand_chip initialization and letting the NAND controller driver
> > register the MTD device, and have nand_release() unregister the MTD
> > device for us.
> > 
> > Maybe we should export a nand_cleanup() function that would just
> > release nand_chip resources and let NAND controller drivers that
> > really care about mtd_device_unregister() return code call it
> > themselves before calling nand_cleanup() (instead of calling
> > nand_release()).  
> 
> You mean renaming nand_release() to nand_cleanup() and the driver
> has to issue mtd_device_unregister() itself before it is allowed to
> do nand_cleanup(). Yes, that is also an option.
> The only downside is that we have to touch a lot of drivers then.
> But the conversion should be almost trivial.

No, I meant adding nand_cleanup() and then patching nand_release() to
first calling mtd_device_unregister() and then nand_cleanup(). This way
you get rid of __nand_release() and use mtd_device_unregister() +
nand_cleanup() in drivers that really care about
mtd_device_unregister() return code.

Re: [PATCH 3/5] mtd: Don't unconditionally unregister reboot notifier

[Brian Norris]

On Tue, Jul 05, 2016 at 12:06:21AM +0200, Richard Weinberger wrote:
> del_mtd_device() is allowed to fail.
> i.e. when the MTD is busy.
> Unregister the reboot notifier only when we're really
> about to delete the MTD.
> 
> Signed-off-by: Richard Weinberger <richard@nod.at>
> ---
>  drivers/mtd/mtdcore.c | 15 ++++++++++-----
>  1 file changed, 10 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c
> index e3936b8..36e5fb0 100644
> --- a/drivers/mtd/mtdcore.c
> +++ b/drivers/mtd/mtdcore.c
> @@ -654,17 +654,22 @@ 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;
>  
>  	if (!device_is_registered(&master->dev))
> -		return 0;
> +		goto unregister;
>  
> -	return del_mtd_device(master);
> +	err = del_mtd_device(master);
> +	if (err)
> +		return err;

Is there any kind of race issue with unregistering the notifier *after*
we've deleted the device? I had intentionally unregistered first,
because I didn't want any chance of the driver/module and/or data
structures being freed before we call the notifier.

I can't think of any particular issue yet, but I wanted to ask.

Brian

> +
> +unregister:
> +	if (master->_reboot)
> +		unregister_reboot_notifier(&master->reboot_notifier);
> +
> +	return 0;
>  }
>  EXPORT_SYMBOL_GPL(mtd_device_unregister);
>  
> -- 
> 2.7.3
> 

Re: [PATCH 4/5] mtd: Don't unconditionally execute remove notifiers

[Brian Norris]

On Tue, Jul 05, 2016 at 12:06:22AM +0200, Richard Weinberger wrote:
> Only call them when we're really removing the MTD.
> 
> Signed-off-by: Richard Weinberger <richard@nod.at>
> ---
>  drivers/mtd/mtdcore.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c
> index 36e5fb0..f49e103 100644
> --- a/drivers/mtd/mtdcore.c
> +++ b/drivers/mtd/mtdcore.c
> @@ -499,16 +499,16 @@ int del_mtd_device(struct mtd_info *mtd)
>  		goto out_error;
>  	}
>  
> -	/* No need to get a refcount on the module containing
> -		the notifier, since we hold the mtd_table_mutex */
> -	list_for_each_entry(not, &mtd_notifiers, list)
> -		not->remove(mtd);
> -
>  	if (mtd->usecount) {
>  		printk(KERN_NOTICE "Removing MTD device #%d (%s) with use count %d\n",
>  		       mtd->index, mtd->name, mtd->usecount);
>  		ret = -EBUSY;
>  	} else {
> +		/* No need to get a refcount on the module containing
> +			the notifier, since we hold the mtd_table_mutex */
> +		list_for_each_entry(not, &mtd_notifiers, list)
> +			not->remove(mtd);
> +

The ordering looks like it was somewhat intentional here:

75c0b84d41c6 mtd: call the remove notifiers before assuming it is in use

Have we fixed or refactored the issues that seem to have prompted that?

Brian


>  		device_unregister(&mtd->dev);
>  
>  		idr_remove(&mtd_idr, mtd->index);
> -- 
> 2.7.3
>