[PATCH v2 1/3] mtdpart: Allow adding a partition of a partition

[PATCH v2 1/3] mtdpart: Allow adding a partition of a partition

[Dan Ehrenberg]

MTD allows dynamic partitioning by the BLKPG ioctl.
The current code restricts partition dynamic addition to work only on
the master MTD device. This doesn't make a lot of sense, and is
impossible to meet if the device is already partitioned (since the
master MTD device is not visible). This commit removes the restriction.

Signed-off-by: Dan Ehrenberg <dehrenberg@chromium.org>
---
 drivers/mtd/mtdchar.c | 4 ----
 drivers/mtd/mtdpart.c | 6 ++++++
 2 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/mtd/mtdchar.c b/drivers/mtd/mtdchar.c
index 5356395..30215c7 100644
--- a/drivers/mtd/mtdchar.c
+++ b/drivers/mtd/mtdchar.c
@@ -545,10 +545,6 @@ static int mtdchar_blkpg_ioctl(struct mtd_info *mtd,
 	switch (a.op) {
 	case BLKPG_ADD_PARTITION:
 
-		/* Only master mtd device must be used to add partitions */
-		if (mtd_is_partition(mtd))
-			return -EINVAL;
-
 		/* Sanitize user input */
 		p.devname[BLKPG_DEVNAMELTH - 1] = '\0';
 
diff --git a/drivers/mtd/mtdpart.c b/drivers/mtd/mtdpart.c
index a3e3a7d..7c6f526 100644
--- a/drivers/mtd/mtdpart.c
+++ b/drivers/mtd/mtdpart.c
@@ -566,6 +566,12 @@ int mtd_add_partition(struct mtd_info *master, const char *name,
 	if (length <= 0)
 		return -EINVAL;
 
+	if (mtd_is_partition(master)) {
+		struct mtd_part *master_partition = PART(master);
+		offset += master_partition->offset;
+		master = master_partition->master;
+	}
+
 	part.name = name;
 	part.size = length;
 	part.offset = offset;
-- 
2.2.0.rc0.207.ga3a616c

[PATCH v2 2/3] mtdpart: 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, and this is useful
for users who want one additional partition to span over the whole
device.

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 7c6f526..7eb9083 100644
--- a/drivers/mtd/mtdpart.c
+++ b/drivers/mtd/mtdpart.c
@@ -551,7 +551,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;
 
@@ -586,27 +586,12 @@ 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);
 
 	add_mtd_device(&new->mtd);
 
 	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

[PATCH v2 3/3] mtdpart: Allow deleting a partition with another partition as master

[Dan Ehrenberg]

This patch makes MTD dynamic partitioning more flexible by allowing
partitions to be deleted by referencing any partition with the same
master. For example, if you have /dev/mtd0 and /dev/mtd1 both
partitions on the same underlying device, then you can call
BLKPG_DEL_PARTITION with an fd of /dev/mtd0 and pno of 1, and
/dev/mtd1 will be deleted (as opposed to returning EINVAL to signal a
missing partition, which it did previously).

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

diff --git a/drivers/mtd/mtdpart.c b/drivers/mtd/mtdpart.c
index 7eb9083..7874bbd 100644
--- a/drivers/mtd/mtdpart.c
+++ b/drivers/mtd/mtdpart.c
@@ -600,6 +600,15 @@ int mtd_del_partition(struct mtd_info *master, int partno)
 	struct mtd_part *slave, *next;
 	int ret = -EINVAL;
 
+	/* If del_partition was called as a partition, just treat
+	 * this as if it were called on its master. This is a harmless
+	 * generalization and lets partitions be deleted in a system
+	 * where they are sometimes already present on boot */
+	if (mtd_is_partition(master)) {
+		struct mtd_part *master_partition = PART(master);
+		master = master_partition->master;
+	}
+
 	mutex_lock(&mtd_partitions_mutex);
 	list_for_each_entry_safe(slave, next, &mtd_partitions, list)
 		if ((slave->master == master) &&
-- 
2.2.0.rc0.207.ga3a616c

Re: [PATCH v2 1/3] mtdpart: Allow adding a partition of a partition

[Daniel Ehrenberg]

Hi all,

Would you recommend any further changes to this patchset?

Thanks,
Dan

On Tue, Feb 10, 2015 at 2:58 PM, Dan Ehrenberg <dehrenberg@chromium.org> wrote:
> MTD allows dynamic partitioning by the BLKPG ioctl.
> The current code restricts partition dynamic addition to work only on
> the master MTD device. This doesn't make a lot of sense, and is
> impossible to meet if the device is already partitioned (since the
> master MTD device is not visible). This commit removes the restriction.
>
> Signed-off-by: Dan Ehrenberg <dehrenberg@chromium.org>
> ---
>  drivers/mtd/mtdchar.c | 4 ----
>  drivers/mtd/mtdpart.c | 6 ++++++
>  2 files changed, 6 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/mtd/mtdchar.c b/drivers/mtd/mtdchar.c
> index 5356395..30215c7 100644
> --- a/drivers/mtd/mtdchar.c
> +++ b/drivers/mtd/mtdchar.c
> @@ -545,10 +545,6 @@ static int mtdchar_blkpg_ioctl(struct mtd_info *mtd,
>         switch (a.op) {
>         case BLKPG_ADD_PARTITION:
>
> -               /* Only master mtd device must be used to add partitions */
> -               if (mtd_is_partition(mtd))
> -                       return -EINVAL;
> -
>                 /* Sanitize user input */
>                 p.devname[BLKPG_DEVNAMELTH - 1] = '\0';
>
> diff --git a/drivers/mtd/mtdpart.c b/drivers/mtd/mtdpart.c
> index a3e3a7d..7c6f526 100644
> --- a/drivers/mtd/mtdpart.c
> +++ b/drivers/mtd/mtdpart.c
> @@ -566,6 +566,12 @@ int mtd_add_partition(struct mtd_info *master, const char *name,
>         if (length <= 0)
>                 return -EINVAL;
>
> +       if (mtd_is_partition(master)) {
> +               struct mtd_part *master_partition = PART(master);
> +               offset += master_partition->offset;
> +               master = master_partition->master;
> +       }
> +
>         part.name = name;
>         part.size = length;
>         part.offset = offset;
> --
> 2.2.0.rc0.207.ga3a616c
>

Re: [PATCH v2 1/3] mtdpart: Allow adding a partition of a partition

[Brian Norris]

On Tue, Feb 10, 2015 at 02:58:56PM -0800, Dan Ehrenberg wrote:
> MTD allows dynamic partitioning by the BLKPG ioctl.
> The current code restricts partition dynamic addition to work only on
> the master MTD device. This doesn't make a lot of sense, and is
> impossible to meet if the device is already partitioned (since the
> master MTD device is not visible). This commit removes the restriction.
> 
> Signed-off-by: Dan Ehrenberg <dehrenberg@chromium.org>

The big question I see is whether there is any downside to using
partitions as proxies for the 'master' MTD when adding/deleting
partitions. I think block devices prohibit this, but MTDs are different,
in that we don't always make the master available (i.e., we don't have
/dev/mtda, /dev/mtda1, /dev/mtda2, /dev/mtdb, /dev/mtdb1, ...).

> ---
>  drivers/mtd/mtdchar.c | 4 ----
>  drivers/mtd/mtdpart.c | 6 ++++++
>  2 files changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/mtd/mtdchar.c b/drivers/mtd/mtdchar.c
> index 5356395..30215c7 100644
> --- a/drivers/mtd/mtdchar.c
> +++ b/drivers/mtd/mtdchar.c
> @@ -545,10 +545,6 @@ static int mtdchar_blkpg_ioctl(struct mtd_info *mtd,
>  	switch (a.op) {
>  	case BLKPG_ADD_PARTITION:
>  
> -		/* Only master mtd device must be used to add partitions */
> -		if (mtd_is_partition(mtd))
> -			return -EINVAL;
> -
>  		/* Sanitize user input */
>  		p.devname[BLKPG_DEVNAMELTH - 1] = '\0';
>  
> diff --git a/drivers/mtd/mtdpart.c b/drivers/mtd/mtdpart.c
> index a3e3a7d..7c6f526 100644
> --- a/drivers/mtd/mtdpart.c
> +++ b/drivers/mtd/mtdpart.c
> @@ -566,6 +566,12 @@ int mtd_add_partition(struct mtd_info *master, const char *name,
>  	if (length <= 0)
>  		return -EINVAL;
>  
> +	if (mtd_is_partition(master)) {
> +		struct mtd_part *master_partition = PART(master);
> +		offset += master_partition->offset;
> +		master = master_partition->master;

One of the problems here is the semantics. Do we really want to do this
offset computation when using the partition instead of the master? That
means if the original partitioning does not cover the early part of the
master MTD, then that piece is lost forever. e.g.,

 master, size 0x1000000, with a single partition
 /dev/mtd0, 1st partition 0x20000 - 0x40000

Then, ioctl(BLKPG, /dev/mtd0) can never create any partitions before
0x20000. But it CAN create partitions after 0x4000! We'd have to support
negative offsets for this to work consistently. (Now that I mention it,
does MTD's BLKPG even do sanity checking on the arguments? I think it
might actually accept negative offsets...)

Also, it's a bit odd that you can use one partition to delete either
another partition, or itself. e.g.,

	flash_part del /dev/mtd1 0  # deletes mtd0
	flash_part del /dev/mtd1 1  # deletes mtd1

This might be acceptable, if awkward. But the first problem looks like
it needs more thought.

> +	}
> +
>  	part.name = name;
>  	part.size = length;
>  	part.offset = offset;

Brian

Re: [PATCH v2 1/3] mtdpart: Allow adding a partition of a partition

[Daniel Ehrenberg]

On Wed, Mar 4, 2015 at 11:23 AM, Brian Norris
<computersforpeace@gmail.com> wrote:
> One of the problems here is the semantics. Do we really want to do this
> offset computation when using the partition instead of the master? That
> means if the original partitioning does not cover the early part of the
> master MTD, then that piece is lost forever. e.g.,
>
>  master, size 0x1000000, with a single partition
>  /dev/mtd0, 1st partition 0x20000 - 0x40000
>
> Then, ioctl(BLKPG, /dev/mtd0) can never create any partitions before
> 0x20000. But it CAN create partitions after 0x4000! We'd have to support
> negative offsets for this to work consistently. (Now that I mention it,
> does MTD's BLKPG even do sanity checking on the arguments? I think it
> might actually accept negative offsets...).
>
> Also, it's a bit odd that you can use one partition to delete either
> another partition, or itself. e.g.,
>
>         flash_part del /dev/mtd1 0  # deletes mtd0
>         flash_part del /dev/mtd1 1  # deletes mtd1
>
> This might be acceptable, if awkward. But the first problem looks like
> it needs more thought.
>
> Brian

OK, how would you feel if I just added some stronger bounds checking
for add? I'm not sure what to do about the delete case.

For my use case, I have one partition set up as mtd0 which spans over
the whole device. I'll always be using this as my master. It'd suit my
purposes if we restricted these special add and removes to that sort
of case, but I generalized it like this because I thought it would be
cleaner. I'm open to more suggestions. It'd also work for me to make
some sort of option (kernel commandline param?) to give a number to
the master device and just refer to that directly (I haven't really
thought this option through).

Dan

Re: [PATCH v2 1/3] mtdpart: Allow adding a partition of a partition

[Brian Norris]

On Wed, Mar 04, 2015 at 05:50:25PM -0800, Daniel Ehrenberg wrote:
> On Wed, Mar 4, 2015 at 11:23 AM, Brian Norris
> <computersforpeace@gmail.com> wrote:
> > One of the problems here is the semantics. Do we really want to do this
> > offset computation when using the partition instead of the master? That
> > means if the original partitioning does not cover the early part of the
> > master MTD, then that piece is lost forever. e.g.,
> >
> >  master, size 0x1000000, with a single partition
> >  /dev/mtd0, 1st partition 0x20000 - 0x40000
> >
> > Then, ioctl(BLKPG, /dev/mtd0) can never create any partitions before
> > 0x20000. But it CAN create partitions after 0x4000! We'd have to support
> > negative offsets for this to work consistently. (Now that I mention it,
> > does MTD's BLKPG even do sanity checking on the arguments? I think it
> > might actually accept negative offsets...).
> >
> > Also, it's a bit odd that you can use one partition to delete either
> > another partition, or itself. e.g.,
> >
> >         flash_part del /dev/mtd1 0  # deletes mtd0
> >         flash_part del /dev/mtd1 1  # deletes mtd1
> >
> > This might be acceptable, if awkward. But the first problem looks like
> > it needs more thought.
> >
> > Brian
> 
> OK, how would you feel if I just added some stronger bounds checking
> for add?

That might be better. It at least would be consistent. But it still
kinda caters to a specific case, where all space you want to partition
is already available in some other partition.

> I'm not sure what to do about the delete case.

Maybe for the delete case, we'd only want to allow deleting the current
partition, when using a partition reference rather than a 'master'
reference?

> For my use case, I have one partition set up as mtd0 which spans over
> the whole device. I'll always be using this as my master.

FWIW, I do the same; I always have one "partition" that covers the whole
device, in addition to any sub-partitions I might actually use.

> It'd suit my
> purposes if we restricted these special add and removes to that sort
> of case, but I generalized it like this because I thought it would be
> cleaner. I'm open to more suggestions. It'd also work for me to make
> some sort of option (kernel commandline param?) to give a number to
> the master device and just refer to that directly (I haven't really
> thought this option through).

(I haven't worked this one out completely either, but...) you may be on
to something there. Personally, I'd prefer it if MTD always registered
the master as a separate device. It would certainly make things like
this easier, and it would eliminate the need for users like you and me
to artificially create entire-device partitions.

But we have this comment in mtdcore.c:

 * We don't register the master, or expect the caller to have done so,
 * for reasons of data integrity.

from ye olden days:

   commit 1f24b5a8ecbb2a3c7080f418974d40e3ffedb221
   Author: David Brownell <dbrownell@users.sourceforge.net>
   Date:   Thu Mar 26 00:42:41 2009 -0700

       [MTD] driver model updates

which answered this question, originating from prehistoric MTD:

 * (Q: should we register the master MTD object as well?)

IMO, the "data integrity" argument is pretty flimsy. Nobody complains
that the SCSI subsystem gives users the power to clobber partition
tables in /dev/sda, do they?

But alas, we probably can't really change the default registration
behavior here without massively breaking the #1 rule of kernel
programming.

Brian