[PATCH v4 0/3] Improve MTD dynamic partitioning support

[PATCH v4 0/3] Improve MTD dynamic partitioning support

[Dan Ehrenberg]

The goal of this patchset is to make mtd dynamic partitioning work
better.
- First, a new config option is added to allow the master device to
  coexist with partitions, which allows partitions to be created or
  removed based on the master even if other partitions exist.
- Second, to identify partitions and improve visibility, 
- Third, remove overlap checks for partitions, which had a bug
  in them and don't seem necessary since static partitions don't
  have overlap checks.

This patchset has gone through a few iterations.

v1 is based on a different concept, where a separate partition is
used as a master, and partitioning is simply made more flexible.

v2 switched to the model of the master coexisting with the partitions.

v3 brings back the patch to remove overlap checks, as testing
discovered that the overlap checks were buggy. This version
also makes some style improvements.

v4 fixes some style issues in the sysfs patch and adds documentation.

Dan Ehrenberg (3):
  mtd: part: Create the master device node when partitioned
  mtd: part: Add sysfs variable for offset of partition
  mtd: part: Remove partition overlap checks

 Documentation/ABI/testing/sysfs-class-mtd | 10 +++++
 drivers/mtd/Kconfig                       | 13 +++++++
 drivers/mtd/mtdcore.c                     | 52 +++++++++++++++++--------
 drivers/mtd/mtdpart.c                     | 64 +++++++++++++++++++++----------
 4 files changed, 102 insertions(+), 37 deletions(-)

-- 
2.2.0.rc0.207.ga3a616c

[PATCH v4 1/3] mtd: part: Create the master device node when partitioned

[Dan Ehrenberg]

For many use cases, it helps to have a device node for the entire
MTD device as well as device nodes for the individual partitions.
For example, this allows querying the entire device's properties.
A common idiom is to create an additional partition which spans
over the whole device.

This patch makes a config option, CONFIG_MTD_PARTITIONED_MASTER,
which makes the master partition present even when the device is
partitioned. This isn't turned on by default since it presents
a backwards-incompatible device numbering.

The patch also makes the parent of a partition device be the master,
if the config flag is set, now that the master is a full device.

Signed-off-by: Dan Ehrenberg <dehrenberg@chromium.org>
---
 drivers/mtd/Kconfig   | 13 +++++++++++++
 drivers/mtd/mtdcore.c | 52 +++++++++++++++++++++++++++++++++++----------------
 drivers/mtd/mtdpart.c | 18 +++++++++++++-----
 3 files changed, 62 insertions(+), 21 deletions(-)

diff --git a/drivers/mtd/Kconfig b/drivers/mtd/Kconfig
index 71fea89..a03ad29 100644
--- a/drivers/mtd/Kconfig
+++ b/drivers/mtd/Kconfig
@@ -309,6 +309,19 @@ config MTD_SWAP
 	  The driver provides wear leveling by storing erase counter into the
 	  OOB.
 
+config MTD_PARTITIONED_MASTER
+	bool "Retain master device when partitioned"
+	default n
+	depends on MTD
+	help
+	  For historical reasons, by default, either a master is present or
+	  several partitions are present, but not both. The concern was that
+	  data listed in multiple partitions was dangerous; however, SCSI does
+	  this and it is frequently useful for applications. This config option
+	  leaves the master in even if the device is partitioned. It also makes
+	  the parent of the partition device be the master device, rather than
+	  what lies behind the master.
+
 source "drivers/mtd/chips/Kconfig"
 
 source "drivers/mtd/maps/Kconfig"
diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c
index 11883bd..d172195 100644
--- a/drivers/mtd/mtdcore.c
+++ b/drivers/mtd/mtdcore.c
@@ -38,6 +38,7 @@
 #include <linux/gfp.h>
 #include <linux/slab.h>
 #include <linux/reboot.h>
+#include <linux/kconfig.h>
 
 #include <linux/mtd/mtd.h>
 #include <linux/mtd/partitions.h>
@@ -501,6 +502,29 @@ out_error:
 	return ret;
 }
 
+static int mtd_add_device_partitions(struct mtd_info *mtd,
+				     struct mtd_partition *real_parts,
+				     int nbparts)
+{
+	int ret;
+
+	if (nbparts == 0 || IS_ENABLED(CONFIG_MTD_PARTITIONED_MASTER)) {
+		ret = add_mtd_device(mtd);
+		if (ret == 1)
+			return -ENODEV;
+	}
+
+	if (nbparts > 0) {
+		ret = add_mtd_partitions(mtd, real_parts, nbparts);
+		if (ret && IS_ENABLED(CONFIG_MTD_PARTITIONED_MASTER))
+			del_mtd_device(mtd);
+		return ret;
+	}
+
+	return 0;
+}
+
+
 /**
  * mtd_device_parse_register - parse partitions and register an MTD device.
  *
@@ -523,7 +547,8 @@ out_error:
  *   found this functions tries to fallback to information specified in
  *   @parts/@nr_parts.
  * * If any partitioning info was found, this function registers the found
- *   partitions.
+ *   partitions. If the MTD_PARTITIONED_MASTER option is set, then the device
+ *   as a whole is registered first.
  * * If no partitions were found this function just registers the MTD device
  *   @mtd and exits.
  *
@@ -534,27 +559,21 @@ int mtd_device_parse_register(struct mtd_info *mtd, const char * const *types,
 			      const struct mtd_partition *parts,
 			      int nr_parts)
 {
-	int err;
-	struct mtd_partition *real_parts;
+	int ret;
+	struct mtd_partition *real_parts = NULL;
 
-	err = parse_mtd_partitions(mtd, types, &real_parts, parser_data);
-	if (err <= 0 && nr_parts && parts) {
+	ret = parse_mtd_partitions(mtd, types, &real_parts, parser_data);
+	if (ret <= 0 && nr_parts && parts) {
 		real_parts = kmemdup(parts, sizeof(*parts) * nr_parts,
 				     GFP_KERNEL);
 		if (!real_parts)
-			err = -ENOMEM;
+			ret = -ENOMEM;
 		else
-			err = nr_parts;
+			ret = nr_parts;
 	}
 
-	if (err > 0) {
-		err = add_mtd_partitions(mtd, real_parts, err);
-		kfree(real_parts);
-	} else if (err == 0) {
-		err = add_mtd_device(mtd);
-		if (err == 1)
-			err = -ENODEV;
-	}
+	if (ret >= 0)
+		ret = mtd_add_device_partitions(mtd, real_parts, ret);
 
 	/*
 	 * FIXME: some drivers unfortunately call this function more than once.
@@ -569,7 +588,8 @@ int mtd_device_parse_register(struct mtd_info *mtd, const char * const *types,
 		register_reboot_notifier(&mtd->reboot_notifier);
 	}
 
-	return err;
+	kfree(real_parts);
+	return ret;
 }
 EXPORT_SYMBOL_GPL(mtd_device_parse_register);
 
diff --git a/drivers/mtd/mtdpart.c b/drivers/mtd/mtdpart.c
index e779de3..a19ec5a 100644
--- a/drivers/mtd/mtdpart.c
+++ b/drivers/mtd/mtdpart.c
@@ -30,6 +30,7 @@
 #include <linux/mtd/mtd.h>
 #include <linux/mtd/partitions.h>
 #include <linux/err.h>
+#include <linux/kconfig.h>
 
 #include "mtdcore.h"
 
@@ -379,10 +380,17 @@ static struct mtd_part *allocate_partition(struct mtd_info *master,
 	slave->mtd.name = name;
 	slave->mtd.owner = master->owner;
 
-	/* NOTE:  we don't arrange MTDs as a tree; it'd be error-prone
-	 * to have the same data be in two different partitions.
+	/* NOTE: Historically, we didn't arrange MTDs as a tree out of
+	 * concern for showing the same data in multiple partitions.
+	 * However, it is very useful to have the master node present,
+	 * so the MTD_PARTITIONED_MASTER option allows that. The master
+	 * will have device nodes etc only if this is set, so make the
+	 * parent conditional on that option. Note, this is a way to
+	 * distinguish between the master and the partition in sysfs.
 	 */
-	slave->mtd.dev.parent = master->dev.parent;
+	slave->mtd.dev.parent = IS_ENABLED(CONFIG_MTD_PARTITIONED_MASTER) ?
+				&master->dev :
+				master->dev.parent;
 
 	slave->mtd._read = part_read;
 	slave->mtd._write = part_write;
@@ -631,8 +639,8 @@ EXPORT_SYMBOL_GPL(mtd_del_partition);
  * and registers slave MTD objects which are bound to the master according to
  * the partition definitions.
  *
- * We don't register the master, or expect the caller to have done so,
- * for reasons of data integrity.
+ * For historical reasons, this function's caller only registers the master
+ * if the MTD_PARTITIONED_MASTER config option is set.
  */
 
 int add_mtd_partitions(struct mtd_info *master,
-- 
2.2.0.rc0.207.ga3a616c

[PATCH v4 2/3] mtd: part: Add sysfs variable for offset of partition

[Dan Ehrenberg]

This patch makes a sysfs variable called 'offset' on each partition
which contains the offset in bytes from the beginning of the master
device that the partition starts.

Signed-off-by: Dan Ehrenberg <dehrenberg@chromium.org>
---
 Documentation/ABI/testing/sysfs-class-mtd | 10 ++++++++++
 drivers/mtd/mtdpart.c                     | 29 +++++++++++++++++++++++++++++
 2 files changed, 39 insertions(+)

diff --git a/Documentation/ABI/testing/sysfs-class-mtd b/Documentation/ABI/testing/sysfs-class-mtd
index 76ee192..3b5c3bc 100644
--- a/Documentation/ABI/testing/sysfs-class-mtd
+++ b/Documentation/ABI/testing/sysfs-class-mtd
@@ -222,3 +222,13 @@ Description:
 		The number of blocks that are marked as reserved, if any, in
 		this partition. These are typically used to store the in-flash
 		bad block table (BBT).
+
+What:		/sys/class/mtd/mtdX/offset
+Date:		March 2015
+KernelVersion:	4.1
+Contact:	linux-mtd@lists.infradead.org
+Description:
+		For a partition, the offset of that partition from the start
+		of the master device in bytes. This attribute is absent on
+		main devices, so it can be used to distinguish between
+		partitions and devices that aren't partitions.
diff --git a/drivers/mtd/mtdpart.c b/drivers/mtd/mtdpart.c
index a19ec5a..be80340 100644
--- a/drivers/mtd/mtdpart.c
+++ b/drivers/mtd/mtdpart.c
@@ -554,6 +554,30 @@ out_register:
 	return slave;
 }
 
+static ssize_t mtd_partition_offset_show(struct device *dev,
+		struct device_attribute *attr, char *buf)
+{
+	struct mtd_info *mtd = dev_get_drvdata(dev);
+	struct mtd_part *part = PART(mtd);
+	return snprintf(buf, PAGE_SIZE, "%lld\n", part->offset);
+}
+
+static DEVICE_ATTR(offset, S_IRUGO, mtd_partition_offset_show, NULL);
+
+static const struct attribute *mtd_partition_attrs[] = {
+	&dev_attr_offset.attr,
+	NULL
+};
+
+static int mtd_add_partition_attrs(struct mtd_part *new)
+{
+	int ret = sysfs_create_files(&new->mtd.dev.kobj, mtd_partition_attrs);
+	if (ret)
+		printk(KERN_WARNING
+		       "mtd: failed to create partition attrs, err=%d\n", ret);
+	return ret;
+}
+
 int mtd_add_partition(struct mtd_info *master, const char *name,
 		      long long offset, long long length)
 {
@@ -603,6 +627,8 @@ int mtd_add_partition(struct mtd_info *master, const char *name,
 
 	add_mtd_device(&new->mtd);
 
+	mtd_add_partition_attrs(new);
+
 	return ret;
 err_inv:
 	mutex_unlock(&mtd_partitions_mutex);
@@ -620,6 +646,8 @@ int mtd_del_partition(struct mtd_info *master, int partno)
 	list_for_each_entry_safe(slave, next, &mtd_partitions, list)
 		if ((slave->master == master) &&
 		    (slave->mtd.index == partno)) {
+			sysfs_remove_files(&slave->mtd.dev.kobj,
+					   mtd_partition_attrs);
 			ret = del_mtd_device(&slave->mtd);
 			if (ret < 0)
 				break;
@@ -663,6 +691,7 @@ int add_mtd_partitions(struct mtd_info *master,
 		mutex_unlock(&mtd_partitions_mutex);
 
 		add_mtd_device(&slave->mtd);
+		mtd_add_partition_attrs(slave);
 
 		cur_offset = slave->offset + slave->mtd.size;
 	}
-- 
2.2.0.rc0.207.ga3a616c

[PATCH v4 3/3] mtd: part: Remove partition overlap checks

[Dan Ehrenberg]

This patch makes MTD dynamic partitioning more flexible by removing
overlap checks for dynamic partitions. I don't see any  particular
reason why overlapping dynamic partitions should be prohibited while
static partitions are allowed to overlap freely.

The checks previously had an off-by-one error, where 'end' should be
one less than what it is currently set at, and adding partitions out of
increasing order will fail. Disabling the checks resolves this issue.

Signed-off-by: Dan Ehrenberg <dehrenberg@chromium.org>
---
 drivers/mtd/mtdpart.c | 17 +----------------
 1 file changed, 1 insertion(+), 16 deletions(-)

diff --git a/drivers/mtd/mtdpart.c b/drivers/mtd/mtdpart.c
index be80340..c444842 100644
--- a/drivers/mtd/mtdpart.c
+++ b/drivers/mtd/mtdpart.c
@@ -582,7 +582,7 @@ int mtd_add_partition(struct mtd_info *master, const char *name,
 		      long long offset, long long length)
 {
 	struct mtd_partition part;
-	struct mtd_part *p, *new;
+	struct mtd_part *new;
 	uint64_t start, end;
 	int ret = 0;
 
@@ -611,17 +611,6 @@ int mtd_add_partition(struct mtd_info *master, const char *name,
 	end = offset + length;
 
 	mutex_lock(&mtd_partitions_mutex);
-	list_for_each_entry(p, &mtd_partitions, list)
-		if (p->master == master) {
-			if ((start >= p->offset) &&
-			    (start < (p->offset + p->mtd.size)))
-				goto err_inv;
-
-			if ((end >= p->offset) &&
-			    (end < (p->offset + p->mtd.size)))
-				goto err_inv;
-		}
-
 	list_add(&new->list, &mtd_partitions);
 	mutex_unlock(&mtd_partitions_mutex);
 
@@ -630,10 +619,6 @@ int mtd_add_partition(struct mtd_info *master, const char *name,
 	mtd_add_partition_attrs(new);
 
 	return ret;
-err_inv:
-	mutex_unlock(&mtd_partitions_mutex);
-	free_partition(new);
-	return -EINVAL;
 }
 EXPORT_SYMBOL_GPL(mtd_add_partition);
 
-- 
2.2.0.rc0.207.ga3a616c

Re: [PATCH v4 3/3] mtd: part: Remove partition overlap checks

[Brian Norris]

On Mon, Mar 30, 2015 at 02:01:38PM -0700, Dan Ehrenberg wrote:
> This patch makes MTD dynamic partitioning more flexible by removing
> overlap checks for dynamic partitions. I don't see any  particular
> reason why overlapping dynamic partitions should be prohibited while
> static partitions are allowed to overlap freely.
> 
> The checks previously had an off-by-one error, where 'end' should be
> one less than what it is currently set at, and adding partitions out of
> increasing order will fail. Disabling the checks resolves this issue.
> 
> Signed-off-by: Dan Ehrenberg <dehrenberg@chromium.org>
> ---
>  drivers/mtd/mtdpart.c | 17 +----------------
>  1 file changed, 1 insertion(+), 16 deletions(-)
> 
> diff --git a/drivers/mtd/mtdpart.c b/drivers/mtd/mtdpart.c
> index be80340..c444842 100644
> --- a/drivers/mtd/mtdpart.c
> +++ b/drivers/mtd/mtdpart.c
> @@ -582,7 +582,7 @@ int mtd_add_partition(struct mtd_info *master, const char *name,
>  		      long long offset, long long length)
>  {
>  	struct mtd_partition part;
> -	struct mtd_part *p, *new;
> +	struct mtd_part *new;
>  	uint64_t start, end;

drivers/mtd/mtdpart.c: In function ‘mtd_add_partition’:
drivers/mtd/mtdpart.c:586:18: warning: variable ‘end’ set but not used [-Wunused-but-set-variable]
  uint64_t start, end;
                  ^
drivers/mtd/mtdpart.c:586:11: warning: variable ‘start’ set but not used [-Wunused-but-set-variable]
  uint64_t start, end;
           ^

>  	int ret = 0;
>  
> @@ -611,17 +611,6 @@ int mtd_add_partition(struct mtd_info *master, const char *name,
>  	end = offset + length;
>  
>  	mutex_lock(&mtd_partitions_mutex);
> -	list_for_each_entry(p, &mtd_partitions, list)
> -		if (p->master == master) {
> -			if ((start >= p->offset) &&
> -			    (start < (p->offset + p->mtd.size)))
> -				goto err_inv;
> -
> -			if ((end >= p->offset) &&
> -			    (end < (p->offset + p->mtd.size)))
> -				goto err_inv;
> -		}
> -
>  	list_add(&new->list, &mtd_partitions);
>  	mutex_unlock(&mtd_partitions_mutex);
>  
> @@ -630,10 +619,6 @@ int mtd_add_partition(struct mtd_info *master, const char *name,
>  	mtd_add_partition_attrs(new);
>  
>  	return ret;
> -err_inv:
> -	mutex_unlock(&mtd_partitions_mutex);
> -	free_partition(new);
> -	return -EINVAL;
>  }
>  EXPORT_SYMBOL_GPL(mtd_add_partition);
>  

Brian