[PATCH 1/4] mtd: diskonchip: remove two-phase partitioning / registration

[PATCH 1/4] mtd: diskonchip: remove two-phase partitioning / registration

[Brian Norris]

It is a Bad Idea (TM) to call mtd_device_register() or
mtd_device_parse_register() twice on the same master MTD. Among other
things, it makes partition overrides (e.g., cmdlinepart) much more
difficult.

Since commit 727dc612c46b ("mtd: part: Create the master device node
when partitioned"), we now have a config option that accomplishes the
same purpose as the double-registration done in diskonchip.c -- it
forces the master MTD to *always* be registered, while partitions may
optionally show up in addition. Eventually, we might like to make
CONFIG_MTD_PARTITIONED_MASTER into the default, but this could be
disruptive to user-space expectations of MTD numbering, so we'll take
that slowly.

Signed-off-by: Brian Norris <computersforpeace@gmail.com>
Cc: Richard Weinberger <richard@nod.at>
Cc: Alexander Shiyan <shc_work@mail.ru>
---
Not tested, as I don't have the hardware

 drivers/mtd/nand/diskonchip.c | 10 ++--------
 1 file changed, 2 insertions(+), 8 deletions(-)

diff --git a/drivers/mtd/nand/diskonchip.c b/drivers/mtd/nand/diskonchip.c
index e5800146cf33..7da266a53979 100644
--- a/drivers/mtd/nand/diskonchip.c
+++ b/drivers/mtd/nand/diskonchip.c
@@ -1301,10 +1301,7 @@ static int __init nftl_scan_bbt(struct mtd_info *mtd)
 	if (ret)
 		return ret;
 
-	mtd_device_register(mtd, NULL, 0);
-	if (!no_autopart)
-		mtd_device_register(mtd, parts, numparts);
-	return 0;
+	return mtd_device_register(mtd, parts, no_autopart ? 0 : numparts);
 }
 
 static int __init inftl_scan_bbt(struct mtd_info *mtd)
@@ -1358,10 +1355,7 @@ static int __init inftl_scan_bbt(struct mtd_info *mtd)
 	   autopartitioning, but I want to give it more thought. */
 	if (!numparts)
 		return -EIO;
-	mtd_device_register(mtd, NULL, 0);
-	if (!no_autopart)
-		mtd_device_register(mtd, parts, numparts);
-	return 0;
+	return mtd_device_register(mtd, parts, no_autopart ? 0 : numparts);
 }
 
 static inline int __init doc2000_init(struct mtd_info *mtd)
-- 
1.9.1

[PATCH 2/4] mtd: propagate error codes from add_mtd_device()

[Brian Norris]

It makes more sense to return error statuses, not 1/0.

At the same time, note a few places where error handling is not
currently done.

Signed-off-by: Brian Norris <computersforpeace@gmail.com>
---
 drivers/mtd/mtdcore.c | 18 +++++++++++-------
 drivers/mtd/mtdpart.c |  2 ++
 2 files changed, 13 insertions(+), 7 deletions(-)

diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c
index f3ca97f139bc..8bbbb751bf45 100644
--- a/drivers/mtd/mtdcore.c
+++ b/drivers/mtd/mtdcore.c
@@ -379,8 +379,7 @@ static int mtd_reboot_notifier(struct notifier_block *n, unsigned long state,
  *
  *	Add a device to the list of MTD devices present in the system, and
  *	notify each currently active MTD 'user' of its arrival. Returns
- *	zero on success or 1 on failure, which currently will only happen
- *	if there is insufficient memory or a sysfs error.
+ *	zero on success or non-zero on failure.
  */
 
 int add_mtd_device(struct mtd_info *mtd)
@@ -394,8 +393,10 @@ int add_mtd_device(struct mtd_info *mtd)
 	mutex_lock(&mtd_table_mutex);
 
 	i = idr_alloc(&mtd_idr, mtd, 0, 0, GFP_KERNEL);
-	if (i < 0)
+	if (i < 0) {
+		error = i;
 		goto fail_locked;
+	}
 
 	mtd->index = i;
 	mtd->usecount = 0;
@@ -424,6 +425,8 @@ int add_mtd_device(struct mtd_info *mtd)
 			printk(KERN_WARNING
 			       "%s: unlock failed, writes may not work\n",
 			       mtd->name);
+		/* Ignore unlock failures? */
+		error = 0;
 	}
 
 	/* Caller should have set dev.parent to match the
@@ -434,7 +437,8 @@ int add_mtd_device(struct mtd_info *mtd)
 	mtd->dev.devt = MTD_DEVT(i);
 	dev_set_name(&mtd->dev, "mtd%d", i);
 	dev_set_drvdata(&mtd->dev, mtd);
-	if (device_register(&mtd->dev) != 0)
+	error = device_register(&mtd->dev);
+	if (error)
 		goto fail_added;
 
 	device_create(&mtd_class, mtd->dev.parent, MTD_DEVT(i) + 1, NULL,
@@ -458,7 +462,7 @@ fail_added:
 	idr_remove(&mtd_idr, i);
 fail_locked:
 	mutex_unlock(&mtd_table_mutex);
-	return 1;
+	return error;
 }
 
 /**
@@ -514,8 +518,8 @@ static int mtd_add_device_partitions(struct mtd_info *mtd,
 
 	if (nbparts == 0 || IS_ENABLED(CONFIG_MTD_PARTITIONED_MASTER)) {
 		ret = add_mtd_device(mtd);
-		if (ret == 1)
-			return -ENODEV;
+		if (ret)
+			return ret;
 	}
 
 	if (nbparts > 0) {
diff --git a/drivers/mtd/mtdpart.c b/drivers/mtd/mtdpart.c
index cafdb8855a79..f7aa944bc6e9 100644
--- a/drivers/mtd/mtdpart.c
+++ b/drivers/mtd/mtdpart.c
@@ -610,6 +610,7 @@ int mtd_add_partition(struct mtd_info *master, const char *name,
 	list_add(&new->list, &mtd_partitions);
 	mutex_unlock(&mtd_partitions_mutex);
 
+	/* FIXME: handle errors */
 	add_mtd_device(&new->mtd);
 
 	mtd_add_partition_attrs(new);
@@ -671,6 +672,7 @@ int add_mtd_partitions(struct mtd_info *master,
 		list_add(&slave->list, &mtd_partitions);
 		mutex_unlock(&mtd_partitions_mutex);
 
+		/* FIXME: handle errors */
 		add_mtd_device(&slave->mtd);
 		mtd_add_partition_attrs(slave);
 
-- 
1.9.1

[PATCH 3/4] mtd: fixup corner case error handling in mtd_device_parse_register()

[Brian Norris]

Since commit 3efe41be224c ("mtd: implement common reboot notifier
boilerplate"), we might try to register a reboot notifier for an MTD
that failed to register. Let's avoid this by making the error path
clearer.

Signed-off-by: Brian Norris <computersforpeace@gmail.com>
---
 drivers/mtd/mtdcore.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c
index 8bbbb751bf45..03eec42dc715 100644
--- a/drivers/mtd/mtdcore.c
+++ b/drivers/mtd/mtdcore.c
@@ -579,9 +579,15 @@ int mtd_device_parse_register(struct mtd_info *mtd, const char * const *types,
 		else
 			ret = nr_parts;
 	}
+	/* Didn't come up with either parsed OR fallback partitions */
+	if (ret < 0) {
+		pr_info("mtd: failed to find partitions\n");
+		goto out;
+	}
 
-	if (ret >= 0)
-		ret = mtd_add_device_partitions(mtd, real_parts, ret);
+	ret = mtd_add_device_partitions(mtd, real_parts, ret);
+	if (ret)
+		goto out;
 
 	/*
 	 * FIXME: some drivers unfortunately call this function more than once.
@@ -596,6 +602,7 @@ int mtd_device_parse_register(struct mtd_info *mtd, const char * const *types,
 		register_reboot_notifier(&mtd->reboot_notifier);
 	}
 
+out:
 	kfree(real_parts);
 	return ret;
 }
-- 
1.9.1

[PATCH 4/4] mtd: warn when registering the same master many times

[Brian Norris]

When CONFIG_MTD_PARTITIONED_MASTER=y, it is fatal to call
mtd_device_parse_register() twice on the same MTD, as we try to register
the same device/kobject multipile times.

When CONFIG_MTD_PARTITIONED_MASTER=n, calling
mtd_device_parse_register() is more of just a nuisance, as we can mostly
navigate around any conflicting actions.

But anyway, doing so is a Bad Thing (TM), and we should complain loudly
for any drivers that try to do this.

Signed-off-by: Brian Norris <computersforpeace@gmail.com>
---
 drivers/mtd/mtdcore.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c
index 03eec42dc715..f0e157e0be89 100644
--- a/drivers/mtd/mtdcore.c
+++ b/drivers/mtd/mtdcore.c
@@ -387,6 +387,14 @@ int add_mtd_device(struct mtd_info *mtd)
 	struct mtd_notifier *not;
 	int i, error;
 
+	/*
+	 * May occur, for instance, on buggy drivers which call
+	 * mtd_device_parse_register() multiple times on the same master MTD,
+	 * especially with CONFIG_MTD_PARTITIONED_MASTER=y.
+	 */
+	if (WARN(mtd->backing_dev_info, "MTD already registered\n"))
+		return -EEXIST;
+
 	mtd->backing_dev_info = &mtd_bdi;
 
 	BUG_ON(mtd->writesize == 0);
@@ -597,6 +605,7 @@ int mtd_device_parse_register(struct mtd_info *mtd, const char * const *types,
 	 * does cause problems with parse_mtd_partitions() above (e.g.,
 	 * cmdlineparts will register partitions more than once).
 	 */
+	WARN(mtd->reboot_notifier.notifier_call, "MTD already registered\n");
 	if (mtd->_reboot && !mtd->reboot_notifier.notifier_call) {
 		mtd->reboot_notifier.notifier_call = mtd_reboot_notifier;
 		register_reboot_notifier(&mtd->reboot_notifier);
-- 
1.9.1

Re: [PATCH 2/4] mtd: propagate error codes from add_mtd_device()

[Richard Weinberger]

Am 02.06.2015 um 01:17 schrieb Brian Norris:
> It makes more sense to return error statuses, not 1/0.
> 
> At the same time, note a few places where error handling is not
> currently done.
> 
> Signed-off-by: Brian Norris <computersforpeace@gmail.com>
> ---
>  drivers/mtd/mtdcore.c | 18 +++++++++++-------
>  drivers/mtd/mtdpart.c |  2 ++
>  2 files changed, 13 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c
> index f3ca97f139bc..8bbbb751bf45 100644
> --- a/drivers/mtd/mtdcore.c
> +++ b/drivers/mtd/mtdcore.c
> @@ -379,8 +379,7 @@ static int mtd_reboot_notifier(struct notifier_block *n, unsigned long state,
>   *
>   *	Add a device to the list of MTD devices present in the system, and
>   *	notify each currently active MTD 'user' of its arrival. Returns
> - *	zero on success or 1 on failure, which currently will only happen
> - *	if there is insufficient memory or a sysfs error.
> + *	zero on success or non-zero on failure.
>   */
>  
>  int add_mtd_device(struct mtd_info *mtd)
> @@ -394,8 +393,10 @@ int add_mtd_device(struct mtd_info *mtd)
>  	mutex_lock(&mtd_table_mutex);
>  
>  	i = idr_alloc(&mtd_idr, mtd, 0, 0, GFP_KERNEL);
> -	if (i < 0)
> +	if (i < 0) {
> +		error = i;
>  		goto fail_locked;
> +	}
>  
>  	mtd->index = i;
>  	mtd->usecount = 0;
> @@ -424,6 +425,8 @@ int add_mtd_device(struct mtd_info *mtd)
>  			printk(KERN_WARNING
>  			       "%s: unlock failed, writes may not work\n",
>  			       mtd->name);
> +		/* Ignore unlock failures? */
> +		error = 0;
>  	}
>  
>  	/* Caller should have set dev.parent to match the
> @@ -434,7 +437,8 @@ int add_mtd_device(struct mtd_info *mtd)
>  	mtd->dev.devt = MTD_DEVT(i);
>  	dev_set_name(&mtd->dev, "mtd%d", i);
>  	dev_set_drvdata(&mtd->dev, mtd);
> -	if (device_register(&mtd->dev) != 0)
> +	error = device_register(&mtd->dev);
> +	if (error)
>  		goto fail_added;
>  
>  	device_create(&mtd_class, mtd->dev.parent, MTD_DEVT(i) + 1, NULL,
> @@ -458,7 +462,7 @@ fail_added:
>  	idr_remove(&mtd_idr, i);
>  fail_locked:
>  	mutex_unlock(&mtd_table_mutex);
> -	return 1;
> +	return error;
>  }
>  
>  /**
> @@ -514,8 +518,8 @@ static int mtd_add_device_partitions(struct mtd_info *mtd,
>  
>  	if (nbparts == 0 || IS_ENABLED(CONFIG_MTD_PARTITIONED_MASTER)) {
>  		ret = add_mtd_device(mtd);
> -		if (ret == 1)
> -			return -ENODEV;
> +		if (ret)
> +			return ret;
>  	}
>  
>  	if (nbparts > 0) {
> diff --git a/drivers/mtd/mtdpart.c b/drivers/mtd/mtdpart.c
> index cafdb8855a79..f7aa944bc6e9 100644
> --- a/drivers/mtd/mtdpart.c
> +++ b/drivers/mtd/mtdpart.c
> @@ -610,6 +610,7 @@ int mtd_add_partition(struct mtd_info *master, const char *name,
>  	list_add(&new->list, &mtd_partitions);
>  	mutex_unlock(&mtd_partitions_mutex);
>  
> +	/* FIXME: handle errors */
>  	add_mtd_device(&new->mtd);
>  
>  	mtd_add_partition_attrs(new);
> @@ -671,6 +672,7 @@ int add_mtd_partitions(struct mtd_info *master,
>  		list_add(&slave->list, &mtd_partitions);
>  		mutex_unlock(&mtd_partitions_mutex);
>  
> +		/* FIXME: handle errors */
>  		add_mtd_device(&slave->mtd);
>  		mtd_add_partition_attrs(slave);
>  
> 

Do you want this FIXME's have fixed in the 4.2 merge window?
Adding new FIXME's is IMHO not nice.

Reviewed-by: Richard Weinberger <richard@nod.at>

Thanks,
//richard

Re: [PATCH 3/4] mtd: fixup corner case error handling in mtd_device_parse_register()

[Richard Weinberger]

Am 02.06.2015 um 01:17 schrieb Brian Norris:
> Since commit 3efe41be224c ("mtd: implement common reboot notifier
> boilerplate"), we might try to register a reboot notifier for an MTD
> that failed to register. Let's avoid this by making the error path
> clearer.
> 
> Signed-off-by: Brian Norris <computersforpeace@gmail.com>
> ---
>  drivers/mtd/mtdcore.c | 11 +++++++++--
>  1 file changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c
> index 8bbbb751bf45..03eec42dc715 100644
> --- a/drivers/mtd/mtdcore.c
> +++ b/drivers/mtd/mtdcore.c
> @@ -579,9 +579,15 @@ int mtd_device_parse_register(struct mtd_info *mtd, const char * const *types,
>  		else
>  			ret = nr_parts;
>  	}
> +	/* Didn't come up with either parsed OR fallback partitions */
> +	if (ret < 0) {
> +		pr_info("mtd: failed to find partitions\n");
> +		goto out;
> +	}
>  
> -	if (ret >= 0)
> -		ret = mtd_add_device_partitions(mtd, real_parts, ret);
> +	ret = mtd_add_device_partitions(mtd, real_parts, ret);
> +	if (ret)
> +		goto out;
>  
>  	/*
>  	 * FIXME: some drivers unfortunately call this function more than once.
> @@ -596,6 +602,7 @@ int mtd_device_parse_register(struct mtd_info *mtd, const char * const *types,
>  		register_reboot_notifier(&mtd->reboot_notifier);
>  	}
>  
> +out:
>  	kfree(real_parts);
>  	return ret;
>  }

Reviewed-by: Richard Weinberger <richard@nod.at>

Thanks,
//richard

Re: [PATCH 4/4] mtd: warn when registering the same master many times

[Richard Weinberger]

Am 02.06.2015 um 01:17 schrieb Brian Norris:
> When CONFIG_MTD_PARTITIONED_MASTER=y, it is fatal to call
> mtd_device_parse_register() twice on the same MTD, as we try to register
> the same device/kobject multipile times.
> 
> When CONFIG_MTD_PARTITIONED_MASTER=n, calling
> mtd_device_parse_register() is more of just a nuisance, as we can mostly
> navigate around any conflicting actions.
> 
> But anyway, doing so is a Bad Thing (TM), and we should complain loudly
> for any drivers that try to do this.
> 
> Signed-off-by: Brian Norris <computersforpeace@gmail.com>
> ---
>  drivers/mtd/mtdcore.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c
> index 03eec42dc715..f0e157e0be89 100644
> --- a/drivers/mtd/mtdcore.c
> +++ b/drivers/mtd/mtdcore.c
> @@ -387,6 +387,14 @@ int add_mtd_device(struct mtd_info *mtd)
>  	struct mtd_notifier *not;
>  	int i, error;
>  
> +	/*
> +	 * May occur, for instance, on buggy drivers which call
> +	 * mtd_device_parse_register() multiple times on the same master MTD,
> +	 * especially with CONFIG_MTD_PARTITIONED_MASTER=y.
> +	 */
> +	if (WARN(mtd->backing_dev_info, "MTD already registered\n"))
> +		return -EEXIST;
> +
>  	mtd->backing_dev_info = &mtd_bdi;
>  
>  	BUG_ON(mtd->writesize == 0);
> @@ -597,6 +605,7 @@ int mtd_device_parse_register(struct mtd_info *mtd, const char * const *types,
>  	 * does cause problems with parse_mtd_partitions() above (e.g.,
>  	 * cmdlineparts will register partitions more than once).
>  	 */
> +	WARN(mtd->reboot_notifier.notifier_call, "MTD already registered\n");
>  	if (mtd->_reboot && !mtd->reboot_notifier.notifier_call) {
>  		mtd->reboot_notifier.notifier_call = mtd_reboot_notifier;
>  		register_reboot_notifier(&mtd->reboot_notifier);

One minor nit, IMHO WARN_ONCE() would make more sense.

Beside of that:
Reviewed-by: Richard Weinberger <richard@nod.at>

Thanks,
//richard

Re: [PATCH 2/4] mtd: propagate error codes from add_mtd_device()

[Brian Norris]

On Tue, Jun 02, 2015 at 10:02:59AM +0200, Richard Weinberger wrote:
> Am 02.06.2015 um 01:17 schrieb Brian Norris:
> > It makes more sense to return error statuses, not 1/0.
> > 
> > At the same time, note a few places where error handling is not
> > currently done.
> > 
> > Signed-off-by: Brian Norris <computersforpeace@gmail.com>
> > ---
> >  drivers/mtd/mtdcore.c | 18 +++++++++++-------
> >  drivers/mtd/mtdpart.c |  2 ++
> >  2 files changed, 13 insertions(+), 7 deletions(-)
> > 
> > diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c
> > index f3ca97f139bc..8bbbb751bf45 100644
> > --- a/drivers/mtd/mtdcore.c
> > +++ b/drivers/mtd/mtdcore.c
> > @@ -379,8 +379,7 @@ static int mtd_reboot_notifier(struct notifier_block *n, unsigned long state,
> >   *
> >   *	Add a device to the list of MTD devices present in the system, and
> >   *	notify each currently active MTD 'user' of its arrival. Returns
> > - *	zero on success or 1 on failure, which currently will only happen
> > - *	if there is insufficient memory or a sysfs error.
> > + *	zero on success or non-zero on failure.
> >   */
> >  
> >  int add_mtd_device(struct mtd_info *mtd)
> > @@ -394,8 +393,10 @@ int add_mtd_device(struct mtd_info *mtd)
> >  	mutex_lock(&mtd_table_mutex);
> >  
> >  	i = idr_alloc(&mtd_idr, mtd, 0, 0, GFP_KERNEL);
> > -	if (i < 0)
> > +	if (i < 0) {
> > +		error = i;
> >  		goto fail_locked;
> > +	}
> >  
> >  	mtd->index = i;
> >  	mtd->usecount = 0;
> > @@ -424,6 +425,8 @@ int add_mtd_device(struct mtd_info *mtd)
> >  			printk(KERN_WARNING
> >  			       "%s: unlock failed, writes may not work\n",
> >  			       mtd->name);
> > +		/* Ignore unlock failures? */
> > +		error = 0;
> >  	}
> >  
> >  	/* Caller should have set dev.parent to match the
> > @@ -434,7 +437,8 @@ int add_mtd_device(struct mtd_info *mtd)
> >  	mtd->dev.devt = MTD_DEVT(i);
> >  	dev_set_name(&mtd->dev, "mtd%d", i);
> >  	dev_set_drvdata(&mtd->dev, mtd);
> > -	if (device_register(&mtd->dev) != 0)
> > +	error = device_register(&mtd->dev);
> > +	if (error)
> >  		goto fail_added;
> >  
> >  	device_create(&mtd_class, mtd->dev.parent, MTD_DEVT(i) + 1, NULL,
> > @@ -458,7 +462,7 @@ fail_added:
> >  	idr_remove(&mtd_idr, i);
> >  fail_locked:
> >  	mutex_unlock(&mtd_table_mutex);
> > -	return 1;
> > +	return error;
> >  }
> >  
> >  /**
> > @@ -514,8 +518,8 @@ static int mtd_add_device_partitions(struct mtd_info *mtd,
> >  
> >  	if (nbparts == 0 || IS_ENABLED(CONFIG_MTD_PARTITIONED_MASTER)) {
> >  		ret = add_mtd_device(mtd);
> > -		if (ret == 1)
> > -			return -ENODEV;
> > +		if (ret)
> > +			return ret;
> >  	}
> >  
> >  	if (nbparts > 0) {
> > diff --git a/drivers/mtd/mtdpart.c b/drivers/mtd/mtdpart.c
> > index cafdb8855a79..f7aa944bc6e9 100644
> > --- a/drivers/mtd/mtdpart.c
> > +++ b/drivers/mtd/mtdpart.c
> > @@ -610,6 +610,7 @@ int mtd_add_partition(struct mtd_info *master, const char *name,
> >  	list_add(&new->list, &mtd_partitions);
> >  	mutex_unlock(&mtd_partitions_mutex);
> >  
> > +	/* FIXME: handle errors */
> >  	add_mtd_device(&new->mtd);
> >  
> >  	mtd_add_partition_attrs(new);
> > @@ -671,6 +672,7 @@ int add_mtd_partitions(struct mtd_info *master,
> >  		list_add(&slave->list, &mtd_partitions);
> >  		mutex_unlock(&mtd_partitions_mutex);
> >  
> > +		/* FIXME: handle errors */
> >  		add_mtd_device(&slave->mtd);
> >  		mtd_add_partition_attrs(slave);
> >  
> > 
> 
> Do you want this FIXME's have fixed in the 4.2 merge window?
> Adding new FIXME's is IMHO not nice.

Not sure. I really wasn't planning on it, and I admit this was a little
lazy of me.

Now, we're really in no worse of a condition regarding error-handling,
as this was already ignoring error-handling, and to drop this patch
entirely actually masks another class of bugs. But I am adding a new
return condition that would make this deficiency more important in the
next patches...

So which of these options do you like best?
1. take this patch as-is and
   a. fix them in the 4.2 cycle
   b. fix them "sometime" (i.e., never)
2. take this patch without the FIXME's
3. don't take this patch until the FIXME's are resolved

Personally, I guess I'd lean toward #2, and then don't take the next
patches that add new error return cases until I can work out all the
other deficient code paths.

> Reviewed-by: Richard Weinberger <richard@nod.at>

Thanks!

Brian

Re: [PATCH 1/4] mtd: diskonchip: remove two-phase partitioning / registration

[Brian Norris]

On Mon, Jun 01, 2015 at 04:17:17PM -0700, Brian Norris wrote:
> It is a Bad Idea (TM) to call mtd_device_register() or
> mtd_device_parse_register() twice on the same master MTD. Among other
> things, it makes partition overrides (e.g., cmdlinepart) much more
> difficult.
> 
> Since commit 727dc612c46b ("mtd: part: Create the master device node
> when partitioned"), we now have a config option that accomplishes the
> same purpose as the double-registration done in diskonchip.c -- it
> forces the master MTD to *always* be registered, while partitions may
> optionally show up in addition. Eventually, we might like to make
> CONFIG_MTD_PARTITIONED_MASTER into the default, but this could be
> disruptive to user-space expectations of MTD numbering, so we'll take
> that slowly.
> 
> Signed-off-by: Brian Norris <computersforpeace@gmail.com>
> Cc: Richard Weinberger <richard@nod.at>
> Cc: Alexander Shiyan <shc_work@mail.ru>

Pushed patch 1 and 2, minus the FIXME's.

Re: [PATCH 3/4] mtd: fixup corner case error handling in mtd_device_parse_register()

[Brian Norris]

On Tue, Jun 02, 2015 at 10:03:05AM +0200, Richard Weinberger wrote:
> Am 02.06.2015 um 01:17 schrieb Brian Norris:
> > Since commit 3efe41be224c ("mtd: implement common reboot notifier
> > boilerplate"), we might try to register a reboot notifier for an MTD
> > that failed to register. Let's avoid this by making the error path
> > clearer.
> > 
> > Signed-off-by: Brian Norris <computersforpeace@gmail.com>
> > ---
> >  drivers/mtd/mtdcore.c | 11 +++++++++--
> >  1 file changed, 9 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c
> > index 8bbbb751bf45..03eec42dc715 100644
> > --- a/drivers/mtd/mtdcore.c
> > +++ b/drivers/mtd/mtdcore.c
> > @@ -579,9 +579,15 @@ int mtd_device_parse_register(struct mtd_info *mtd, const char * const *types,
> >  		else
> >  			ret = nr_parts;
> >  	}
> > +	/* Didn't come up with either parsed OR fallback partitions */
> > +	if (ret < 0) {
> > +		pr_info("mtd: failed to find partitions\n");
> > +		goto out;
> > +	}
> >  
> > -	if (ret >= 0)
> > -		ret = mtd_add_device_partitions(mtd, real_parts, ret);
> > +	ret = mtd_add_device_partitions(mtd, real_parts, ret);
> > +	if (ret)
> > +		goto out;
> >  
> >  	/*
> >  	 * FIXME: some drivers unfortunately call this function more than once.
> > @@ -596,6 +602,7 @@ int mtd_device_parse_register(struct mtd_info *mtd, const char * const *types,
> >  		register_reboot_notifier(&mtd->reboot_notifier);
> >  	}
> >  
> > +out:
> >  	kfree(real_parts);
> >  	return ret;
> >  }
> 
> Reviewed-by: Richard Weinberger <richard@nod.at>

Not sure why I left this one; applied to l2-mtd.git

Re: [PATCH 4/4] mtd: warn when registering the same master many times

[Brian Norris]

On Tue, Jun 02, 2015 at 10:03:10AM +0200, Richard Weinberger wrote:
> Am 02.06.2015 um 01:17 schrieb Brian Norris:
> > When CONFIG_MTD_PARTITIONED_MASTER=y, it is fatal to call
> > mtd_device_parse_register() twice on the same MTD, as we try to register
> > the same device/kobject multipile times.
> > 
> > When CONFIG_MTD_PARTITIONED_MASTER=n, calling
> > mtd_device_parse_register() is more of just a nuisance, as we can mostly
> > navigate around any conflicting actions.
> > 
> > But anyway, doing so is a Bad Thing (TM), and we should complain loudly
> > for any drivers that try to do this.
> > 
> > Signed-off-by: Brian Norris <computersforpeace@gmail.com>
> > ---
> >  drivers/mtd/mtdcore.c | 9 +++++++++
> >  1 file changed, 9 insertions(+)
> > 
> > diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c
> > index 03eec42dc715..f0e157e0be89 100644
> > --- a/drivers/mtd/mtdcore.c
> > +++ b/drivers/mtd/mtdcore.c
> > @@ -387,6 +387,14 @@ int add_mtd_device(struct mtd_info *mtd)
> >  	struct mtd_notifier *not;
> >  	int i, error;
> >  
> > +	/*
> > +	 * May occur, for instance, on buggy drivers which call
> > +	 * mtd_device_parse_register() multiple times on the same master MTD,
> > +	 * especially with CONFIG_MTD_PARTITIONED_MASTER=y.
> > +	 */
> > +	if (WARN(mtd->backing_dev_info, "MTD already registered\n"))
> > +		return -EEXIST;
> > +
> >  	mtd->backing_dev_info = &mtd_bdi;
> >  
> >  	BUG_ON(mtd->writesize == 0);
> > @@ -597,6 +605,7 @@ int mtd_device_parse_register(struct mtd_info *mtd, const char * const *types,
> >  	 * does cause problems with parse_mtd_partitions() above (e.g.,
> >  	 * cmdlineparts will register partitions more than once).
> >  	 */
> > +	WARN(mtd->reboot_notifier.notifier_call, "MTD already registered\n");
> >  	if (mtd->_reboot && !mtd->reboot_notifier.notifier_call) {
> >  		mtd->reboot_notifier.notifier_call = mtd_reboot_notifier;
> >  		register_reboot_notifier(&mtd->reboot_notifier);
> 
> One minor nit, IMHO WARN_ONCE() would make more sense.
> 
> Beside of that:
> Reviewed-by: Richard Weinberger <richard@nod.at>

Changed to WARN_ONCE() and applied