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

Submitted by Huang Shijie on Aug. 25, 2012, 2:26 p.m.

Details

Message ID 1345904767-23011-1-git-send-email-shijie8@gmail.com
State New, archived
Headers show

Commit Message

Huang Shijie Aug. 25, 2012, 2:26 p.m.
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(-)

Comments

Shmulik Ladkani Aug. 25, 2012, 9:02 a.m.
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
Huang Shijie Aug. 25, 2012, 9:26 a.m.
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
Shmulik Ladkani Aug. 25, 2012, 9:42 a.m.
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
Huang Shijie Aug. 25, 2012, 11:07 a.m.
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
Shmulik Ladkani Aug. 26, 2012, 6:06 a.m.
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
Huang Shijie Aug. 26, 2012, 6:47 a.m.
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
Artem Bityutskiy Aug. 29, 2012, 8:16 a.m.
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 Bityutskiy Aug. 29, 2012, 8:24 a.m.
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.
Huang Shijie Aug. 29, 2012, 8:48 a.m.
于 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
Shmulik Ladkani Aug. 29, 2012, 8:51 a.m.
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
Artem Bityutskiy Aug. 29, 2012, 9:10 a.m.
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.

Patch hide | download patch | download mbox

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