| Message ID | 1346001700-26895-1-git-send-email-shijie8@gmail.com |
|---|---|
| State | New, archived |
| Headers | show |
On Sun, 2012-08-26 at 13:21 -0400, Huang Shijie wrote: > There are typically two types to set the mtd partitions: > > <1> set with the `size`, such as > gpmi-nand:100m(boot),100m(kernel),1g(rootfs) > > <2> set with the `offset`, such as > gpmi-nand:100m@0(boot),100m@100m(kernel),1g@200m(rootfs) > gpmi-nand:1g@200m(rootfs),100m@0(boot),100m@100m(kernel) > > If we mix these two types, such as: > gpmi-nand:100m@0(boot),100m(kernel),1g@200m(rootfs) > gpmi-nand:1g@200m(rootfs),100m@0(boot),100m(kernel) > > It's hard to understand the cmdline. And also it is hard to sort the > partitions in this mixed type. So we explicitly forbid the mixed type. So "explicitly forbid" is just to add a "do not do this" comment?
On Fri, Aug 31, 2012 at 7:45 AM, Artem Bityutskiy <dedekind1@gmail.com> wrote: > On Sun, 2012-08-26 at 13:21 -0400, Huang Shijie wrote: >> There are typically two types to set the mtd partitions: >> >> <1> set with the `size`, such as >> gpmi-nand:100m(boot),100m(kernel),1g(rootfs) >> >> <2> set with the `offset`, such as >> gpmi-nand:100m@0(boot),100m@100m(kernel),1g@200m(rootfs) >> gpmi-nand:1g@200m(rootfs),100m@0(boot),100m@100m(kernel) >> >> If we mix these two types, such as: >> gpmi-nand:100m@0(boot),100m(kernel),1g@200m(rootfs) >> gpmi-nand:1g@200m(rootfs),100m@0(boot),100m(kernel) >> >> It's hard to understand the cmdline. And also it is hard to sort the >> partitions in this mixed type. So we explicitly forbid the mixed type. > > So "explicitly forbid" is just to add a "do not do this" comment? > This is the simplest way. ;) It's make code complicated if we change the code to forbid this mixed type. Best Regards Huang Shijie
On Fri, Aug 31, 2012 at 7:45 AM, Artem Bityutskiy <dedekind1@gmail.com> wrote: > On Sun, 2012-08-26 at 13:21 -0400, Huang Shijie wrote: >> There are typically two types to set the mtd partitions: >> >> <1> set with the `size`, such as >> gpmi-nand:100m(boot),100m(kernel),1g(rootfs) >> >> <2> set with the `offset`, such as >> gpmi-nand:100m@0(boot),100m@100m(kernel),1g@200m(rootfs) >> gpmi-nand:1g@200m(rootfs),100m@0(boot),100m@100m(kernel) >> >> If we mix these two types, such as: >> gpmi-nand:100m@0(boot),100m(kernel),1g@200m(rootfs) >> gpmi-nand:1g@200m(rootfs),100m@0(boot),100m(kernel) >> >> It's hard to understand the cmdline. And also it is hard to sort the >> partitions in this mixed type. So we explicitly forbid the mixed type. > > So "explicitly forbid" is just to add a "do not do this" comment? > Do you think we should change the code to forbid this mixed type? Huang Shijie
On Sun, 2012-08-26 at 13:21 -0400, Huang Shijie wrote: > + * > + * Note: > + * If you choose to set the @offset for the <partdef>, please set all > + * the partitions with the same syntax, such as: > + * gpmi-nand:100m@0(boot),100m@100m(kernel),1g@200m(rootfs) > + * > + * Please do _NOT_ set the partitions like this: > + * gpmi-nand:100m@0(boot),100m(kernel),1g@200m(rootfs) > + * The `kernel` partition does not set with the @offset, this is not permitted. > */ I guess it is indeed OK to sort the partitions, just makes things a lot simpler. But we probably then should also do the following: 1. Make sure there is only one partition without offset. If there are several - error out. 2. Check that partitions do not intersect - I did not notice that we do this in the code. So AFAICS, this patch is not needed and we better have the following patches: 1. Add sorting 2. Add a check that partitions do not overlap and there is only one offset-less partition. How does this sound?
On Mon, Sep 3, 2012 at 3:18 AM, Artem Bityutskiy <dedekind1@gmail.com> wrote: > On Sun, 2012-08-26 at 13:21 -0400, Huang Shijie wrote: >> + * >> + * Note: >> + * If you choose to set the @offset for the <partdef>, please set all >> + * the partitions with the same syntax, such as: >> + * gpmi-nand:100m@0(boot),100m@100m(kernel),1g@200m(rootfs) >> + * >> + * Please do _NOT_ set the partitions like this: >> + * gpmi-nand:100m@0(boot),100m(kernel),1g@200m(rootfs) >> + * The `kernel` partition does not set with the @offset, this is not permitted. >> */ > > I guess it is indeed OK to sort the partitions, just makes things a lot > simpler. But we probably then should also do the following: > > 1. Make sure there is only one partition without offset. If there are > several - error out. Why allow `only one partition without offset`? Take the following unsorted partitions as an example: #gpmi-nand:1g@200m(rootfs),100m(boot),100m@100m(kernel) The current code will parses out the following partitions: rootfs: < size = 1g, offset=200m> boot: < size = 100m, offset= OFFSET_CONTINOUS> kernel: <size = 100m, offset = 100m> If we sort the partitions by the offset, we get the following result: "kernel" , "rootfs", "boot" In actually, we expect the following result: "boot", "kernel", "rootfs" What's your idea about this issue? In my opinion, we should forbid the mixed type. thanks Huang Shijie > 2. Check that partitions do not intersect - I did not notice that we do > this in the code. > > So AFAICS, this patch is not needed and we better have the following > patches: > > 1. Add sorting > 2. Add a check that partitions do not overlap and there is only one > offset-less partition. > > How does this sound? > > -- > Best Regards, > Artem Bityutskiy
[Dropped extra CCs - let's spam less] On Mon, 2012-09-03 at 11:09 -0400, Huang Shijie wrote: > On Mon, Sep 3, 2012 at 3:18 AM, Artem Bityutskiy <dedekind1@gmail.com> wrote: > > On Sun, 2012-08-26 at 13:21 -0400, Huang Shijie wrote: > >> + * > >> + * Note: > >> + * If you choose to set the @offset for the <partdef>, please set all > >> + * the partitions with the same syntax, such as: > >> + * gpmi-nand:100m@0(boot),100m@100m(kernel),1g@200m(rootfs) > >> + * > >> + * Please do _NOT_ set the partitions like this: > >> + * gpmi-nand:100m@0(boot),100m(kernel),1g@200m(rootfs) > >> + * The `kernel` partition does not set with the @offset, this is not permitted. > >> */ > > > > I guess it is indeed OK to sort the partitions, just makes things a lot > > simpler. But we probably then should also do the following: > > > > 1. Make sure there is only one partition without offset. If there are > > several - error out. > > Why allow `only one partition without offset`? E.g., gpmi-nand:1g@200m(rootfs),100m(boot),100m@100m(kernel),200m(ququ) how would "boot" and "ququ" look like? What I basically say is that we should refuse lines like this. Which means checking that there is only one "OFFSET_CONTINOUS" partition. > Take the following unsorted partitions as an example: > #gpmi-nand:1g@200m(rootfs),100m(boot),100m@100m(kernel) > > The current code will parses out the following partitions: > rootfs: < size = 1g, offset=200m> > boot: < size = 100m, offset= OFFSET_CONTINOUS> > kernel: <size = 100m, offset = 100m> > > If we sort the partitions by the offset, we get the following result: > "kernel" , "rootfs", "boot" Yeah, for some reason I assumed that if you do not specify offset then you mean the partition must be the _last_ and span the rest of flash. I assume other usage is bogus. So from this POW the example is bogus and lines like this would be rejected. I guess the criteria is that this sting contains a "gap" (0-100m). With the logic I suggested, for this case what I said would basically: 1. Sort. This leads to 100m@100m(kernel) 1g@200m(rootfs) 100m(boot) "OFFSET_CONTINOUS" is always the largest and will be at the end when sorting. 2. Verification. We verify for overlaps and gaps. And refuse this one.
于 2012年09月03日 23:35, Artem Bityutskiy 写道: > 1. Sort. This leads to > > 100m@100m(kernel) > 1g@200m(rootfs) > 100m(boot) > > "OFFSET_CONTINOUS" is always the largest and will be at the end when > sorting. > ok. > 2. Verification. > > We verify for overlaps and gaps. And refuse this one. > It's really necessary to check the overlaps. I will try to send out the new patch set. Best Regards Huang Shijie
diff --git a/drivers/mtd/cmdlinepart.c b/drivers/mtd/cmdlinepart.c index fe7e3a5..0b7b2ad 100644 --- a/drivers/mtd/cmdlinepart.c +++ b/drivers/mtd/cmdlinepart.c @@ -35,6 +35,15 @@ * * 1 NOR Flash with 2 partitions, 1 NAND with one * edb7312-nor:256k(ARMboot)ro,-(root);edb7312-nand:-(home) + * + * Note: + * If you choose to set the @offset for the <partdef>, please set all + * the partitions with the same syntax, such as: + * gpmi-nand:100m@0(boot),100m@100m(kernel),1g@200m(rootfs) + * + * Please do _NOT_ set the partitions like this: + * gpmi-nand:100m@0(boot),100m(kernel),1g@200m(rootfs) + * The `kernel` partition does not set with the @offset, this is not permitted. */ #include <linux/kernel.h>
There are typically two types to set the mtd partitions: <1> set with the `size`, such as gpmi-nand:100m(boot),100m(kernel),1g(rootfs) <2> set with the `offset`, such as gpmi-nand:100m@0(boot),100m@100m(kernel),1g@200m(rootfs) gpmi-nand:1g@200m(rootfs),100m@0(boot),100m@100m(kernel) If we mix these two types, such as: gpmi-nand:100m@0(boot),100m(kernel),1g@200m(rootfs) gpmi-nand:1g@200m(rootfs),100m@0(boot),100m(kernel) It's hard to understand the cmdline. And also it is hard to sort the partitions in this mixed type. So we explicitly forbid the mixed type. Signed-off-by: Huang Shijie <shijie8@gmail.com> --- drivers/mtd/cmdlinepart.c | 9 +++++++++ 1 files changed, 9 insertions(+), 0 deletions(-)