| Message ID | 1345904767-23011-1-git-send-email-shijie8@gmail.com |
|---|---|
| State | New, archived |
| Headers | show |
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
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
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
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
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
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
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?
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.
于 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 <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
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.
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; }
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(-)