[PATCH 1/3] mtd: cmdlinepart: make the partitions rule more strict

[PATCH 1/3] mtd: cmdlinepart: make the partitions rule more strict

[Huang Shijie]

There are typically two types to set the mtd partitions:

<1> set with the `size`, such as
    gpmi-nand:100m(boot),100m(kernel),1g(rootfs)

<2> set with the `offset`, such as
    gpmi-nand:100m@0(boot),100m@100m(kernel),1g@200m(rootfs)
    gpmi-nand:1g@200m(rootfs),100m@0(boot),100m@100m(kernel)

If we mix these two types, such as:
     gpmi-nand:100m@0(boot),100m(kernel),1g@200m(rootfs)
     gpmi-nand:1g@200m(rootfs),100m@0(boot),100m(kernel)

It's hard to understand the cmdline. And also it is hard to sort the
partitions in this mixed type. So we explicitly forbid the mixed type.

Signed-off-by: Huang Shijie <shijie8@gmail.com>
---
 drivers/mtd/cmdlinepart.c |    9 +++++++++
 1 files changed, 9 insertions(+), 0 deletions(-)

diff --git a/drivers/mtd/cmdlinepart.c b/drivers/mtd/cmdlinepart.c
index fe7e3a5..0b7b2ad 100644
--- a/drivers/mtd/cmdlinepart.c
+++ b/drivers/mtd/cmdlinepart.c
@@ -35,6 +35,15 @@
  *
  * 1 NOR Flash with 2 partitions, 1 NAND with one
  * edb7312-nor:256k(ARMboot)ro,-(root);edb7312-nand:-(home)
+ *
+ * Note:
+ * If you choose to set the @offset for the <partdef>, please set all
+ * the partitions with the same syntax, such as:
+ *     gpmi-nand:100m@0(boot),100m@100m(kernel),1g@200m(rootfs)
+ *
+ * Please do _NOT_ set the partitions like this:
+ *     gpmi-nand:100m@0(boot),100m(kernel),1g@200m(rootfs)
+ * The `kernel` partition does not set with the @offset, this is not permitted.
  */
 
 #include <linux/kernel.h>
-- 
1.7.4.4

[PATCH 2/3] mtd: cmdlinepart: sort the unsorted partitions

[Huang Shijie]

Assume we have a 1GB(8Gb) nand chip.
It is legit if we set the partitions as the following:
    gpmi-nand:1g@200m(rootfs),100m@0(boot),100m@100m(kernel)

But the current code can not parse out any partition with this
cmdline.

This patch sorts the unsorted partitions by the @offset.
For there are maybe only several partitions, i use the simple
Bubble sort algorithm.

Signed-off-by: Huang Shijie <shijie8@gmail.com>
---
 drivers/mtd/cmdlinepart.c |   29 +++++++++++++++++++++++++++++
 1 files changed, 29 insertions(+), 0 deletions(-)

diff --git a/drivers/mtd/cmdlinepart.c b/drivers/mtd/cmdlinepart.c
index 0b7b2ad..f40d390 100644
--- a/drivers/mtd/cmdlinepart.c
+++ b/drivers/mtd/cmdlinepart.c
@@ -234,6 +234,32 @@ static struct mtd_partition * newpart(char *s,
 	return parts;
 }
 
+/* There are only several partitions, so the Bubble sort is enough. */
+static inline void sort_partitons(struct mtd_partition *parts, int num_parts)
+{
+	int i, j;
+
+	if (num_parts < 2)
+		return;
+
+	if (parts[0].offset == OFFSET_CONTINUOUS)
+		return;
+
+	/* sort by the offset */
+	for (i = 0; i < num_parts - 1; i++) {
+		for (j = 1; j < num_parts - i; j++) {
+			if (parts[j - 1].offset > parts[j].offset) {
+				struct mtd_partition tmp;
+
+				tmp = parts[j - 1];
+				parts[j - 1] = parts[j];
+				parts[j] = tmp;
+			}
+		}
+	}
+	return;
+}
+
 /*
  * Parse the command line.
  */
@@ -292,6 +318,9 @@ static int mtdpart_setup_real(char *s)
 		this_mtd->mtd_id = (char*)(this_mtd + 1);
 		strlcpy(this_mtd->mtd_id, mtd_id, mtd_id_len + 1);
 
+		/* sort the partitions */
+		sort_partitons(parts, num_parts);
+
 		/* link into chain */
 		this_mtd->next = partitions;
 		partitions = this_mtd;
-- 
1.7.4.4

Re: [PATCH 2/3] mtd: cmdlinepart: sort the unsorted partitions

[Artem Bityutskiy]

[-- Attachment #1: Type: text/plain, Size: 867 bytes --]

On Sun, 2012-08-26 at 13:21 -0400, Huang Shijie wrote:
> Assume we have a 1GB(8Gb) nand chip.
> It is legit if we set the partitions as the following:
>     gpmi-nand:1g@200m(rootfs),100m@0(boot),100m@100m(kernel)
> 
> But the current code can not parse out any partition with this
> cmdline.
> 
> This patch sorts the unsorted partitions by the @offset.
> For there are maybe only several partitions, i use the simple
> Bubble sort algorithm.
> 
> Signed-off-by: Huang Shijie <shijie8@gmail.com>

I still cannot find time to actually think about this carefully, but the
commit message does not sound convincing, it does not explain why
sorting is the right way to fix the issue, and what would be the
alternatives. It actually also does not explain why exactly we currently
cannot parse the example string.

-- 
Best Regards,
Artem Bityutskiy

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 2/3] mtd: cmdlinepart: sort the unsorted partitions

[Huang Shijie]

On Fri, Aug 31, 2012 at 9:59 AM, Artem Bityutskiy <dedekind1@gmail.com> wrote:
> On Sun, 2012-08-26 at 13:21 -0400, Huang Shijie wrote:
>> Assume we have a 1GB(8Gb) nand chip.
>> It is legit if we set the partitions as the following:
>>     gpmi-nand:1g@200m(rootfs),100m@0(boot),100m@100m(kernel)
>>
>> But the current code can not parse out any partition with this
>> cmdline.
>>
>> This patch sorts the unsorted partitions by the @offset.
>> For there are maybe only several partitions, i use the simple
>> Bubble sort algorithm.
>>
>> Signed-off-by: Huang Shijie <shijie8@gmail.com>
>
> I still cannot find time to actually think about this carefully, but the
> commit message does not sound convincing, it does not explain why
> sorting is the right way to fix the issue, and what would be the
> alternatives. It actually also does not explain why exactly we currently
> cannot parse the example string.
>
thanks .

I will add more comment in the next version.

Best Regards
Huang Shijie

Re: [PATCH 2/3] mtd: cmdlinepart: sort the unsorted partitions

[Artem Bityutskiy]

[-- Attachment #1: Type: text/plain, Size: 1041 bytes --]

On Sun, 2012-08-26 at 13:21 -0400, Huang Shijie wrote:
> +/* There are only several partitions, so the Bubble sort is enough. */
> +static inline void sort_partitons(struct mtd_partition *parts, int num_parts)
> +{
> +	int i, j;
> +
> +	if (num_parts < 2)
> +		return;

Not necessary, the for loop should work for this case.
> +
> +	if (parts[0].offset == OFFSET_CONTINUOUS)
> +		return;

Hmm, I guess the sorting function should not have this check. You
probably can just sort normally these ones, they will be the last ones.

And then we can do a separate pass where we check for overlapping
partitions and multiple OFFSET_CONTINUOUS.

> +
> +	/* sort by the offset */
> +	for (i = 0; i < num_parts - 1; i++) {
> +		for (j = 1; j < num_parts - i; j++) {
> +			if (parts[j - 1].offset > parts[j].offset) {
> +				struct mtd_partition tmp;
> +
> +				tmp = parts[j - 1];
> +				parts[j - 1] = parts[j];
> +				parts[j] = tmp;
> +			}
> +		}
> +	}
> +	return;
> +}


-- 
Best Regards,
Artem Bityutskiy

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[PATCH 3/3] mtd: cmdlinepart: fix the wrong partitions number when truncating occurs

[Huang Shijie]

This patch is based on the assumption that all the partitions are
in the right offset order.

Assume we have a 1GB(8Gb) nand chip, and we set the partitions
in the command line like this:
    #gpmi-nand:100m(boot),100m(kernel),1g(rootfs)

In this case, the partition truncating occurs. The current code will
get the following result:

     ----------------------------------
	root@freescale ~$ cat /proc/mtd
	dev:    size   erasesize  name
	mtd0: 06400000 00040000 "boot"
	mtd1: 06400000 00040000 "kernel"
     ----------------------------------

It is obvious that we lost the truncated partition `rootfs` which should
be 824M in this case.

Why? The old code sets the wrong partitions number when the truncating
occurs. This patch fixes it. Alao add a `break` to shortcut the code in this
case.

After apply this patch, the result becomes:
     ----------------------------------
	root@freescale ~$ cat /proc/mtd
	dev:    size   erasesize  name
	mtd0: 06400000 00040000 "boot"
	mtd1: 06400000 00040000 "kernel"
	mtd2: 33800000 00040000 "rootfs"
     ----------------------------------

We get the right result.

Signed-off-by: Huang Shijie <shijie8@gmail.com>
---
 drivers/mtd/cmdlinepart.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/drivers/mtd/cmdlinepart.c b/drivers/mtd/cmdlinepart.c
index f40d390..9f0afe5 100644
--- a/drivers/mtd/cmdlinepart.c
+++ b/drivers/mtd/cmdlinepart.c
@@ -382,7 +382,8 @@ static int parse_cmdline_partitions(struct mtd_info *master,
 					       "%s: partitioning exceeds flash size, truncating\n",
 					       part->mtd_id);
 					part->parts[i].size = master->size - offset;
-					part->num_parts = i;
+					part->num_parts = i + 1;
+					break;
 				}
 				offset += part->parts[i].size;
 			}
-- 
1.7.4.4

Re: [PATCH 3/3] mtd: cmdlinepart: fix the wrong partitions number when truncating occurs

[Artem Bityutskiy]

[-- Attachment #1: Type: text/plain, Size: 1401 bytes --]

On Sun, 2012-08-26 at 13:21 -0400, Huang Shijie wrote:
> This patch is based on the assumption that all the partitions are
> in the right offset order.
> 
> Assume we have a 1GB(8Gb) nand chip, and we set the partitions
> in the command line like this:
>     #gpmi-nand:100m(boot),100m(kernel),1g(rootfs)
> 
> In this case, the partition truncating occurs. The current code will
> get the following result:
> 
>      ----------------------------------
> 	root@freescale ~$ cat /proc/mtd
> 	dev:    size   erasesize  name
> 	mtd0: 06400000 00040000 "boot"
> 	mtd1: 06400000 00040000 "kernel"
>      ----------------------------------
> 
> It is obvious that we lost the truncated partition `rootfs` which should
> be 824M in this case.
> 
> Why? The old code sets the wrong partitions number when the truncating
> occurs. This patch fixes it. Alao add a `break` to shortcut the code in this
> case.
> 
> After apply this patch, the result becomes:
>      ----------------------------------
> 	root@freescale ~$ cat /proc/mtd
> 	dev:    size   erasesize  name
> 	mtd0: 06400000 00040000 "boot"
> 	mtd1: 06400000 00040000 "kernel"
> 	mtd2: 33800000 00040000 "rootfs"
>      ----------------------------------
> 
> We get the right result.
> 
> Signed-off-by: Huang Shijie <shijie8@gmail.com>

Should this have CC to -stable?

-- 
Best Regards,
Artem Bityutskiy

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 3/3] mtd: cmdlinepart: fix the wrong partitions number when truncating occurs

[Huang Shijie]

On Thu, Aug 30, 2012 at 2:43 PM, Artem Bityutskiy <dedekind1@gmail.com> wrote:
> On Sun, 2012-08-26 at 13:21 -0400, Huang Shijie wrote:
>> This patch is based on the assumption that all the partitions are
>> in the right offset order.
>>
>> Assume we have a 1GB(8Gb) nand chip, and we set the partitions
>> in the command line like this:
>>     #gpmi-nand:100m(boot),100m(kernel),1g(rootfs)
>>
>> In this case, the partition truncating occurs. The current code will
>> get the following result:
>>
>>      ----------------------------------
>>       root@freescale ~$ cat /proc/mtd
>>       dev:    size   erasesize  name
>>       mtd0: 06400000 00040000 "boot"
>>       mtd1: 06400000 00040000 "kernel"
>>      ----------------------------------
>>
>> It is obvious that we lost the truncated partition `rootfs` which should
>> be 824M in this case.
>>
>> Why? The old code sets the wrong partitions number when the truncating
>> occurs. This patch fixes it. Alao add a `break` to shortcut the code in this
>> case.
>>
>> After apply this patch, the result becomes:
>>      ----------------------------------
>>       root@freescale ~$ cat /proc/mtd
>>       dev:    size   erasesize  name
>>       mtd0: 06400000 00040000 "boot"
>>       mtd1: 06400000 00040000 "kernel"
>>       mtd2: 33800000 00040000 "rootfs"
>>      ----------------------------------
>>
>> We get the right result.
>>
>> Signed-off-by: Huang Shijie <shijie8@gmail.com>
>
> Should this have CC to -stable?

It's ok to CC to stable.


thanks
Huang Shijie
>
> --
> Best Regards,
> Artem Bityutskiy

Re: [PATCH 1/3] mtd: cmdlinepart: make the partitions rule more strict

[Artem Bityutskiy]

[-- Attachment #1: Type: text/plain, Size: 800 bytes --]

On Sun, 2012-08-26 at 13:21 -0400, Huang Shijie wrote:
> There are typically two types to set the mtd partitions:
> 
> <1> set with the `size`, such as
>     gpmi-nand:100m(boot),100m(kernel),1g(rootfs)
> 
> <2> set with the `offset`, such as
>     gpmi-nand:100m@0(boot),100m@100m(kernel),1g@200m(rootfs)
>     gpmi-nand:1g@200m(rootfs),100m@0(boot),100m@100m(kernel)
> 
> If we mix these two types, such as:
>      gpmi-nand:100m@0(boot),100m(kernel),1g@200m(rootfs)
>      gpmi-nand:1g@200m(rootfs),100m@0(boot),100m(kernel)
> 
> It's hard to understand the cmdline. And also it is hard to sort the
> partitions in this mixed type. So we explicitly forbid the mixed type.

So "explicitly forbid" is just to add a "do not do this" comment?

-- 
Best Regards,
Artem Bityutskiy

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 1/3] mtd: cmdlinepart: make the partitions rule more strict

[Huang Shijie]

On Fri, Aug 31, 2012 at 7:45 AM, Artem Bityutskiy <dedekind1@gmail.com> wrote:
> On Sun, 2012-08-26 at 13:21 -0400, Huang Shijie wrote:
>> There are typically two types to set the mtd partitions:
>>
>> <1> set with the `size`, such as
>>     gpmi-nand:100m(boot),100m(kernel),1g(rootfs)
>>
>> <2> set with the `offset`, such as
>>     gpmi-nand:100m@0(boot),100m@100m(kernel),1g@200m(rootfs)
>>     gpmi-nand:1g@200m(rootfs),100m@0(boot),100m@100m(kernel)
>>
>> If we mix these two types, such as:
>>      gpmi-nand:100m@0(boot),100m(kernel),1g@200m(rootfs)
>>      gpmi-nand:1g@200m(rootfs),100m@0(boot),100m(kernel)
>>
>> It's hard to understand the cmdline. And also it is hard to sort the
>> partitions in this mixed type. So we explicitly forbid the mixed type.
>
> So "explicitly forbid" is just to add a "do not do this" comment?
>

This is the simplest way. ;)

It's make code complicated if we change the code to forbid this mixed type.

Best Regards
Huang Shijie

Re: [PATCH 1/3] mtd: cmdlinepart: make the partitions rule more strict

[Huang Shijie]

On Fri, Aug 31, 2012 at 7:45 AM, Artem Bityutskiy <dedekind1@gmail.com> wrote:
> On Sun, 2012-08-26 at 13:21 -0400, Huang Shijie wrote:
>> There are typically two types to set the mtd partitions:
>>
>> <1> set with the `size`, such as
>>     gpmi-nand:100m(boot),100m(kernel),1g(rootfs)
>>
>> <2> set with the `offset`, such as
>>     gpmi-nand:100m@0(boot),100m@100m(kernel),1g@200m(rootfs)
>>     gpmi-nand:1g@200m(rootfs),100m@0(boot),100m@100m(kernel)
>>
>> If we mix these two types, such as:
>>      gpmi-nand:100m@0(boot),100m(kernel),1g@200m(rootfs)
>>      gpmi-nand:1g@200m(rootfs),100m@0(boot),100m(kernel)
>>
>> It's hard to understand the cmdline. And also it is hard to sort the
>> partitions in this mixed type. So we explicitly forbid the mixed type.
>
> So "explicitly forbid" is just to add a "do not do this" comment?
>
Do you think we should change the code to forbid this mixed type?

Huang Shijie

Re: [PATCH 1/3] mtd: cmdlinepart: make the partitions rule more strict

[Artem Bityutskiy]

[-- Attachment #1: Type: text/plain, Size: 1066 bytes --]

On Sun, 2012-08-26 at 13:21 -0400, Huang Shijie wrote:
> + *
> + * Note:
> + * If you choose to set the @offset for the <partdef>, please set all
> + * the partitions with the same syntax, such as:
> + *     gpmi-nand:100m@0(boot),100m@100m(kernel),1g@200m(rootfs)
> + *
> + * Please do _NOT_ set the partitions like this:
> + *     gpmi-nand:100m@0(boot),100m(kernel),1g@200m(rootfs)
> + * The `kernel` partition does not set with the @offset, this is not permitted.
>   */

I guess it is indeed OK to sort the partitions, just makes things a lot
simpler. But we probably then should also do the following:

1. Make sure there is only one partition without offset. If there are
several - error out.
2. Check that partitions do not intersect - I did not notice that we do
this in the code.

So AFAICS, this patch is not needed and we better have the following
patches:

1. Add sorting
2. Add a check that partitions do not overlap and there is only one
offset-less partition.

How does this sound?

-- 
Best Regards,
Artem Bityutskiy

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 1/3] mtd: cmdlinepart: make the partitions rule more strict

[Huang Shijie]

On Mon, Sep 3, 2012 at 3:18 AM, Artem Bityutskiy <dedekind1@gmail.com> wrote:
> On Sun, 2012-08-26 at 13:21 -0400, Huang Shijie wrote:
>> + *
>> + * Note:
>> + * If you choose to set the @offset for the <partdef>, please set all
>> + * the partitions with the same syntax, such as:
>> + *     gpmi-nand:100m@0(boot),100m@100m(kernel),1g@200m(rootfs)
>> + *
>> + * Please do _NOT_ set the partitions like this:
>> + *     gpmi-nand:100m@0(boot),100m(kernel),1g@200m(rootfs)
>> + * The `kernel` partition does not set with the @offset, this is not permitted.
>>   */
>
> I guess it is indeed OK to sort the partitions, just makes things a lot
> simpler. But we probably then should also do the following:
>
> 1. Make sure there is only one partition without offset. If there are
> several - error out.

 Why allow `only one partition without offset`?

Take the following unsorted partitions as an example:
     #gpmi-nand:1g@200m(rootfs),100m(boot),100m@100m(kernel)

The current code will parses out the following partitions:
      rootfs: < size = 1g, offset=200m>
      boot:   < size = 100m, offset= OFFSET_CONTINOUS>
      kernel: <size = 100m, offset = 100m>

If we sort the partitions by the offset, we get the following result:
     "kernel" , "rootfs", "boot"

In actually, we expect the following result:
    "boot", "kernel", "rootfs"

What's your idea about this issue?
In my opinion, we should forbid the mixed type.

thanks
Huang Shijie







> 2. Check that partitions do not intersect - I did not notice that we do
> this in the code.
>
> So AFAICS, this patch is not needed and we better have the following
> patches:
>
> 1. Add sorting
> 2. Add a check that partitions do not overlap and there is only one
> offset-less partition.
>
> How does this sound?
>
> --
> Best Regards,
> Artem Bityutskiy