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 <[email protected]>
---
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
Hi Huang,
On Sat, 25 Aug 2012 10:26:07 -0400 Huang Shijie <[email protected]> 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
On Sat, Aug 25, 2012 at 5:02 AM, Shmulik Ladkani
<[email protected]> wrote:
> Hi Huang,
>
> On Sat, 25 Aug 2012 10:26:07 -0400 Huang Shijie <[email protected]> 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
On Sat, 25 Aug 2012 05:26:51 -0400 Huang Shijie <[email protected]> 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
On Sat, Aug 25, 2012 at 5:42 AM, Shmulik Ladkani
<[email protected]> wrote:
> On Sat, 25 Aug 2012 05:26:51 -0400 Huang Shijie <[email protected]> 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
Hi,
On Sat, 25 Aug 2012 05:26:51 -0400 Huang Shijie <[email protected]> wrote:
> On Sat, Aug 25, 2012 at 5:02 AM, Shmulik Ladkani
> <[email protected]> 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
On Sun, Aug 26, 2012 at 2:06 AM, Shmulik Ladkani
<[email protected]> wrote:
> Hi,
>
> On Sat, 25 Aug 2012 05:26:51 -0400 Huang Shijie <[email protected]> wrote:
>> On Sat, Aug 25, 2012 at 5:02 AM, Shmulik Ladkani
>> <[email protected]> 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
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
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
于 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
On Wed, 29 Aug 2012 11:16:05 +0300 Artem Bityutskiy <[email protected]> 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
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