mtd: diskonchip: remove two-phase partitioning / registration

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 <[email protected]>

---
 drivers/mtd/mtdcore.c | 18 +++++++++++-------
 drivers/mtd/mtdpart.c |  2 ++
 2 files changed, 13 insertions(+), 7 deletions(-)

drivers/mtd/mtdcore.c +11 -7

index f3ca97f139bc..8bbbb751bf45 100644
@@ -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) {

drivers/mtd/mtdpart.c +1 -0

index cafdb8855a79..f7aa944bc6e9 100644
@@ -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);

Am 02.06.2015 um 01:17 schrieb Brian Norris:

Quoted 92 lines

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 <[email protected]>

---
 drivers/mtd/mtdcore.c | 18 +++++++++++-------
 drivers/mtd/mtdpart.c |  2 ++
 2 files changed, 13 insertions(+), 7 deletions(-)

drivers/mtd/mtdcore.c +11 -7

index f3ca97f139bc..8bbbb751bf45 100644
@@ -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) {

drivers/mtd/mtdpart.c +1 -0

index cafdb8855a79..f7aa944bc6e9 100644
@@ -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);
 

drivers/mtd/mtdpart.c int add_mtd_partitions(struct mtd_info *master, · line 678

Quoted 5 lines

drivers/mtd/mtdpart.c +1 -0

+		/* 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 <[email protected]>

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 <[email protected]>

---
 drivers/mtd/mtdcore.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

drivers/mtd/mtdcore.c +8 -2

index 8bbbb751bf45..03eec42dc715 100644
@@ -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.

Am 02.06.2015 um 01:17 schrieb Brian Norris:

Quoted 36 lines

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 <[email protected]>

---
 drivers/mtd/mtdcore.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

drivers/mtd/mtdcore.c +8 -2

index 8bbbb751bf45..03eec42dc715 100644
@@ -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);
 	}
 

drivers/mtd/mtdcore.c int mtd_device_parse_register(struct mtd_info *mtd, const char * const *types, · line 608

Quoted 4 lines

drivers/mtd/mtdcore.c +1 -0

+out:
 	kfree(real_parts);
 	return ret;
 }

Reviewed-by Richard Weinberger <[email protected]>

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 <[email protected]>

---
 drivers/mtd/mtdcore.c | 9 +++++++++
 1 file changed, 9 insertions(+)

drivers/mtd/mtdcore.c +8 -0

index 03eec42dc715..f0e157e0be89 100644
@@ -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);

Am 02.06.2015 um 01:17 schrieb Brian Norris:

Quoted 36 lines

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 <[email protected]>

---
 drivers/mtd/mtdcore.c | 9 +++++++++
 1 file changed, 9 insertions(+)

drivers/mtd/mtdcore.c +8 -0

index 03eec42dc715..f0e157e0be89 100644
@@ -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);

drivers/mtd/mtdcore.c int mtd_device_parse_register(struct mtd_info *mtd, const char * const *types, · line 611

Quoted 7 lines

drivers/mtd/mtdcore.c +1 -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 <[email protected]>

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 <[email protected]>

Cc Richard Weinberger <[email protected]>

Cc Alexander Shiyan <[email protected]>