[PATCH][RFC] Drop the "static __initdata" from tmp_cmdline in

[PATCH][RFC] Drop the "static __initdata" from tmp_cmdline in

[Robert P. J. Day]

There appears to be no reason for the tmp_cmdline array to be
qualified as "static __initdata" since its value is apparently gone
once this routine ends.

Compile- and boot-time tested on i386.

Signed-off-by: Robert P. J. Day <rpjday@mindspring.com>

---

  (i'm willing to be corrected on this one.)

  there's a bit of a story behind this.  as i was following the logic
for defining and processing kernel boot-time parameters, i was baffled
as to why that array was declared with "static __initdata", as if it
needed to retain its contents after the function returned.

  since parse_early_param() defines a static "done" variable that
guarantees that it can only ever be run once, there seemed to be no
point in retaining that copy of the command line once the command-line
parameters were processed.

  if you follow the logic, you eventually work your way down to the
routine do_early_param() which, given a parameter name and its value
(both strings), simply invokes:

...
 if (p->setup_func(val) != 0)
     printk(KERN_WARNING "Malformed early option '%s'\n", param);
...

which inspires the obvious question -- when a kernel parameter setup
routine is called, is it allowed to assume that the data at the value
address it's passed will remain *even after it returns*?  if not, then
it would seem that there is no reason for that "static __initdata".

  on the other hand, if any one of those setup routines *is* making
that assumption, then dropping that qualifier will definitely cause
bad things to happen somewhere down the road at some point in the boot
process.

  it seems like a trivial patch until you appreciate the possible
implications for all of those kernel parm setup routines defined in
all those calls to __setup() and early_param().  in essence, by
applying that patch, you'll find out who's cheating.

  thoughts?

p.s.  also, if it's not required, declaring it as "static __initdata"
is seriously confusing, since it gives the obvious impression that the
contents *need* to be retained, which just makes following the logic
that much harder if you're working under that misimpression.


diff --git a/init/main.c b/init/main.c
index 9def935..86d2ffc 100644
--- a/init/main.c
+++ b/init/main.c
@@ -482,7 +482,7 @@ static int __init do_early_param(char *param, char *val)
 void __init parse_early_param(void)
 {
 	static __initdata int done = 0;
-	static __initdata char tmp_cmdline[COMMAND_LINE_SIZE];
+	char tmp_cmdline[COMMAND_LINE_SIZE];

 	if (done)
 		return;


-- 
====================================
Robert P. J. Day
Linux Consulting, Training and Annoying Kernel Pedantry
Waterloo, Ontario, CANADA

http://crashcourse.ca
====================================

Re: [PATCH][RFC] Drop the "static __initdata" from tmp_cmdline in

[Thomas Petazzoni]

Hi,

Robert P. J. Day wrote:

> --- a/init/main.c
> +++ b/init/main.c
> @@ -482,7 +482,7 @@ static int __init do_early_param(char *param, char *val)
>  void __init parse_early_param(void)
>  {
>  	static __initdata int done = 0;
> -	static __initdata char tmp_cmdline[COMMAND_LINE_SIZE];
> +	char tmp_cmdline[COMMAND_LINE_SIZE];

As I've said in an answer to an e-mail from you on the kernelnewbies 
mailing list, this change is going to allocate 2k on the stack, which is 
quite big compared to the overall stack size. If you use 8k stacks, it 
may still work, but with 4k stacks, it eats half of the stack, leaving 
only 2k for the rest of the code to execute, which may not be enough. I 
think that why this buffer is static.

Sincerly,

Thomas
-- 
PETAZZONI Thomas - thomas.petazzoni@enix.org
http://thomas.enix.org - Jabber: thomas.petazzoni@jabber.dk
KOS: http://kos.enix.org/ - SOS: http://sos.enix.org
Fingerprint : 0BE1 4CF3 CEA4 AC9D CC6E  1624 F653 CB30 98D3 F7A7

Re: [PATCH][RFC] Drop the "static __initdata" from tmp_cmdline in

[Robert P. J. Day]

On Thu, 13 Sep 2007, Thomas Petazzoni wrote:

> Hi,
>
> Robert P. J. Day wrote:
>
> > --- a/init/main.c
> > +++ b/init/main.c
> > @@ -482,7 +482,7 @@ static int __init do_early_param(char *param, char *val)
> >  void __init parse_early_param(void)
> >  {
> >  	static __initdata int done = 0;
> > -	static __initdata char tmp_cmdline[COMMAND_LINE_SIZE];
> > +	char tmp_cmdline[COMMAND_LINE_SIZE];
>
> As I've said in an answer to an e-mail from you on the kernelnewbies
> mailing list, this change is going to allocate 2k on the stack,
> which is quite big compared to the overall stack size. If you use 8k
> stacks, it may still work, but with 4k stacks, it eats half of the
> stack, leaving only 2k for the rest of the code to execute, which
> may not be enough. I think that why this buffer is static.

ah, quite right -- that hadn't occurred to me.  so it's entirely
possible that there is no *programmatic* reason why that array needs
to persist after the function call other than for reasons of stack
size.  that would certainly explain a lot of other "static __initdata"
declarations i've seen that didn't really seem necessary.

thanks.

rday
-- 
====================================
Robert P. J. Day
Linux Consulting, Training and Annoying Kernel Pedantry
Waterloo, Ontario, CANADA

http://crashcourse.ca
====================================

Re: [PATCH][RFC] Drop the "static __initdata" from tmp_cmdline in

[Robert P. J. Day]

On Thu, 13 Sep 2007, Thomas Petazzoni wrote:

> Hi,
>
> Robert P. J. Day wrote:
>
> > --- a/init/main.c
> > +++ b/init/main.c
> > @@ -482,7 +482,7 @@ static int __init do_early_param(char *param, char *val)
> >  void __init parse_early_param(void)
> >  {
> >  	static __initdata int done = 0;
> > -	static __initdata char tmp_cmdline[COMMAND_LINE_SIZE];
> > +	char tmp_cmdline[COMMAND_LINE_SIZE];
>
> As I've said in an answer to an e-mail from you on the kernelnewbies
> mailing list, this change is going to allocate 2k on the stack,
> which is quite big compared to the overall stack size. If you use 8k
> stacks, it may still work, but with 4k stacks, it eats half of the
> stack, leaving only 2k for the rest of the code to execute, which
> may not be enough. I think that why this buffer is static.

  as a (sort of) related followup to the above, note that having
dropped the "static __initdata" would have had a nasty effect on any
__setup or early_param-registered setup functions that assumed that
the data at the address they were passed was going to persist, or
assumed that they could write back to that address.

  but technically, that's not disallowed.  look at the layout of the
relevant structure:

struct obs_kernel_param {
        const char *str;
        int (*setup_func)(char *);
        int early;
};

  note that the setup_func accepts "char *" as opposed to "const char
*".  any legitimate reason for that?  are there, in fact, any of those
setup functions anywhere in the tree that try to modify the data back
at that passed address?  i would have assumed not, but i haven't
looked closely enough.

rday
-- 
====================================
Robert P. J. Day
Linux Consulting, Training and Annoying Kernel Pedantry
Waterloo, Ontario, CANADA

http://crashcourse.ca
====================================

Re: [PATCH][RFC] Drop the "static __initdata" from tmp_cmdline in

[walter harms]

would a kalloc/free be suffizient ? some could do it like strdup().

re,
 wh


Thomas Petazzoni wrote:
> Hi,
> 
> Robert P. J. Day wrote:
> 
>> --- a/init/main.c
>> +++ b/init/main.c
>> @@ -482,7 +482,7 @@ static int __init do_early_param(char *param, char
>> *val)
>>  void __init parse_early_param(void)
>>  {
>>      static __initdata int done = 0;
>> -    static __initdata char tmp_cmdline[COMMAND_LINE_SIZE];
>> +    char tmp_cmdline[COMMAND_LINE_SIZE];
> 
> As I've said in an answer to an e-mail from you on the kernelnewbies
> mailing list, this change is going to allocate 2k on the stack, which is
> quite big compared to the overall stack size. If you use 8k stacks, it
> may still work, but with 4k stacks, it eats half of the stack, leaving
> only 2k for the rest of the code to execute, which may not be enough. I
> think that why this buffer is static.
> 
> Sincerly,
> 
> Thomas