* [PATCH v2] mtd: cmdlinepart: fix the wrong partitions number when truncating occurs @ 2012-08-25 14:26 Huang Shijie 2012-08-25 9:02 ` Shmulik Ladkani 0 siblings, 1 reply; 12+ messages in thread From: Huang Shijie @ 2012-08-25 14:26 UTC (permalink / raw) To: dwmw2; +Cc: linux-mtd, linux-kernel, dedekind1, 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 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH v2] mtd: cmdlinepart: fix the wrong partitions number when truncating occurs 2012-08-25 14:26 [PATCH v2] mtd: cmdlinepart: fix the wrong partitions number when truncating occurs Huang Shijie @ 2012-08-25 9:02 ` Shmulik Ladkani 2012-08-25 9:26 ` Huang Shijie 0 siblings, 1 reply; 12+ messages in thread From: Shmulik Ladkani @ 2012-08-25 9:02 UTC (permalink / raw) To: Huang Shijie; +Cc: dwmw2, linux-mtd, linux-kernel, dedekind1 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2] mtd: cmdlinepart: fix the wrong partitions number when truncating occurs 2012-08-25 9:02 ` Shmulik Ladkani @ 2012-08-25 9:26 ` Huang Shijie 2012-08-25 9:42 ` Shmulik Ladkani 2012-08-26 6:06 ` Shmulik Ladkani 0 siblings, 2 replies; 12+ messages in thread From: Huang Shijie @ 2012-08-25 9:26 UTC (permalink / raw) To: Shmulik Ladkani; +Cc: dwmw2, linux-mtd, linux-kernel, dedekind1 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2] mtd: cmdlinepart: fix the wrong partitions number when truncating occurs 2012-08-25 9:26 ` Huang Shijie @ 2012-08-25 9:42 ` Shmulik Ladkani 2012-08-25 11:07 ` Huang Shijie 2012-08-26 6:06 ` Shmulik Ladkani 1 sibling, 1 reply; 12+ messages in thread From: Shmulik Ladkani @ 2012-08-25 9:42 UTC (permalink / raw) To: Huang Shijie; +Cc: dwmw2, linux-mtd, linux-kernel, dedekind1 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2] mtd: cmdlinepart: fix the wrong partitions number when truncating occurs 2012-08-25 9:42 ` Shmulik Ladkani @ 2012-08-25 11:07 ` Huang Shijie 0 siblings, 0 replies; 12+ messages in thread From: Huang Shijie @ 2012-08-25 11:07 UTC (permalink / raw) To: Shmulik Ladkani; +Cc: dwmw2, linux-mtd, linux-kernel, dedekind1 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2] mtd: cmdlinepart: fix the wrong partitions number when truncating occurs 2012-08-25 9:26 ` Huang Shijie 2012-08-25 9:42 ` Shmulik Ladkani @ 2012-08-26 6:06 ` Shmulik Ladkani 2012-08-26 6:47 ` Huang Shijie 2012-08-29 8:16 ` Artem Bityutskiy 1 sibling, 2 replies; 12+ messages in thread From: Shmulik Ladkani @ 2012-08-26 6:06 UTC (permalink / raw) To: Huang Shijie; +Cc: dwmw2, linux-mtd, linux-kernel, dedekind1 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2] mtd: cmdlinepart: fix the wrong partitions number when truncating occurs 2012-08-26 6:06 ` Shmulik Ladkani @ 2012-08-26 6:47 ` Huang Shijie 2012-08-29 8:24 ` Artem Bityutskiy 2012-08-29 8:16 ` Artem Bityutskiy 1 sibling, 1 reply; 12+ messages in thread From: Huang Shijie @ 2012-08-26 6:47 UTC (permalink / raw) To: Shmulik Ladkani; +Cc: dwmw2, linux-mtd, linux-kernel, dedekind1 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2] mtd: cmdlinepart: fix the wrong partitions number when truncating occurs 2012-08-26 6:47 ` Huang Shijie @ 2012-08-29 8:24 ` Artem Bityutskiy 2012-08-29 8:48 ` Huang Shijie 0 siblings, 1 reply; 12+ messages in thread From: Artem Bityutskiy @ 2012-08-29 8:24 UTC (permalink / raw) To: Huang Shijie; +Cc: Shmulik Ladkani, dwmw2, linux-mtd, linux-kernel [-- 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 --] ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2] mtd: cmdlinepart: fix the wrong partitions number when truncating occurs 2012-08-29 8:24 ` Artem Bityutskiy @ 2012-08-29 8:48 ` Huang Shijie 0 siblings, 0 replies; 12+ messages in thread From: Huang Shijie @ 2012-08-29 8:48 UTC (permalink / raw) To: dedekind1; +Cc: Huang Shijie, linux-mtd, dwmw2, Shmulik Ladkani, linux-kernel 于 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2] mtd: cmdlinepart: fix the wrong partitions number when truncating occurs 2012-08-26 6:06 ` Shmulik Ladkani 2012-08-26 6:47 ` Huang Shijie @ 2012-08-29 8:16 ` Artem Bityutskiy 2012-08-29 8:51 ` Shmulik Ladkani 1 sibling, 1 reply; 12+ messages in thread From: Artem Bityutskiy @ 2012-08-29 8:16 UTC (permalink / raw) To: Shmulik Ladkani; +Cc: Huang Shijie, dwmw2, linux-mtd, linux-kernel [-- 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 --] ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2] mtd: cmdlinepart: fix the wrong partitions number when truncating occurs 2012-08-29 8:16 ` Artem Bityutskiy @ 2012-08-29 8:51 ` Shmulik Ladkani 2012-08-29 9:10 ` Artem Bityutskiy 0 siblings, 1 reply; 12+ messages in thread From: Shmulik Ladkani @ 2012-08-29 8:51 UTC (permalink / raw) To: dedekind1; +Cc: Huang Shijie, dwmw2, linux-mtd, linux-kernel 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2] mtd: cmdlinepart: fix the wrong partitions number when truncating occurs 2012-08-29 8:51 ` Shmulik Ladkani @ 2012-08-29 9:10 ` Artem Bityutskiy 0 siblings, 0 replies; 12+ messages in thread From: Artem Bityutskiy @ 2012-08-29 9:10 UTC (permalink / raw) To: Shmulik Ladkani; +Cc: Huang Shijie, dwmw2, linux-mtd, linux-kernel [-- 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 --] ^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2012-08-29 9:05 UTC | newest] Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2012-08-25 14:26 [PATCH v2] mtd: cmdlinepart: fix the wrong partitions number when truncating occurs Huang Shijie 2012-08-25 9:02 ` Shmulik Ladkani 2012-08-25 9:26 ` Huang Shijie 2012-08-25 9:42 ` Shmulik Ladkani 2012-08-25 11:07 ` Huang Shijie 2012-08-26 6:06 ` Shmulik Ladkani 2012-08-26 6:47 ` Huang Shijie 2012-08-29 8:24 ` Artem Bityutskiy 2012-08-29 8:48 ` Huang Shijie 2012-08-29 8:16 ` Artem Bityutskiy 2012-08-29 8:51 ` Shmulik Ladkani 2012-08-29 9:10 ` Artem Bityutskiy
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).