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

[PATCH v2 1/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)

Unfortunately, the current code can not parse out any partition with this
cmdline. The parse_cmdline_partitions() will meet the
truncated partition `rootfs` firstly. But the parse_cmdline_partitions()
will break its `for` loop with the following line when the truncating occurs.

       ...........................
	part->num_parts = i;
       ...........................

What's worse is that this line has a bug in it. It accidently misses
the truncated partitions. So we can not parse out any partition.

In this case, we should parse out the `boot` and `kernel` partitions at least.
So we need to sort the unsorted partitions. With the sorted partitions, the
parse_cmdline_partitions() can parse out the `boot` and `kernel` successfully.

This patch sorts the unsorted partitions by the @offset.
For there are maybe only several partitions, so the Bubble sort algorithm
is enough.

We also need another patch to fix the missed truncated partition issue.

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

diff --git a/drivers/mtd/cmdlinepart.c b/drivers/mtd/cmdlinepart.c
index 17b0bd4..dde8e84 100644
--- a/drivers/mtd/cmdlinepart.c
+++ b/drivers/mtd/cmdlinepart.c
@@ -205,6 +205,26 @@ 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;
+
+	/* 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.
  */
@@ -262,6 +282,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.0.4

[PATCH v2 2/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 <b32955@freescale.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 dde8e84..edd17e0 100644
--- a/drivers/mtd/cmdlinepart.c
+++ b/drivers/mtd/cmdlinepart.c
@@ -347,7 +347,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.0.4

[PATCH v2 3/3] mtd: cmdlinepart: check the partitions

[Huang Shijie]

The partitions are all sorted well by the @offset.
But we should check if there are errors in the partitions.

The following cases are regarded as errors:
  [1] There is a hole in the partitions, such as
      #gpmi-nand:100m(boot),100m@100m(kernel),1g@200m(rootfs)

  [2] There is a overlap in the partitions, such as
      #gpmi-nand:100m@0(boot),100m@50m(kernel),1g@150m(rootfs)

  [3] Not all the partitions are set with @offset, and there are
      more then one partion whose offset is OFFSET_CONTINUOUS.
      #gpmi-nand:100m@0(boot),100m(kernel),-(usr)

Signed-off-by: Huang Shijie <b32955@freescale.com>
---
 drivers/mtd/cmdlinepart.c |   82 ++++++++++++++++++++++++++++++++++++++++++---
 1 files changed, 77 insertions(+), 5 deletions(-)

diff --git a/drivers/mtd/cmdlinepart.c b/drivers/mtd/cmdlinepart.c
index edd17e0..3473af5 100644
--- a/drivers/mtd/cmdlinepart.c
+++ b/drivers/mtd/cmdlinepart.c
@@ -225,6 +225,53 @@ static inline void sort_partitons(struct mtd_partition *parts, int num_parts)
 	return;
 }
 
+enum partition_check_result {
+	PARTITION_GOOD,
+	PARTITION_OVERLAPPED,
+	PARTITION_HOLE,
+	PARTITION_TOO_MUCH_CONT,
+};
+
+/* The partitions have been sorted well by the @offset. */
+static inline int check_partitions(struct mtd_partition *parts, int num_parts)
+{
+	int i;
+	uint64_t offset = 0;
+	int offset_continuous_cnt = 0;
+
+	for (i = 0; i < num_parts; i++) {
+		if (parts[i].offset == OFFSET_CONTINUOUS) {
+			offset_continuous_cnt++;
+			continue;
+		}
+
+		/* find a hole. */
+		if (parts[i].offset > offset)
+			return PARTITION_HOLE;
+
+		/* find an overlapped partition. */
+		if (parts[i].offset < offset)
+			return PARTITION_OVERLAPPED;
+
+		offset += parts[i].size;
+	}
+
+	if (offset_continuous_cnt) {
+		/* We do not set partitions with the @offset in the cmdline. */
+		if (offset_continuous_cnt == num_parts)
+			return PARTITION_GOOD;
+
+		/* It's ok if there is only one OFFSET_CONTINUOUS partition. */
+		if (offset_continuous_cnt == 1)
+			return PARTITION_GOOD;
+
+		return PARTITION_TOO_MUCH_CONT;
+	}
+
+	/* We set with @offset for all the partitions in the cmdline. */
+	return PARTITION_GOOD;
+}
+
 /*
  * Parse the command line.
  */
@@ -238,6 +285,7 @@ static int mtdpart_setup_real(char *s)
 		struct mtd_partition *parts;
 		int mtd_id_len, num_parts;
 		char *p, *mtd_id;
+		enum partition_check_result ret;
 
 		mtd_id = s;
 
@@ -285,13 +333,37 @@ static int mtdpart_setup_real(char *s)
 		/* sort the partitions */
 		sort_partitons(parts, num_parts);
 
-		/* link into chain */
-		this_mtd->next = partitions;
-		partitions = this_mtd;
+		/* check the partitions */
+		ret = check_partitions(parts, num_parts);
+		switch (ret) {
+		case PARTITION_GOOD:
+			/* link into chain */
+			this_mtd->next = partitions;
+			partitions = this_mtd;
 
-		dbg(("mtdid=<%s> num_parts=<%d>\n",
-		     this_mtd->mtd_id, this_mtd->num_parts));
+			dbg(("mtdid=<%s> num_parts=<%d>\n",
+			     this_mtd->mtd_id, this_mtd->num_parts));
+			break;
 
+		case PARTITION_OVERLAPPED:
+			printk(KERN_ERR ERRP "%s:The partitions overlap,"
+				"please check the cmdline, and fix it.\n",
+				this_mtd->mtd_id);
+			break;
+
+		case PARTITION_HOLE:
+			printk(KERN_ERR ERRP "%s:There is a hole in the "
+				"partitions, please check the cmdline, "
+				"and fix it.\n",
+				this_mtd->mtd_id);
+			break;
+
+		case PARTITION_TOO_MUCH_CONT:
+			printk(KERN_ERR ERRP "%s:The offset is not right,"
+				"please check the cmdline, and fix it.\n",
+				this_mtd->mtd_id);
+			break;
+		}
 
 		/* EOS - we're done */
 		if (*s == 0)
-- 
1.7.0.4

Re: [PATCH v2 3/3] mtd: cmdlinepart: check the partitions

[Artem Bityutskiy]

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

On Tue, 2012-09-04 at 15:43 +0800, Huang Shijie wrote:
> +enum partition_check_result {
> +	PARTITION_GOOD,
> +	PARTITION_OVERLAPPED,
> +	PARTITION_HOLE,
> +	PARTITION_TOO_MUCH_CONT,
> +};

Could you please kill these constants, they are not really needed I
think.

> +/* The partitions have been sorted well by the @offset. */
> +static inline int check_partitions(struct mtd_partition *parts, int num_parts)
> +{
> +	int i;
> +	uint64_t offset = 0;
> +	int offset_continuous_cnt = 0;
> +
> +	for (i = 0; i < num_parts; i++) {
> +		if (parts[i].offset == OFFSET_CONTINUOUS) {
> +			offset_continuous_cnt++;
> +			continue;
> +		}
> +
> +		/* find a hole. */
> +		if (parts[i].offset > offset)
> +			return PARTITION_HOLE;

Just print the error messages in this function and return -EINVAL.


> @@ -285,13 +333,37 @@ static int mtdpart_setup_real(char *s)
>  		/* sort the partitions */
>  		sort_partitons(parts, num_parts);
>  
> -		/* link into chain */
> -		this_mtd->next = partitions;
> -		partitions = this_mtd;
> +		/* check the partitions */
> +		ret = check_partitions(parts, num_parts);

Just do

if (ret)
	return ret;


and kill the huge 'switch' that you added.

-- 
Best Regards,
Artem Bityutskiy

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

Re: [PATCH v2 3/3] mtd: cmdlinepart: check the partitions

[Huang Shijie]

于 2012年09月04日 16:19, Artem Bityutskiy 写道:
> Just do
>
> if (ret)
> 	return ret;
>
>
> and kill the huge 'switch' that you added.
If the cmdline have two mtd-ids such as:
   
gpmi-nand:100m(root),100m@100m(kernel),1g@200m(rootfs);edb7212-nand:-(home)

The `return` will also bypass the `edb7212-nand`.

Is it acceptable?

Best Regards
Huang Shijie

Re: [PATCH v2 3/3] mtd: cmdlinepart: check the partitions

[Artem Bityutskiy]

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

On Tue, 2012-09-04 at 16:25 +0800, Huang Shijie wrote:
> 于 2012年09月04日 16:19, Artem Bityutskiy 写道:
> > Just do
> >
> > if (ret)
> > 	return ret;
> >
> >
> > and kill the huge 'switch' that you added.
> If the cmdline have two mtd-ids such as:
>    
> gpmi-nand:100m(root),100m@100m(kernel),1g@200m(rootfs);edb7212-nand:-(home)
> 
> The `return` will also bypass the `edb7212-nand`.
> 
> Is it acceptable?

I thought the whole idea is to refuse command lines which are ambiguous
or make no sense, no?

You could actually do this in two step and split your 3rd patch:

1. Add 'check_partitions' which prints an error messages and returns
-EINVAL if the command line has overlaps or gaps or other bad things.
But ignore the return code in 'mtdpart_setup_real()'

2. Make 'mtdpart_setup_real()' check for the error code and propagate it
up, so we'll have a prink + real refusal.

-- 
Best Regards,
Artem Bityutskiy

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

[PATCH v3] mtd: cmdlinepart: check the partitions

[Huang Shijie]

The partitions are all sorted well by the @offset.
But we should check if there are errors in the partitions.

The following cases are regarded as errors:
  [1] There is a hole in the partitions, such as
      #gpmi-nand:100m(boot),100m@100m(kernel),1g@200m(rootfs)

  [2] There is a overlap in the partitions, such as
      #gpmi-nand:100m@0(boot),100m@50m(kernel),1g@150m(rootfs)

  [3] Not all the partitions are set with @offset, and there are
      more then one partion whose offset is OFFSET_CONTINUOUS.
      #gpmi-nand:100m@0(boot),100m(kernel),-(usr)

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

diff --git a/drivers/mtd/cmdlinepart.c b/drivers/mtd/cmdlinepart.c
index edd17e0..5a603d1 100644
--- a/drivers/mtd/cmdlinepart.c
+++ b/drivers/mtd/cmdlinepart.c
@@ -225,6 +225,55 @@ static inline void sort_partitons(struct mtd_partition *parts, int num_parts)
 	return;
 }
 
+/* The partitions have been sorted well by the @offset. */
+static inline int check_partitions(struct mtd_partition *parts, int num_parts)
+{
+	int i;
+	uint64_t offset = 0;
+	int offset_continuous_cnt = 0;
+
+	for (i = 0; i < num_parts; i++) {
+		if (parts[i].offset == OFFSET_CONTINUOUS) {
+			offset_continuous_cnt++;
+			continue;
+		}
+
+		/* find a hole. */
+		if (parts[i].offset > offset) {
+			printk(KERN_ERR ERRP "There is a hole in the "
+				"partitions, please check the cmdline, "
+				"and fix it.\n");
+			return -EINVAL;
+		}
+
+		/* find an overlapped partition. */
+		if (parts[i].offset < offset) {
+			printk(KERN_ERR ERRP "The partitions overlap,"
+				"please check the cmdline, and fix it.\n");
+			return -EINVAL;
+		}
+
+		offset += parts[i].size;
+	}
+
+	if (offset_continuous_cnt) {
+		/* We do not set partitions with the @offset in the cmdline. */
+		if (offset_continuous_cnt == num_parts)
+			return 0;
+
+		/* It's ok if there is only one OFFSET_CONTINUOUS partition. */
+		if (offset_continuous_cnt == 1)
+			return 0;
+
+		printk(KERN_ERR ERRP "The offset is not right,"
+			"please check the cmdline, and fix it.\n");
+		return -EINVAL;
+	}
+
+	/* We set with @offset for all the partitions in the cmdline. */
+	return 0;
+}
+
 /*
  * Parse the command line.
  */
@@ -238,6 +287,7 @@ static int mtdpart_setup_real(char *s)
 		struct mtd_partition *parts;
 		int mtd_id_len, num_parts;
 		char *p, *mtd_id;
+		int ret;
 
 		mtd_id = s;
 
@@ -285,6 +335,11 @@ static int mtdpart_setup_real(char *s)
 		/* sort the partitions */
 		sort_partitons(parts, num_parts);
 
+		/* check the partitions */
+		ret = check_partitions(parts, num_parts);
+		if (ret)
+			return ret;
+
 		/* link into chain */
 		this_mtd->next = partitions;
 		partitions = this_mtd;
-- 
1.7.0.4