x86: kill arch/x86/kernel/mpparse.c debugging printk.

Hi Andrew.

<repeat below changelog>

Rene.

From: Rene Herman <[email protected]>
Date: Thu, 7 Aug 2008 01:50:35 +0200
Subject: [PATCH] x86: kill arch/x86/kernel/mpparse.c debugging printk.

commit 11a62a056093a7f25f1595fbd8bd5f93559572b6 turns some formerly nopped debugging printks in arch/x86/kernel/mppparse.c into regular ones. The one at the top of smp_scan_config() in particular also prints on !CONFIG_SMP/CONFIG_X86_LOCAL_APIC kernels and UP machines without anything resembling MP tables which makes their lowly UP owners wonder...

given that it was up to this point also not considered valuable user-level information, let's just kill that one.

---
 arch/x86/kernel/mpparse.c |    1 -
 1 files changed, 0 insertions(+), 1 deletions(-)

index 6ae005c..519a6a9 100644
@@ -695,7 +695,6 @@ static int __init smp_scan_config(unsigned long base, unsigned long length,
 	unsigned int *bp = phys_to_virt(base);
 	struct intel_mp_floating *mpf;
 
-	printk(KERN_DEBUG "Scan SMP from %p for %ld bytes.\n", bp, length);
 	BUILD_BUG_ON(sizeof(*mpf) != 16);
 
 	while (length > 0) {

1.5.5

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

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.

From: Rene Herman <[email protected]>
Date: Mon, 11 Aug 2008 17:20:42 +0200
Subject: [PATCH] x86: make arch/x86/kernel/mpparse.c debugging printk's apic_printk's

commit 11a62a056093a7f25f1595fbd8bd5f93559572b6 turns some formerly nopped debugging printks in arch/x86/kernel/mppparse.c into regular ones. The one at the top of smp_scan_config() in particular also prints on !CONFIG_SMP/CONFIG_X86_LOCAL_APIC kernels and UP machines without anything resembling MP tables which makes their lowly UP owners wonder...

Turn the former Dprintk()s into apic_printk()s instead meaning that their printing is dependent on passing the apic=verbose (or =debug) command line param.

On 32-bit, "apic" is a __setup() param which isn't early enough for this code and therefore needs a followup changing it into an early_param(). On 64-bit, it already is.

---
 arch/x86/kernel/mpparse.c |   11 ++++++-----
 1 files changed, 6 insertions(+), 5 deletions(-)

index 6ae005c..6780905 100644
@@ -83,7 +83,7 @@ static void __init MP_bus_info(struct mpc_config_bus *m)
 	if (x86_quirks->mpc_oem_bus_info)
 		x86_quirks->mpc_oem_bus_info(m, str);
 	else
-		printk(KERN_INFO "Bus #%d is %s\n", m->mpc_busid, str);
+		apic_printk(APIC_VERBOSE, "Bus #%d is %s\n", m->mpc_busid, str);
 
 #if MAX_MP_BUSSES < 256
 	if (m->mpc_busid >= MAX_MP_BUSSES) {

@@ -154,7 +154,7 @@ static void __init MP_ioapic_info(struct mpc_config_ioapic *m)
 
 static void print_MP_intsrc_info(struct mpc_config_intsrc *m)
 {
-	printk(KERN_CONT "Int: type %d, pol %d, trig %d, bus %02x,"
+	apic_printk(APIC_VERBOSE, "Int: type %d, pol %d, trig %d, bus %02x,"
 		" IRQ %02x, APIC ID %x, APIC INT %02x\n",
 		m->mpc_irqtype, m->mpc_irqflag & 3,
 		(m->mpc_irqflag >> 2) & 3, m->mpc_srcbus,

@@ -163,7 +163,7 @@ static void print_MP_intsrc_info(struct mpc_config_intsrc *m)
 
 static void __init print_mp_irq_info(struct mp_config_intsrc *mp_irq)
 {
-	printk(KERN_CONT "Int: type %d, pol %d, trig %d, bus %02x,"
+	apic_printk(APIC_VERBOSE, "Int: type %d, pol %d, trig %d, bus %02x,"
 		" IRQ %02x, APIC ID %x, APIC INT %02x\n",
 		mp_irq->mp_irqtype, mp_irq->mp_irqflag & 3,
 		(mp_irq->mp_irqflag >> 2) & 3, mp_irq->mp_srcbus,

@@ -235,7 +235,7 @@ static void __init MP_intsrc_info(struct mpc_config_intsrc *m)
 
 static void __init MP_lintsrc_info(struct mpc_config_lintsrc *m)
 {
-	printk(KERN_INFO "Lint: type %d, pol %d, trig %d, bus %02x,"
+	apic_printk(APIC_VERBOSE, "Lint: type %d, pol %d, trig %d, bus %02x,"
 		" IRQ %02x, APIC ID %x, APIC LINT %02x\n",
 		m->mpc_irqtype, m->mpc_irqflag & 3,
 		(m->mpc_irqflag >> 2) & 3, m->mpc_srcbusid,

@@ -695,7 +695,8 @@ static int __init smp_scan_config(unsigned long base, unsigned long length,
 	unsigned int *bp = phys_to_virt(base);
 	struct intel_mp_floating *mpf;
 
-	printk(KERN_DEBUG "Scan SMP from %p for %ld bytes.\n", bp, length);
+	apic_printk(APIC_VERBOSE, "Scan SMP from %p for %ld bytes.\n",
+			bp, length);
 	BUILD_BUG_ON(sizeof(*mpf) != 16);
 
 	while (length > 0) {

1.5.5

Ie:

Rene.

From: Rene Herman <[email protected]>
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().

---
 arch/x86/kernel/apic_32.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

index d6c8983..f432d48 100644
@@ -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

[Rene Herman - Mon, Aug 11, 2008 at 05:45:53PM +0200]

| From 9655f5ab2537ecd5c5d92b03840f3b6aeed441ad Mon Sep 17 00:00:00 2001
| From: Rene Herman <[email protected]>
| 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 <[email protected]>
| ---
|  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 -

[ ... ]

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

Rene.

From: Rene Herman <[email protected]>
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().

---
 arch/x86/kernel/apic_32.c |   14 +++++++++-----
 1 files changed, 9 insertions(+), 5 deletions(-)

index d6c8983..039a8d4 100644
@@ -1720,15 +1720,19 @@ 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 1;
+
+	return 0;
 }
-__setup("apic=", apic_set_verbosity);
+early_param("apic", apic_set_verbosity);
 
 static int __init lapic_insert_resource(void)
 {

1.5.5

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

Ingo

-------------->

From: Rene Herman <[email protected]>
Date: Mon, 11 Aug 2008 19:20:17 +0200
Subject: [PATCH] x86: make "apic" an early_param() on 32-bit, NULL check

Cyrill Gorcunov observed:

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.

---
 arch/x86/kernel/apic_32.c |   10 +++++++---
 1 files changed, 7 insertions(+), 3 deletions(-)

index f432d48..039a8d4 100644
@@ -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);

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.

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

[Ingo Molnar - Mon, Aug 11, 2008 at 08:33:00PM +0200]

| 
| * Rene Herman <[email protected]> wrote:
| 
| > On 11-08-08 19:41, Ingo Molnar wrote:
| >
| >> * Rene Herman <[email protected]> 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 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 -

[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 -

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

[Ingo Molnar - Mon, Aug 11, 2008 at 08:49:03PM +0200]

| 
| * Cyrill Gorcunov <[email protected]> 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 to numeric values only (which shouldn't lead to any new bugs I hope :)

- Cyrill -

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 again. Just tried it and it works nicely.

Yes, I'm endpoint.

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 early_param()?

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

Rene

applied to tip/x86/urgent - thanks Rene.

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

Ingo