[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 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

[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

[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

[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

[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 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 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

[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 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

[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?
>

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 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

[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

[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 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

[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

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

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 --]

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

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

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

[Artem Bityutskiy]

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

[Dropped extra CCs - let's spam less]

On Mon, 2012-09-03 at 11:09 -0400, Huang Shijie wrote:
> 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`?

E.g.,

gpmi-nand:1g@200m(rootfs),100m(boot),100m@100m(kernel),200m(ququ)

how would "boot" and "ququ" look like?

What I basically say is that we should refuse lines like this. Which
means checking that there is only one "OFFSET_CONTINOUS" partition.

> 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"

Yeah, for some reason I assumed that if you do not specify offset then
you mean the partition must be the _last_ and span the rest of flash. I
assume other usage is bogus. So from this POW the example is bogus and
lines like this would be rejected.

I guess the criteria is that this sting contains a "gap" (0-100m).

With the logic I suggested, for this case what I said would basically:

1. Sort. This leads to

100m@100m(kernel)
1g@200m(rootfs)
100m(boot)

"OFFSET_CONTINOUS" is always the largest and will be at the end when
sorting.

2. Verification.

We verify for overlaps and gaps. And refuse this one.

-- 
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]

于 2012年09月03日 23:35, Artem Bityutskiy 写道:
> 1. Sort. This leads to
>
> 100m@100m(kernel)
> 1g@200m(rootfs)
> 100m(boot)
>
> "OFFSET_CONTINOUS" is always the largest and will be at the end when
> sorting.
>
ok.
> 2. Verification.
>
> We verify for overlaps and gaps. And refuse this one.
>
It's really necessary to check the overlaps.

I will try to send out the new patch set.


Best Regards
Huang Shijie

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

[Shmulik Ladkani]

On Mon, 03 Sep 2012 18:35:44 +0300 Artem Bityutskiy <dedekind1@gmail.com> wrote:
> With the logic I suggested, for this case what I said would basically:
> 
> 1. Sort. This leads to
> 
> 100m@100m(kernel)
> 1g@200m(rootfs)
> 100m(boot)
> 
> "OFFSET_CONTINOUS" is always the largest and will be at the end when
> sorting.
> 
> 2. Verification.
> 
> We verify for overlaps and gaps. And refuse this one.

Artem, Huang,

Sorry for the long delay, got busy with urgent matters (found out I need
to go thru some surgery), I will probably not be available in the
upcoming days, but I've read the correspondance and wanted to share my
two cents...

My POV is that sorting and verification is not needed, is troublesome,
and might affect users in ways they don't expect.

So far, mtdparts commandline parsing has been very lenient and liberal.
I think we should keep this approach; give the user the flexibility,
he'll be responsible to provide meaningful cmdline parts for his
system.

Actually, Huang's initial complaint was that 'parse_cmdline_partitions'
was too strict - the truncated partition was not registered!

Now, philosophy aside, let's talk about some usecases that might break.

I remember overlapping partitions (more precisely, partition that is a
subset of another) being common in some embedded systems, where the
"rootfs" and "kernel" partitions are a subset of the "overall image"
partition.
Why break this? if users enjoy the flexibility, and careful not to
create silly partitions, I'm in favor of keeping the flexibility.

Same goes for sorting.
If one has a system hacked to work with mtd0 hardcodedly, but mtd0's
physical location is somewhere at the end of the device, why reorder the
partitions enumeration affecting this system?

I'd say keep it simple and flexible.
I trust the user to provide meaningful partitions in the cmdline.

Anyways, please consider this approach.

My patch suggestion might look as (and might need some rework):

diff --git a/drivers/mtd/cmdlinepart.c b/drivers/mtd/cmdlinepart.c
index 17b0bd4..f5df613 100644
--- a/drivers/mtd/cmdlinepart.c
+++ b/drivers/mtd/cmdlinepart.c
@@ -324,7 +324,6 @@ 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;
                                }
                                offset += part->parts[i].size;
                        }

Regards,
Shmulik

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

[Huang Shijie]

于 2012年09月04日 19:48, Shmulik Ladkani 写道:
> On Mon, 03 Sep 2012 18:35:44 +0300 Artem Bityutskiy<dedekind1@gmail.com>  wrote:
>> With the logic I suggested, for this case what I said would basically:
>>
>> 1. Sort. This leads to
>>
>> 100m@100m(kernel)
>> 1g@200m(rootfs)
>> 100m(boot)
>>
>> "OFFSET_CONTINOUS" is always the largest and will be at the end when
>> sorting.
>>
>> 2. Verification.
>>
>> We verify for overlaps and gaps. And refuse this one.
> Artem, Huang,
>
> Sorry for the long delay, got busy with urgent matters (found out I need
> to go thru some surgery), I will probably not be available in the
> upcoming days, but I've read the correspondance and wanted to share my
> two cents...
>
> My POV is that sorting and verification is not needed, is troublesome,
> and might affect users in ways they don't expect.
>
> So far, mtdparts commandline parsing has been very lenient and liberal.
> I think we should keep this approach; give the user the flexibility,
> he'll be responsible to provide meaningful cmdline parts for his
> system.
>
> Actually, Huang's initial complaint was that 'parse_cmdline_partitions'
> was too strict - the truncated partition was not registered!
>
> Now, philosophy aside, let's talk about some usecases that might break.
>
> I remember overlapping partitions (more precisely, partition that is a
> subset of another) being common in some embedded systems, where the
> "rootfs" and "kernel" partitions are a subset of the "overall image"
> partition.
> Why break this? if users enjoy the flexibility, and careful not to
> create silly partitions, I'm in favor of keeping the flexibility.
>
> Same goes for sorting.
> If one has a system hacked to work with mtd0 hardcodedly, but mtd0's
> physical location is somewhere at the end of the device, why reorder the
> partitions enumeration affecting this system?
>
> I'd say keep it simple and flexible.
yes, it's really simple and flexible.
But I think it's not wise to export the 0-size partition to user.
The user may confused at this.

And the mtd-utils, such as mtd-info/mtd-debug, do they can work with a 
0-size partition?

thanks
Huang Shijie

> I trust the user to provide meaningful partitions in the cmdline.
>
> Anyways, please consider this approach.
>
> My patch suggestion might look as (and might need some rework):
>
> diff --git a/drivers/mtd/cmdlinepart.c b/drivers/mtd/cmdlinepart.c
> index 17b0bd4..f5df613 100644
> --- a/drivers/mtd/cmdlinepart.c
> +++ b/drivers/mtd/cmdlinepart.c
> @@ -324,7 +324,6 @@ 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;
>                                  }
>                                  offset += part->parts[i].size;
>                          }
>
> Regards,
> Shmulik
>
> ______________________________________________________
> Linux MTD discussion mailing list
> http://lists.infradead.org/mailman/listinfo/linux-mtd/
>

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

[Shmulik Ladkani]

On Wed, 5 Sep 2012 10:12:20 +0800 Huang Shijie <b32955@freescale.com> wrote:
> yes, it's really simple and flexible.
> But I think it's not wise to export the 0-size partition to user.
> The user may confused at this.
> 
> And the mtd-utils, such as mtd-info/mtd-debug, do they can work with a 
> 0-size partition?

I think the 0 sized partition case is really seldom, don't worth the
trouble, the user will probably observe something went wrong in his
cmdline parts specification.

But nevertheless, if we think it's a MUST to forbid the 0 sized
partitions, then amend my patch as follows:

(sorry, didn't even compile tested this, please review and test, if the
approach is acceptible)

diff --git a/drivers/mtd/cmdlinepart.c b/drivers/mtd/cmdlinepart.c
index 17b0bd4..aed1b8a 100644
--- a/drivers/mtd/cmdlinepart.c
+++ b/drivers/mtd/cmdlinepart.c
@@ -319,12 +319,22 @@ static int parse_cmdline_partitions(struct mtd_info *master,
 				if (part->parts[i].size == SIZE_REMAINING)
 					part->parts[i].size = master->size - offset;
 
+				if (part->parts[i].size == 0) {
+					printk(KERN_WARNING ERRP
+					       "%s: skipping zero sized partition\n",
+					       part->mtd_id);
+					part->num_parts--;
+					memmove(&part->parts[i],
+						&part->parts[i + 1],
+						sizeof(*part->parts) * (part->num_parts - i));
+					continue;
+				}
+
 				if (offset + part->parts[i].size > master->size) {
 					printk(KERN_WARNING ERRP
 					       "%s: partitioning exceeds flash size, truncating\n",
 					       part->mtd_id);
 					part->parts[i].size = master->size - offset;
-					part->num_parts = i;
 				}
 				offset += part->parts[i].size;
 			}

Also, I'll send a short coding cleanup patch, this loop's indent level
is horrible...

Regards,
Shmulik

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

[Artem Bityutskiy]

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

On Wed, 2012-09-05 at 08:30 +0300, Shmulik Ladkani wrote:
> On Wed, 5 Sep 2012 10:12:20 +0800 Huang Shijie <b32955@freescale.com> wrote:
> > yes, it's really simple and flexible.
> > But I think it's not wise to export the 0-size partition to user.
> > The user may confused at this.
> > 
> > And the mtd-utils, such as mtd-info/mtd-debug, do they can work with a 
> > 0-size partition?
> 
> I think the 0 sized partition case is really seldom, don't worth the
> trouble, the user will probably observe something went wrong in his
> cmdline parts specification.
> 
> But nevertheless, if we think it's a MUST to forbid the 0 sized
> partitions, then amend my patch as follows:

OK, pushed this one to l2-mtd.git. Guys, if there are more patches on
top of this, please, re-send them.

-- 
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

[Christopher Cordahi]

Huang Shijie <b32955 <at> freescale.com> writes:

> 于 2012年09月04日 19:48, Shmulik Ladkani 写道:
> >
> > My POV is that sorting and verification is not needed, is troublesome,
> > and might affect users in ways they don't expect.
> >
> > So far, mtdparts commandline parsing has been very lenient and liberal.
> > I think we should keep this approach; give the user the flexibility,
> > he'll be responsible to provide meaningful cmdline parts for his
> > system.
> >
> > Actually, Huang's initial complaint was that 'parse_cmdline_partitions'
> > was too strict - the truncated partition was not registered!
> >
> > Now, philosophy aside, let's talk about some usecases that might break.
> >
> > I remember overlapping partitions (more precisely, partition that is a
> > subset of another) being common in some embedded systems, where the
> > "rootfs" and "kernel" partitions are a subset of the "overall image"
> > partition.
> > Why break this? if users enjoy the flexibility, and careful not to
> > create silly partitions, I'm in favor of keeping the flexibility.
> >
> > Same goes for sorting.
> > If one has a system hacked to work with mtd0 hardcodedly, but mtd0's
> > physical location is somewhere at the end of the device, why reorder the
> > partitions enumeration affecting this system?
> >
> > I'd say keep it simple and flexible.
> yes, it's really simple and flexible.

Is it agreed that sorting is not necessary and will not be implemented?
If so are there objections to me modifying the documentation to indicate
that out of order partitions is part of the supported design?

In my embedded distribution, I would like to introduce a MTD partition
at the end of the list to not affect existing hardcoded MTD numbers.

The fw_printenv/fw_setenv command line interface to U-Boot environment
makes use of a /etc/fw_env.config with hardcoded device numbers which
would be broken by a sort.

Although my out of order partitions worked, I couldn't find any
documentation indicating it was a supported feature.  This thread with
Artem's support for sorting got me worried.

If the only reason for sorting is to prevent overlapping partitions,
I think this can be written just as efficiently without a sort although
Shmulik also gives a reason why this restriction should not exist.
I'd prefer to accept that task rather than have to handle changing
MTD numbers.

> But I think it's not wise to export the 0-size partition to user.
> The user may confused at this.

Although I just submitted a patch to improve this, personally I think
if a user specifies a zero sized partition, they should get it.  Or if
it's inherently incorrect I think it should be an error.  Similarly
I'd expect the same for truncated partitions, either provide the size
specified or error.  Since I don't ever foresee using either of these
two features I don't have any objections to current situation.  These
are just my $0.02

--
Chris

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

[Brian Norris]

(re-include others on CC)

On Sun, Dec 16, 2012 at 5:11 PM, Christopher Cordahi
<christophercordahi@nanometrics.ca> wrote:
> Huang Shijie <b32955 <at> freescale.com> writes:
>> 于 2012年09月04日 19:48, Shmulik Ladkani 写道:
>> >
>> > My POV is that sorting and verification is not needed, is troublesome,
>> > and might affect users in ways they don't expect.
>> >
>> > So far, mtdparts commandline parsing has been very lenient and liberal.
>> > I think we should keep this approach; give the user the flexibility,
>> > he'll be responsible to provide meaningful cmdline parts for his
>> > system.
>> >
>> > Actually, Huang's initial complaint was that 'parse_cmdline_partitions'
>> > was too strict - the truncated partition was not registered!
>> >
>> > Now, philosophy aside, let's talk about some usecases that might break.
>> >
>> > I remember overlapping partitions (more precisely, partition that is a
>> > subset of another) being common in some embedded systems, where the
>> > "rootfs" and "kernel" partitions are a subset of the "overall image"
>> > partition.
>> > Why break this? if users enjoy the flexibility, and careful not to
>> > create silly partitions, I'm in favor of keeping the flexibility.
>> >
>> > Same goes for sorting.
>> > If one has a system hacked to work with mtd0 hardcodedly, but mtd0's
>> > physical location is somewhere at the end of the device, why reorder the
>> > partitions enumeration affecting this system?
>> >
>> > I'd say keep it simple and flexible.
>> yes, it's really simple and flexible.
>
> Is it agreed that sorting is not necessary and will not be implemented?
> If so are there objections to me modifying the documentation to indicate
> that out of order partitions is part of the supported design?

I don't object but rather would welcome the documentation. I think the
consensus was that there were real reasons to allow unsorted
partitions at least, and maybe even overlapping partitions.

> In my embedded distribution, I would like to introduce a MTD partition
> at the end of the list to not affect existing hardcoded MTD numbers.

FWIW, my embedded systems make use of both out-of-order and
overlapping partitions. I don't see why they must be sorted, and
currently, overlapping is useful for me in exactly the scenarios given
above (rootfs + kernel as subsets of entire_device).

> Although my out of order partitions worked, I couldn't find any
> documentation indicating it was a supported feature.  This thread with
> Artem's support for sorting got me worried.
>
> If the only reason for sorting is to prevent overlapping partitions,
> I think this can be written just as efficiently without a sort although
> Shmulik also gives a reason why this restriction should not exist.
> I'd prefer to accept that task rather than have to handle changing
> MTD numbers.
>
>> But I think it's not wise to export the 0-size partition to user.
>> The user may confused at this.
>
> Although I just submitted a patch to improve this, personally I think
> if a user specifies a zero sized partition, they should get it.  Or if
> it's inherently incorrect I think it should be an error.  Similarly
> I'd expect the same for truncated partitions, either provide the size
> specified or error.  Since I don't ever foresee using either of these
> two features I don't have any objections to current situation.  These
> are just my $0.02

Well, the truncated partition is helpful sometimes. Perhaps an old
flash partition layout is used when swapping in a different, smaller
NAND; the truncated partition allows the system to still work, with a
nice warning in case the user wants to fix it.

Brian

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

[Artem Bityutskiy]

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

On Mon, 2012-12-17 at 01:11 +0000, Christopher Cordahi wrote:
> > > I'd say keep it simple and flexible.
> > yes, it's really simple and flexible.
> 
> Is it agreed that sorting is not necessary and will not be implemented?
> If so are there objections to me modifying the documentation to indicate
> that out of order partitions is part of the supported design?

I think we agreed not to do this.

-- 
Best Regards,
Artem Bityutskiy

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