Hi Andrew.

<repeat below changelog>

Rene.
[ message continues ]

hm, i found it useful in the past in about 2-3 cases.

How about a patch that makes the printout depend on apic=debug ? That 
way the message can still be there in case of bugreports that somehow 
deal with SMP or APIC bugs (without having to recompile the kernel).

The way to make the printout depend on apic=debug/verbose is to do 
something like this:

	apic_printk(APIC_VERBOSE, "Scan SMP from %p for %ld bytes.\n", bp, length);

Would you mind to send a patch for that?

Thanks,

	Ingo
--

[ message continues ]

I wouldn't. Like this? This turns the printk's that used to be Dprintk's 
into apic_printk's.

I am myself only interested in the one at the top of smp_scan_config() 
(it made me think I had misconfigured something upon all of a sudden 
seeing SMP printk's on my UP machine on 2.6.27-rc) but I guess this is 
the more complete version.

One problem; on 32-bit, "apic=" is a __setup() param and isn't actually 
early enough for us here so this needs it turned into an early_param() 
(followup).

Rene.
[ message continues ]

Ie:

Rene.
[ message continues ]

| From 9655f5ab2537ecd5c5d92b03840f3b6aeed441ad Mon Sep 17 00:00:00 2001
| From: Rene Herman <rene.herman@gmail.com>
| Date: Mon, 11 Aug 2008 17:35:41 +0200
| Subject: [PATCH] x86: make "apic" an early_param() on 32-bit
| 
| On 32-bit, "apic" is a __setup() param meaning it is parsed rather
| late in the game. Make it an early_param() for apic_printk() use
| by arch/x86/kernel/mpparse.c.
| 
| On 64-bit, it already is an early_param().
| 
| Signed-off-by: Rene Herman <rene.herman@gmail.com>
| ---
|  arch/x86/kernel/apic_32.c |    4 ++--
|  1 files changed, 2 insertions(+), 2 deletions(-)
| 
| diff --git a/arch/x86/kernel/apic_32.c b/arch/x86/kernel/apic_32.c
| index d6c8983..f432d48 100644
| --- a/arch/x86/kernel/apic_32.c
| +++ b/arch/x86/kernel/apic_32.c
| @@ -1726,9 +1726,9 @@ static int __init apic_set_verbosity(char *str)
|  		apic_verbosity = APIC_DEBUG;
|  	else if (strcmp("verbose", str) == 0)
|  		apic_verbosity = APIC_VERBOSE;
| -	return 1;
| +	return 0;
|  }
| -__setup("apic=", apic_set_verbosity);
| +early_param("apic", apic_set_verbosity);
|  
|  static int __init lapic_insert_resource(void)
|  {
| -- 
| 1.5.5
| 

Hi Rene,

you turned it into early_param so now it's NULL injecting vulnerabled.
Could you please add checking for NULL str param?

		- Cyrill -
--

[ message continues ]

Ah, I was unaware of that difference, thank you. Ingo, can you replace 
the previous incarnation with this one?

Rene.
[ message continues ]

sure - but some other commits were queued already so i've applied the 
delta fix below.

	Ingo

-------------->
From 48d97cb65e62a5f1122ac2cf1149800d4f4693e8 Mon Sep 17 00:00:00 2001
From: Rene Herman <rene.herman@keyaccess.nl>
Date: Mon, 11 Aug 2008 19:20:17 +0200
Subject: [PATCH] x86: make "apic" an early_param() on 32-bit, NULL check


fix that.

Also, change the name of 'str' into 'arg', to make it more apparent
that this is an optional argument that can be NULL, not a string
parameter that is empty when unset.

Reported-by: Cyrill Gorcunov <gorcunov@gmail.com>
Signed-off-by: Rene Herman <rene.herman@gmail.com>
Signed-off-by: Ingo Molnar <mingo@elte.hu>
---
 arch/x86/kernel/apic_32.c |   10 +++++++---
 1 files changed, 7 insertions(+), 3 deletions(-)

diff --git a/arch/x86/kernel/apic_32.c b/arch/x86/kernel/apic_32.c
index f432d48..039a8d4 100644
--- a/arch/x86/kernel/apic_32.c
+++ b/arch/x86/kernel/apic_32.c
@@ -1720,12 +1720,16 @@ static int __init parse_lapic_timer_c2_ok(char *arg)
 }
 early_param("lapic_timer_c2_ok", parse_lapic_timer_c2_ok);
 
-static int __init apic_set_verbosity(char *str)
+static int __init apic_set_verbosity(char *arg)
 {
-	if (strcmp("debug", str) == 0)
+	if (!arg)
+		return -EINVAL;
+
+	if (strcmp(arg, "debug") == 0)
 		apic_verbosity = APIC_DEBUG;
-	else if (strcmp("verbose", str) == 0)
+	else if (strcmp(arg, "verbose") == 0)
 		apic_verbosity = APIC_VERBOSE;
+
 	return 0;
 }
 early_param("apic", apic_set_verbosity);
--

[ message continues ]

Thanks and fine ofcourse but from the Cheats 'R Us GIT handbook, when 
there's n patches on top of the one I want to edit:

$ mkdir tmp
$ git format-patch -o tmp HEAD~n
$ git reset --hard HEAD~n
$ git reset --soft HEAD^
<fix>
$ git commit -a -c ORIG_HEAD
$ git am tmp/*
$ rm -rf tmp

Just in case someone finds it interesting... :-)

Rene.
--

[ message continues ]

i think something like this would do it as well:

    git-rebase -i HEAD~$[n+1]

Change the patch you want to edit from 'pick' to 'edit', and do a "git 
commit --amend" to fix it up and then a "git rebase continue" to reapply 
the other n patches ontop of the changed patch. (This is straight from 
the Cheats 'R Us GIT handbook, second edition ;-)

The problem with rebasing though is that it does not interact with 
normal Git workflows very nicely. Someone might have based further work 
on those sha1's that we now change under them. When that further work is 
backmerged later on we have overlapping sha1's.

There are two further specific non-Git-workflow arguments in favor of 
the delta patch as well:

- in this case your first change was the obvious one and your NULL fix 
  and your cleanup to the parameter expose a fundamental weakness of
  early_param conversions - and i think highlighting that as separate 
  commits might give someone ideas to improve the early_param() 
  facility, if they see the fix patterns.

- Also, the NULL condition is obscure, so there's no bisection breakage
  risk and it's the easiest for me to do append-only patches. The effort
  and thought process you and Cyrill have put into it deserve a separate
  commit as well anyway - and others might learn from it when looking at
  logs.

	Ingo
--

[ message continues ]

[Ingo Molnar - Mon, Aug 11, 2008 at 08:33:00PM +0200]
| 
| * Rene Herman <rene.herman@keyaccess.nl> wrote:
| 
| > On 11-08-08 19:41, Ingo Molnar wrote:
| >
| >> * Rene Herman <rene.herman@keyaccess.nl> wrote:
| >
| >>> Ah, I was unaware of that difference, thank you. Ingo, can you 
| >>> replace  the previous incarnation with this one?
| >>
| >> sure - but some other commits were queued already so i've applied the  
| >> delta fix below.
| >
| > Thanks and fine ofcourse but from the Cheats 'R Us GIT handbook, when  
| > there's n patches on top of the one I want to edit:
| >
| > $ mkdir tmp
| > $ git format-patch -o tmp HEAD~n
| > $ git reset --hard HEAD~n
| > $ git reset --soft HEAD^
| > <fix>
| > $ git commit -a -c ORIG_HEAD
| > $ git am tmp/*
| > $ rm -rf tmp
| >
| > Just in case someone finds it interesting... :-)
| 
| i think something like this would do it as well:
| 
|     git-rebase -i HEAD~$[n+1]
| 
| Change the patch you want to edit from 'pick' to 'edit', and do a "git 
| commit --amend" to fix it up and then a "git rebase continue" to reapply 
| the other n patches ontop of the changed patch. (This is straight from 
| the Cheats 'R Us GIT handbook, second edition ;-)
| 
| The problem with rebasing though is that it does not interact with 
| normal Git workflows very nicely. Someone might have based further work 
| on those sha1's that we now change under them. When that further work is 
| backmerged later on we have overlapping sha1's.
| 
| There are two further specific non-Git-workflow arguments in favor of 
| the delta patch as well:
| 
| - in this case your first change was the obvious one and your NULL fix 
|   and your cleanup to the parameter expose a fundamental weakness of
|   early_param conversions - and i think highlighting that as separate 
|   commits might give someone ideas to improve the early_param() 
|   facility, if they see the fix patterns.

Ingo - I think the problem with early_param is not NULL itself but
rather - what is ...
[ message continues ]

[Cyrill Gorcunov - Mon, Aug 11, 2008 at 10:42:57PM +0400]
...
| | 
| | - in this case your first change was the obvious one and your NULL fix 
| |   and your cleanup to the parameter expose a fundamental weakness of
| |   early_param conversions - and i think highlighting that as separate 
| |   commits might give someone ideas to improve the early_param() 
| |   facility, if they see the fix patterns.
| 
| Ingo - I think the problem with early_param is not NULL itself but
| rather - what is the right way to deal with boot params? I mean we
| could pass empty string (not NULL) in case of argument absence _but_ would it
| be the right way? If you remember when I sent first series for early_param
| checking (and actually there are number of same issue exists for example
| in s390 arch) - I was asking community what is the best way - since I'm not
| that strong in interface engineering - i prefer fix the bugs :)
| 
| | 
| | - Also, the NULL condition is obscure, so there's no bisection breakage
| |   risk and it's the easiest for me to do append-only patches. The effort
| |   and thought process you and Cyrill have put into it deserve a separate
| |   commit as well anyway - and others might learn from it when looking at
| |   logs.
| | 
| | 	Ingo
| | 
| 		- Cyrill -

here is the link to that message

	http://lkml.org/lkml/2008/7/9/222

		- Cyrill -
--

[ message continues ]

what would be the downside of passing in empty strings? I cannot see any 
serious one. The upside is that the conversion is more mechanic and 
safer as well.

Maybe the return code inversion could be / should be fixed as well, that 
seems like an unnecessary change as well:

  -       return 1;
  +       return 0;
   }
  -__setup("apic=", apic_set_verbosity);
  +early_param("apic", apic_set_verbosity);

Why do early-params have a different return convention from 
usual-params? It's just an unnecessary barrier against conversion to 
early params.

	Ingo
--

[ message continues ]

[Ingo Molnar - Mon, Aug 11, 2008 at 08:49:03PM +0200]
| 
| * Cyrill Gorcunov <gorcunov@gmail.com> wrote:
| 
| > |   early_param conversions - and i think highlighting that as 
| > |   separate commits might give someone ideas to improve the 
| > |   early_param() facility, if they see the fix patterns.
| > 
| > Ingo - I think the problem with early_param is not NULL itself but 
| > rather - what is the right way to deal with boot params? I mean we 
| > could pass empty string (not NULL) in case of argument absence _but_ 
| > would it be the right way? If you remember when I sent first series 
| > for early_param checking (and actually there are number of same issue 
| > exists for example in s390 arch) - I was asking community what is the 
| > best way - since I'm not that strong in interface engineering - i 
| > prefer fix the bugs :)
| 
| what would be the downside of passing in empty strings? I cannot see any 
| serious one. The upside is that the conversion is more mechanic and 
| safer as well.
| 
| Maybe the return code inversion could be / should be fixed as well, that 
| seems like an unnecessary change as well:
| 
|   -       return 1;
|   +       return 0;
|    }
|   -__setup("apic=", apic_set_verbosity);
|   +early_param("apic", apic_set_verbosity);
| 
| Why do early-params have a different return convention from 
| usual-params? It's just an unnecessary barrier against conversion to 
| early params.
| 
| 	Ingo
| 

Actually - I don't know why is checking for 0. It's defined in
init/main.c:do_early_param -

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

if we change it there - the lot of kernel code should be patched then.
I don't think it will be approved (even by being mechanical change) :) 

To pass empty string - I think it's possible. I think I'll check it
tomorrow evening (have no time now). Or maybe someone else could
grep kernel tree to check if there only strcmp and conversion ...
[ message continues ]

Okay, okay, okay, so nobody found it interesting. Got the same bit of 
advice in private approximately 2 seconds after sending... ;-)

Thanks to both though. And now that you mention it, I remember actually 
having gotten the rebase -i advice earlier but it had slipped my mind 


On that note, I sort of wonder why there is an early_param(). As in, not 
just a kernel_param(). Does __setup() have fundamental advantages over 

(true, I neglected to point out Cyrill's bug catching)

Rene
--

[ message continues ]

good spotting, applied that one to tip/x86/urgent as well - thanks.

	Ingo
--

[ message continues ]