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

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

[Shmulik Ladkani]

Hi Huang,

On Sat, 25 Aug 2012 10:26:07 -0400 Huang Shijie <shijie8@gmail.com> wrote:
> diff --git a/drivers/mtd/cmdlinepart.c b/drivers/mtd/cmdlinepart.c
> index 4558e0f..fc960a3 100644
> --- a/drivers/mtd/cmdlinepart.c
> +++ b/drivers/mtd/cmdlinepart.c
> @@ -344,7 +344,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;

Your analysis seems right, but let me offer an alternative approach.

I would simply:

-					part->num_parts = i;

(and not replace it with anything).

The specified cmdline partitions might not be ordered (according to
start offset), so next partition specified after the truncated one might
define a partition at the beginning of the device, which is okay
(regardless the truncation of current partition).

Your patch skips the definitions of next partitions, which can be legit.

I agree specifying "unsorted" partitions is not commonly used (and it
might make no sense when using the "remaining" syntax), but it is legit
to define all partitions _explicitly_ with their size@offset in an
unordered fashion.

Regards,
Shmulik

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

[Huang Shijie]

On Sat, Aug 25, 2012 at 5:02 AM, Shmulik Ladkani
<shmulik.ladkani@gmail.com> wrote:
> Hi Huang,
>
> On Sat, 25 Aug 2012 10:26:07 -0400 Huang Shijie <shijie8@gmail.com> wrote:
>> diff --git a/drivers/mtd/cmdlinepart.c b/drivers/mtd/cmdlinepart.c
>> index 4558e0f..fc960a3 100644
>> --- a/drivers/mtd/cmdlinepart.c
>> +++ b/drivers/mtd/cmdlinepart.c
>> @@ -344,7 +344,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;
>
> Your analysis seems right, but let me offer an alternative approach.
>
> I would simply:
>
> -                                       part->num_parts = i;
your code does not wors in such kernel command line(also with the 1GB
nand chip):
#gpmi-nand:100m(root),100m(kernel),1g(rootfs),1g(user),-(rest)

For you see, we must keep the code robust enough. It should passes all
the possible kernel command lines.




>
> (and not replace it with anything).
>
> The specified cmdline partitions might not be ordered (according to
> start offset), so next partition specified after the truncated one might
> define a partition at the beginning of the device, which is okay
> (regardless the truncation of current partition).
could you please give me an example of this specified cmdline?

I can test it.

Best Regards
Huang Shijie

>
> Your patch skips the definitions of next partitions, which can be legit.
>
> I agree specifying "unsorted" partitions is not commonly used (and it
> might make no sense when using the "remaining" syntax), but it is legit
> to define all partitions _explicitly_ with their size@offset in an
> unordered fashion.
>
> Regards,
> Shmulik

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

[Shmulik Ladkani]

On Sat, 25 Aug 2012 05:26:51 -0400 Huang Shijie <shijie8@gmail.com> wrote:
> > The specified cmdline partitions might not be ordered (according to
> > start offset), so next partition specified after the truncated one might
> > define a partition at the beginning of the device, which is okay
> > (regardless the truncation of current partition).
> could you please give me an example of this specified cmdline?

Assume we have a 1GB(8Gb) nand chip:
  #gpmi-nand:1g@200m(rootfs),100m@0(boot),100m@100m(kernel)

I am used to explicitly specify size@offset for all my parts.

Obviously I won't define a partition above the device size... somewhat
hypothetical discussion here...

But your code will stop after creating 'rootfs' (and original code will
not create a single partition).

Is that what we want?

Or do we want to truncate 'rootfs', but still have the valid 'boot' and
'kerner' partitions?

Regards,
Shmulik

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

[Huang Shijie]

On Sat, Aug 25, 2012 at 5:42 AM, Shmulik Ladkani
<shmulik.ladkani@gmail.com> wrote:
> On Sat, 25 Aug 2012 05:26:51 -0400 Huang Shijie <shijie8@gmail.com> wrote:
>> > The specified cmdline partitions might not be ordered (according to
>> > start offset), so next partition specified after the truncated one might
>> > define a partition at the beginning of the device, which is okay
>> > (regardless the truncation of current partition).
>> could you please give me an example of this specified cmdline?
>
> Assume we have a 1GB(8Gb) nand chip:
>   #gpmi-nand:1g@200m(rootfs),100m@0(boot),100m@100m(kernel)
>
> I am used to explicitly specify size@offset for all my parts.

thanks for this example.

I tested it just now.
The current code (without my patch) can not parse out none of the partitions.
It  directly stops at the first truncated `rootfs` partition.

I think i should send another patch to sort all the partitions.

thanks a lot.

Huang Shijie

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

[Huang Shijie]

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>
---
v1 --> v2:
	[1] add more commit info.
---
 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 4558e0f..fc960a3 100644
--- a/drivers/mtd/cmdlinepart.c
+++ b/drivers/mtd/cmdlinepart.c
@@ -344,7 +344,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 v2] mtd: cmdlinepart: fix the wrong partitions number when truncating occurs

[Shmulik Ladkani]

Hi,

On Sat, 25 Aug 2012 05:26:51 -0400 Huang Shijie <shijie8@gmail.com> wrote:
> On Sat, Aug 25, 2012 at 5:02 AM, Shmulik Ladkani
> <shmulik.ladkani@gmail.com> wrote:
> > Your analysis seems right, but let me offer an alternative approach.
> >
> > I would simply:
> >
> > -                                       part->num_parts = i;
> your code does not wors in such kernel command line(also with the 1GB
> nand chip):
> #gpmi-nand:100m(root),100m(kernel),1g(rootfs),1g(user),-(rest)
> 

Can you please detail what do you mean by "not work"?

To my understanding, in this example, according to my suggestion, the
resulting partitions would be:

root	100m@0
kernel	100m@100m
rootfs	800m@200m (truncated)
user	0@1g (truncated)
rest	0@1g

Reasonable IMO, given the fact that the mtd device size is smaller than
the specified parts.

I saw you submitted a patch which sorts the cmdline parts; I don't
understand why this is necessary.
Also, sorting might not be desirable, as the user specified the unsorted
partitions might have _wanted_ them to appear in that order.

Now lets focus on your original suggestion and its consequences:

- Orignal code STOPPED parsing at the 1st truncated partition,
  this partition WAS NOT returned to the caller
- Your patch STOPS AFTER parsing the 1st truncated partition,
  this partiton IS returned to the caller (but partitions specified
  later are no longer parsed)
- My suggestion CONTINUES parsing all partitions.
  So later partitions (specified with the 'size' but *without* 'offset')
  will be truncated AND presented to the caller.
  AND, if later partitions are specified using the 'size@offset'
  explicit format, they are parsed normally.
  
Regards,
Shmulik

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

[Huang Shijie]

On Sun, Aug 26, 2012 at 2:06 AM, Shmulik Ladkani
<shmulik.ladkani@gmail.com> wrote:
> Hi,
>
> On Sat, 25 Aug 2012 05:26:51 -0400 Huang Shijie <shijie8@gmail.com> wrote:
>> On Sat, Aug 25, 2012 at 5:02 AM, Shmulik Ladkani
>> <shmulik.ladkani@gmail.com> wrote:
>> > Your analysis seems right, but let me offer an alternative approach.
>> >
>> > I would simply:
>> >
>> > -                                       part->num_parts = i;
>> your code does not wors in such kernel command line(also with the 1GB
>> nand chip):
>> #gpmi-nand:100m(root),100m(kernel),1g(rootfs),1g(user),-(rest)
>>
>
> Can you please detail what do you mean by "not work"?
sorry. :)

My meaning was the result is unreadable. It looks too strange. That's
why i use the 'break'
to shortcut the loop.

>
> To my understanding, in this example, according to my suggestion, the
> resulting partitions would be:
>
> root    100m@0
> kernel  100m@100m
> rootfs  800m@200m (truncated)
> user    0@1g (truncated)
> rest    0@1g
>

yes, the result is like this.

> Reasonable IMO, given the fact that the mtd device size is smaller than

> the specified parts.
>
> I saw you submitted a patch which sorts the cmdline parts; I don't
> understand why this is necessary.
> Also, sorting might not be desirable, as the user specified the unsorted
> partitions might have _wanted_ them to appear in that order.
>
> Now lets focus on your original suggestion and its consequences:
>
> - Orignal code STOPPED parsing at the 1st truncated partition,
>   this partition WAS NOT returned to the caller
> - Your patch STOPS AFTER parsing the 1st truncated partition,
>   this partiton IS returned to the caller (but partitions specified
>   later are no longer parsed)
> - My suggestion CONTINUES parsing all partitions.
>   So later partitions (specified with the 'size' but *without* 'offset')
>   will be truncated AND presented to the caller.
>   AND, if later partitions are specified using the 'size@offset'
>   explicit format, they are parsed normally.
>

Could Artem or David point us a direction about this?
[1]  Should the unsorted partitions be supported?
[2] And what should we do when we meet a 1GB nand, and the cmdline is
      gpmi-nand:100m(root),100m(kernel),1g(rootfs),1g(user),-(rest)

thanks a lot
Huang Shijie

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

[Artem Bityutskiy]

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

On Sun, 2012-08-26 at 09:06 +0300, Shmulik Ladkani wrote:
> root	100m@0
> kernel	100m@100m
> rootfs	800m@200m (truncated)
> user	0@1g (truncated)
> rest	0@1g

Who would benefit from having those 2 0-sized partitions and how? How
many users/scripts would be confused by this (these 2 ghost partitions
would be visible in /proc/mtd and sysfs)? How much RAM would we spend
for creating sysfs files and directories for these ghost partitions
(note, one sysfs file costs a couple KiB I thing, because 'sizeof
(struct inode)').

While you suggestion is clever, do we really benefit from this?

-- 
Best Regards,
Artem Bityutskiy

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

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

[Artem Bityutskiy]

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

While appreciating Shmulik's attention to details, I vote this way:

On Sun, 2012-08-26 at 02:47 -0400, Huang Shijie wrote:
> Could Artem or David point us a direction about this?
> [1]  Should the unsorted partitions be supported?

No, it is asking for troubles.

> [2] And what should we do when we meet a 1GB nand, and the cmdline is
>       gpmi-nand:100m(root),100m(kernel),1g(rootfs),1g(user),-(rest) 

Drop 'user' and 'rest' - I've sent some explanations in the previous
e-mail.

-- 
Best Regards,
Artem Bityutskiy

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

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

[Huang Shijie]

于 2012年08月29日 16:24, Artem Bityutskiy 写道:
> While appreciating Shmulik's attention to details, I vote this way:
>
> On Sun, 2012-08-26 at 02:47 -0400, Huang Shijie wrote:
>> Could Artem or David point us a direction about this?
>> [1]  Should the unsorted partitions be supported?
> No, it is asking for troubles.
>
thanks.
>> [2] And what should we do when we meet a 1GB nand, and the cmdline is
>>        gpmi-nand:100m(root),100m(kernel),1g(rootfs),1g(user),-(rest)
> Drop 'user' and 'rest' - I've sent some explanations in the previous
> e-mail.
>
ok, thanks.

I think this patch is enough to fix this issue.


Huang Shijie

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

[Shmulik Ladkani]

On Wed, 29 Aug 2012 11:16:05 +0300 Artem Bityutskiy <dedekind1@gmail.com> wrote:
> On Sun, 2012-08-26 at 09:06 +0300, Shmulik Ladkani wrote:
> > root	100m@0
> > kernel	100m@100m
> > rootfs	800m@200m (truncated)
> > user	0@1g (truncated)
> > rest	0@1g
> 
> Who would benefit from having those 2 0-sized partitions and how? How
> many users/scripts would be confused by this (these 2 ghost partitions
> would be visible in /proc/mtd and sysfs)? How much RAM would we spend
> for creating sysfs files and directories for these ghost partitions
> (note, one sysfs file costs a couple KiB I thing, because 'sizeof
> (struct inode)').
> 
> While you suggestion is clever, do we really benefit from this?

Artem, please note this is just a side effect of what I've suggested
(that its, continue parsing after first truncated partition), not a real
use case I'd expect and intentionally wish to support.

I am used to specify partitions explicitly using size@offset (specifying
offset for all parts, even if sometimes adjacent), and sometimes in an
unsorted fashion.
I never defined some partition that got truncated, so the whole
discussion is theoretical to _my_ usecase.

The only benefit of continuing to parse, is that if there _are_ later
partitions which are defined _explicitly_ with an offset, whose location
is _before_ the truncated partition, these would still be parsed and
registered.

Not so important, and would rarely happen, but simplistic and naive.

And reagrding 0 sized partitions, we can always skip these.

Regards,
Shmulik

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

[Artem Bityutskiy]

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

On Wed, 2012-08-29 at 11:51 +0300, Shmulik Ladkani wrote:
> > While you suggestion is clever, do we really benefit from this?
> 
> Artem, please note this is just a side effect of what I've suggested
> (that its, continue parsing after first truncated partition), not a real
> use case I'd expect and intentionally wish to support.

Sorry, I was not reading carefully enough. Could you please recap - are
you fine with Huang's latest patch-set or not :) To me sorting and then
dropping 0-size partitions looks like a simple and robust approach.

-- 
Best Regards,
Artem Bityutskiy

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