Patchwork [1/3] mtd: cmdlinepart: make the partitions rule more strict

login
register
mail settings
Submitter Shmulik Ladkani
Date Sept. 4, 2012, 11:48 a.m.
Message ID <20120904144826.7b9b7518@halley>
Download mbox | patch
Permalink /patch/181545/
State New
Headers show

Comments

Shmulik Ladkani - Sept. 4, 2012, 11:48 a.m.
On Mon, 03 Sep 2012 18:35:44 +0300 Artem Bityutskiy <dedekind1@gmail.com> wrote:
> 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.

Artem, Huang,

Sorry for the long delay, got busy with urgent matters (found out I need
to go thru some surgery), I will probably not be available in the
upcoming days, but I've read the correspondance and wanted to share my
two cents...

My POV is that sorting and verification is not needed, is troublesome,
and might affect users in ways they don't expect.

So far, mtdparts commandline parsing has been very lenient and liberal.
I think we should keep this approach; give the user the flexibility,
he'll be responsible to provide meaningful cmdline parts for his
system.

Actually, Huang's initial complaint was that 'parse_cmdline_partitions'
was too strict - the truncated partition was not registered!

Now, philosophy aside, let's talk about some usecases that might break.

I remember overlapping partitions (more precisely, partition that is a
subset of another) being common in some embedded systems, where the
"rootfs" and "kernel" partitions are a subset of the "overall image"
partition.
Why break this? if users enjoy the flexibility, and careful not to
create silly partitions, I'm in favor of keeping the flexibility.

Same goes for sorting.
If one has a system hacked to work with mtd0 hardcodedly, but mtd0's
physical location is somewhere at the end of the device, why reorder the
partitions enumeration affecting this system?

I'd say keep it simple and flexible.
I trust the user to provide meaningful partitions in the cmdline.

Anyways, please consider this approach.

My patch suggestion might look as (and might need some rework):


Regards,
Shmulik
Huang Shijie - Sept. 5, 2012, 2:12 a.m.
于 2012年09月04日 19:48, Shmulik Ladkani 写道:
> On Mon, 03 Sep 2012 18:35:44 +0300 Artem Bityutskiy<dedekind1@gmail.com>  wrote:
>> 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.
> Artem, Huang,
>
> Sorry for the long delay, got busy with urgent matters (found out I need
> to go thru some surgery), I will probably not be available in the
> upcoming days, but I've read the correspondance and wanted to share my
> two cents...
>
> My POV is that sorting and verification is not needed, is troublesome,
> and might affect users in ways they don't expect.
>
> So far, mtdparts commandline parsing has been very lenient and liberal.
> I think we should keep this approach; give the user the flexibility,
> he'll be responsible to provide meaningful cmdline parts for his
> system.
>
> Actually, Huang's initial complaint was that 'parse_cmdline_partitions'
> was too strict - the truncated partition was not registered!
>
> Now, philosophy aside, let's talk about some usecases that might break.
>
> I remember overlapping partitions (more precisely, partition that is a
> subset of another) being common in some embedded systems, where the
> "rootfs" and "kernel" partitions are a subset of the "overall image"
> partition.
> Why break this? if users enjoy the flexibility, and careful not to
> create silly partitions, I'm in favor of keeping the flexibility.
>
> Same goes for sorting.
> If one has a system hacked to work with mtd0 hardcodedly, but mtd0's
> physical location is somewhere at the end of the device, why reorder the
> partitions enumeration affecting this system?
>
> I'd say keep it simple and flexible.
yes, it's really simple and flexible.
But I think it's not wise to export the 0-size partition to user.
The user may confused at this.

And the mtd-utils, such as mtd-info/mtd-debug, do they can work with a 
0-size partition?

thanks
Huang Shijie

> I trust the user to provide meaningful partitions in the cmdline.
>
> Anyways, please consider this approach.
>
> My patch suggestion might look as (and might need some rework):
>
> diff --git a/drivers/mtd/cmdlinepart.c b/drivers/mtd/cmdlinepart.c
> index 17b0bd4..f5df613 100644
> --- a/drivers/mtd/cmdlinepart.c
> +++ b/drivers/mtd/cmdlinepart.c
> @@ -324,7 +324,6 @@ 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;
>                                  }
>                                  offset += part->parts[i].size;
>                          }
>
> Regards,
> Shmulik
>
> ______________________________________________________
> Linux MTD discussion mailing list
> http://lists.infradead.org/mailman/listinfo/linux-mtd/
>
Christopher Cordahi - Dec. 17, 2012, 1:11 a.m.
Huang Shijie <b32955 <at> freescale.com> writes:

> 于 2012年09月04日 19:48, Shmulik Ladkani 写道:
> >
> > My POV is that sorting and verification is not needed, is troublesome,
> > and might affect users in ways they don't expect.
> >
> > So far, mtdparts commandline parsing has been very lenient and liberal.
> > I think we should keep this approach; give the user the flexibility,
> > he'll be responsible to provide meaningful cmdline parts for his
> > system.
> >
> > Actually, Huang's initial complaint was that 'parse_cmdline_partitions'
> > was too strict - the truncated partition was not registered!
> >
> > Now, philosophy aside, let's talk about some usecases that might break.
> >
> > I remember overlapping partitions (more precisely, partition that is a
> > subset of another) being common in some embedded systems, where the
> > "rootfs" and "kernel" partitions are a subset of the "overall image"
> > partition.
> > Why break this? if users enjoy the flexibility, and careful not to
> > create silly partitions, I'm in favor of keeping the flexibility.
> >
> > Same goes for sorting.
> > If one has a system hacked to work with mtd0 hardcodedly, but mtd0's
> > physical location is somewhere at the end of the device, why reorder the
> > partitions enumeration affecting this system?
> >
> > I'd say keep it simple and flexible.
> yes, it's really simple and flexible.

Is it agreed that sorting is not necessary and will not be implemented?
If so are there objections to me modifying the documentation to indicate
that out of order partitions is part of the supported design?

In my embedded distribution, I would like to introduce a MTD partition
at the end of the list to not affect existing hardcoded MTD numbers.

The fw_printenv/fw_setenv command line interface to U-Boot environment
makes use of a /etc/fw_env.config with hardcoded device numbers which
would be broken by a sort.

Although my out of order partitions worked, I couldn't find any
documentation indicating it was a supported feature.  This thread with
Artem's support for sorting got me worried.

If the only reason for sorting is to prevent overlapping partitions,
I think this can be written just as efficiently without a sort although
Shmulik also gives a reason why this restriction should not exist.
I'd prefer to accept that task rather than have to handle changing
MTD numbers.

> But I think it's not wise to export the 0-size partition to user.
> The user may confused at this.

Although I just submitted a patch to improve this, personally I think
if a user specifies a zero sized partition, they should get it.  Or if
it's inherently incorrect I think it should be an error.  Similarly
I'd expect the same for truncated partitions, either provide the size
specified or error.  Since I don't ever foresee using either of these
two features I don't have any objections to current situation.  These
are just my $0.02

--
Chris
Brian Norris - Dec. 18, 2012, 5:27 a.m.
(re-include others on CC)

On Sun, Dec 16, 2012 at 5:11 PM, Christopher Cordahi
<christophercordahi@nanometrics.ca> wrote:
> Huang Shijie <b32955 <at> freescale.com> writes:
>> 于 2012年09月04日 19:48, Shmulik Ladkani 写道:
>> >
>> > My POV is that sorting and verification is not needed, is troublesome,
>> > and might affect users in ways they don't expect.
>> >
>> > So far, mtdparts commandline parsing has been very lenient and liberal.
>> > I think we should keep this approach; give the user the flexibility,
>> > he'll be responsible to provide meaningful cmdline parts for his
>> > system.
>> >
>> > Actually, Huang's initial complaint was that 'parse_cmdline_partitions'
>> > was too strict - the truncated partition was not registered!
>> >
>> > Now, philosophy aside, let's talk about some usecases that might break.
>> >
>> > I remember overlapping partitions (more precisely, partition that is a
>> > subset of another) being common in some embedded systems, where the
>> > "rootfs" and "kernel" partitions are a subset of the "overall image"
>> > partition.
>> > Why break this? if users enjoy the flexibility, and careful not to
>> > create silly partitions, I'm in favor of keeping the flexibility.
>> >
>> > Same goes for sorting.
>> > If one has a system hacked to work with mtd0 hardcodedly, but mtd0's
>> > physical location is somewhere at the end of the device, why reorder the
>> > partitions enumeration affecting this system?
>> >
>> > I'd say keep it simple and flexible.
>> yes, it's really simple and flexible.
>
> Is it agreed that sorting is not necessary and will not be implemented?
> If so are there objections to me modifying the documentation to indicate
> that out of order partitions is part of the supported design?

I don't object but rather would welcome the documentation. I think the
consensus was that there were real reasons to allow unsorted
partitions at least, and maybe even overlapping partitions.

> In my embedded distribution, I would like to introduce a MTD partition
> at the end of the list to not affect existing hardcoded MTD numbers.

FWIW, my embedded systems make use of both out-of-order and
overlapping partitions. I don't see why they must be sorted, and
currently, overlapping is useful for me in exactly the scenarios given
above (rootfs + kernel as subsets of entire_device).

> Although my out of order partitions worked, I couldn't find any
> documentation indicating it was a supported feature.  This thread with
> Artem's support for sorting got me worried.
>
> If the only reason for sorting is to prevent overlapping partitions,
> I think this can be written just as efficiently without a sort although
> Shmulik also gives a reason why this restriction should not exist.
> I'd prefer to accept that task rather than have to handle changing
> MTD numbers.
>
>> But I think it's not wise to export the 0-size partition to user.
>> The user may confused at this.
>
> Although I just submitted a patch to improve this, personally I think
> if a user specifies a zero sized partition, they should get it.  Or if
> it's inherently incorrect I think it should be an error.  Similarly
> I'd expect the same for truncated partitions, either provide the size
> specified or error.  Since I don't ever foresee using either of these
> two features I don't have any objections to current situation.  These
> are just my $0.02

Well, the truncated partition is helpful sometimes. Perhaps an old
flash partition layout is used when swapping in a different, smaller
NAND; the truncated partition allows the system to still work, with a
nice warning in case the user wants to fix it.

Brian
Artem Bityutskiy - Jan. 15, 2013, 11:49 a.m.
On Mon, 2012-12-17 at 01:11 +0000, Christopher Cordahi wrote:
> > > I'd say keep it simple and flexible.
> > yes, it's really simple and flexible.
> 
> Is it agreed that sorting is not necessary and will not be implemented?
> If so are there objections to me modifying the documentation to indicate
> that out of order partitions is part of the supported design?

I think we agreed not to do this.

Patch

diff --git a/drivers/mtd/cmdlinepart.c b/drivers/mtd/cmdlinepart.c
index 17b0bd4..f5df613 100644
--- a/drivers/mtd/cmdlinepart.c
+++ b/drivers/mtd/cmdlinepart.c
@@ -324,7 +324,6 @@  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;
                                }
                                offset += part->parts[i].size;
                        }