Command line parsing

Command line parsing

[David A. Braun]

Is there any particular reason parse_cmdline_partitions() in 
drivers/mtd/cmdlineparse.c doesn't completely setup the partition 
table(s) it builds? It fails to initialize the offsets and sizes of any 
partition tabled unless it is the one named by "master". This one has 
the offsets and lengths filled in but the others are left zero with the 
exception of partitions specified with "-" or SIZE_REMAINING. I would 
have expected all partitions described in the cmdline to be initialized 
if any are.

Dave Braun

Re: Command line parsing

[Jörn Engel]

On Sat, 17 September 2005 10:55:40 -0400, David A. Braun wrote:
> 
> Is there any particular reason parse_cmdline_partitions() in 
> drivers/mtd/cmdlineparse.c doesn't completely setup the partition 
> table(s) it builds? It fails to initialize the offsets and sizes of any 
> partition tabled unless it is the one named by "master". This one has 
> the offsets and lengths filled in but the others are left zero with the 
> exception of partitions specified with "-" or SIZE_REMAINING. I would 
> have expected all partitions described in the cmdline to be initialized 
> if any are.

Iirc, the code was written by someone at Sysgo, possibly Robert, and
hasn't been touched for quite a while.  So if you have any patches
that improve the situation for you, feel free to send them.

Jörn

-- 
Rules of Optimization:
Rule 1: Don't do it.
Rule 2 (for experts only): Don't do it yet.
-- M.A. Jackson 

Re: Command line parsing

[Marius Groeger]

On Mon, 19 Sep 2005, Jörn Engel wrote:

> On Sat, 17 September 2005 10:55:40 -0400, David A. Braun wrote:
>>
>> Is there any particular reason parse_cmdline_partitions() in
>> drivers/mtd/cmdlineparse.c doesn't completely setup the partition
>> table(s) it builds? It fails to initialize the offsets and sizes of any
>> partition tabled unless it is the one named by "master". This one has
>> the offsets and lengths filled in but the others are left zero with the
>> exception of partitions specified with "-" or SIZE_REMAINING. I would
>> have expected all partitions described in the cmdline to be initialized
>> if any are.
>
> Iirc, the code was written by someone at Sysgo, possibly Robert, and
> hasn't been touched for quite a while.  So if you have any patches
> that improve the situation for you, feel free to send them.

Acutally it was me :-), but someone decided to remove my name and just 
leave in the reference to our company... Anyway you're right about my 
not having touched that code. Looking at it briefly, though, I don't 
really spot anything suspicous. Apart from the (potential) oddity that 
the cmdline is saved and processed much later at a time the flash 
hardware is actually, I don't see what is not initialized.

In other words, David, what is the actual problem you're seeing?

Regards,
Marius

-- 
Marius Groeger <mgroeger@sysgo.com>
SYSGO AG                      Embedded and Real-Time Software
Voice: +49 6136 9948 0                  FAX: +49 6136 9948 10
www.sysgo.com | www.elinos.com | www.osek.de | www.pikeos.com

Re: Command line parsing

[Jörn Engel]

On Mon, 19 September 2005 16:26:07 +0200, Marius Groeger wrote:
> On Mon, 19 Sep 2005, Jörn Engel wrote:
> >On Sat, 17 September 2005 10:55:40 -0400, David A. Braun wrote:
> >>
> >>Is there any particular reason parse_cmdline_partitions() in
> >>drivers/mtd/cmdlineparse.c doesn't completely setup the partition
> >>table(s) it builds? It fails to initialize the offsets and sizes of any
> >>partition tabled unless it is the one named by "master". This one has
> >>the offsets and lengths filled in but the others are left zero with the
> >>exception of partitions specified with "-" or SIZE_REMAINING. I would
> >>have expected all partitions described in the cmdline to be initialized
> >>if any are.
> >
> >Iirc, the code was written by someone at Sysgo, possibly Robert, and
> >hasn't been touched for quite a while.  So if you have any patches
> >that improve the situation for you, feel free to send them.
> 
> Acutally it was me :-), but someone decided to remove my name and just 
> leave in the reference to our company... Anyway you're right about my 
> not having touched that code. Looking at it briefly, though, I don't 
> really spot anything suspicous. Apart from the (potential) oddity that 
> the cmdline is saved and processed much later at a time the flash 
> hardware is actually, I don't see what is not initialized.

[ slightly off-topic ]

If you want your copyright line restored and don't have cvs access,
send me a patch.

For those less aware of how copyright law works:
The copyright itself is independent of some lines explicitly claiming
it for somebody.  So this is not about a legal issue, just politeness.

Jörn

-- 
I can say that I spend most of my time fixing bugs even if I have lots
of new features to implement in mind, but I give bugs more priority.
-- Andrea Arcangeli, 2000

Re: Command line parsing

[Marius Groeger]

On Mon, 19 Sep 2005, Marius Groeger wrote:

> in the reference to our company... Anyway you're right about my not having 
> touched that code.

Add "... in a long time" to make this more reasonable to parse :-)

And sorry for the noise...

Marius

-- 
Marius Groeger <mgroeger@sysgo.com>
SYSGO AG                      Embedded and Real-Time Software
Voice: +49 6136 9948 0                  FAX: +49 6136 9948 10
www.sysgo.com | www.elinos.com | www.osek.de | www.pikeos.com

Re: Command line parsing

[David A. Braun]

If the name in master->name doesn't match any of the partitions 
mentioned on the kernel command line then *pparts->offset is not 
initialized. The result is that all the partitions start at 0. Also the 
length of the "SIZE_REMAINING" partition is never initialized. Also the 
return value is -EINVAL instead of the number of partitions. This 
results in none of the partitions being pass to the driver. The only 
partition that appears in /proc/partitions is one for the whole device.

There are two problems. The if statement

    if ((!mtd_id) || (!strcmp(part->mtd_id, mtd_id)))
    {
          ...

before the innermost for loop prevents the non-matching cmdline 
partitions from being initialized. Secondly the return statement just 
after this same for loop stops processing any remaining partitions 
following the matching partition.

As it turns out after looking over the code some more the table in 
question is internal to the cmdline parser anyway so no harm is done. 
Any partition declared on the cmdline that is referenced by a mtd driver 
gets initialized correctly. Unreferenced ones don't and need not be. 
There is however a small amount of kernel memory wasted on the partition 
table in cmdlineparts. It doesn't look like it ever gets released, not 
even if the cmdline parser gets de-registered (which it doesn't).

dave

Jörn Engel wrote:

>On Mon, 19 September 2005 16:26:07 +0200, Marius Groeger wrote:
>  
>
>>On Mon, 19 Sep 2005, Jörn Engel wrote:
>>    
>>
>>>On Sat, 17 September 2005 10:55:40 -0400, David A. Braun wrote:
>>>      
>>>
>>>>Is there any particular reason parse_cmdline_partitions() in
>>>>drivers/mtd/cmdlineparse.c doesn't completely setup the partition
>>>>table(s) it builds? It fails to initialize the offsets and sizes of any
>>>>partition tabled unless it is the one named by "master". This one has
>>>>the offsets and lengths filled in but the others are left zero with the
>>>>exception of partitions specified with "-" or SIZE_REMAINING. I would
>>>>have expected all partitions described in the cmdline to be initialized
>>>>if any are.
>>>>        
>>>>
>>>Iirc, the code was written by someone at Sysgo, possibly Robert, and
>>>hasn't been touched for quite a while.  So if you have any patches
>>>that improve the situation for you, feel free to send them.
>>>      
>>>
>>Acutally it was me :-), but someone decided to remove my name and just 
>>leave in the reference to our company... Anyway you're right about my 
>>not having touched that code. Looking at it briefly, though, I don't 
>>really spot anything suspicous. Apart from the (potential) oddity that 
>>the cmdline is saved and processed much later at a time the flash 
>>hardware is actually, I don't see what is not initialized.
>>    
>>
>
>[ slightly off-topic ]
>
>If you want your copyright line restored and don't have cvs access,
>send me a patch.
>
>For those less aware of how copyright law works:
>The copyright itself is independent of some lines explicitly claiming
>it for somebody.  So this is not about a legal issue, just politeness.
>
>Jörn
>
>  
>

Re: Command line parsing

[Marius Groeger]

On Mon, 19 Sep 2005, David A. Braun wrote:

> If the name in master->name doesn't match any of the partitions 
> mentioned on the kernel command line then *pparts->offset is not 
> initialized. The result is that all the partitions start at 0. Also 
> the length of the "SIZE_REMAINING" partition is never initialized. 
> Also the return value is -EINVAL instead of the number of 
> partitions. This results in none of the partitions being pass to the 
> driver. The only partition that appears in /proc/partitions is one 
> for the whole device.

Thanks for digging so deeply in to the code. I suggest it would be 
best if you prepare a patch and send it to the list.

Regards,
Marius

-- 
Marius Groeger <mgroeger@sysgo.com>
SYSGO AG                      Embedded and Real-Time Software
Voice: +49 6136 9948 0                  FAX: +49 6136 9948 10
www.sysgo.com | www.elinos.com | www.osek.de | www.pikeos.com

Re: Command line parsing

[Timofei V. Bondarenko]

David A. Braun wrote:
> If the name in master->name doesn't match any of the partitions 
> mentioned on the kernel command line then *pparts->offset is not 
> initialized. The result is that all the partitions start at 0. Also the 
> length of the "SIZE_REMAINING" partition is never initialized. Also the 
> return value is -EINVAL instead of the number of partitions. This 
> results in none of the partitions being pass to the driver. The only 
> partition that appears in /proc/partitions is one for the whole device.
> 

Is not it intentional?

There is a comment:
<Q> mtd-id := unique name used in mapping driver/device (mtd->name) </Q>

These names are identifing the separate devices rather than partitions.
And IMO it's only way to deal multiple flash devices with different partitioning.

--
Regards.
	Tim.

Re: Command line parsing

[David A. Braun]

as Clinton said - it depends on what you mean by "it"

If by "it" you mean the return value of EINVAL when the named partition 
by master->name is not found then you are correct. My mistake.

dave

Timofei V. Bondarenko wrote:

> David A. Braun wrote:
>
>> If the name in master->name doesn't match any of the partitions 
>> mentioned on the kernel command line then *pparts->offset is not 
>> initialized. The result is that all the partitions start at 0. Also 
>> the length of the "SIZE_REMAINING" partition is never initialized. 
>> Also the return value is -EINVAL instead of the number of partitions. 
>> This results in none of the partitions being pass to the driver. The 
>> only partition that appears in /proc/partitions is one for the whole 
>> device.
>>
>
> Is not it intentional?
>
> There is a comment:
> <Q> mtd-id := unique name used in mapping driver/device (mtd->name) </Q>
>
> These names are identifing the separate devices rather than partitions.
> And IMO it's only way to deal multiple flash devices with different 
> partitioning.
>
> -- 
> Regards.
>     Tim.
>