[PATCH] x86: kill arch/x86/kernel/mpparse.c debugging printk.

[PATCH] x86: kill arch/x86/kernel/mpparse.c debugging printk.

[Rene Herman]

[-- Attachment #1: Type: text/plain, Size: 44 bytes --]

Hi Andrew.

<repeat below changelog>

Rene.

[-- Attachment #2: 0001-x86-kill-arch-x86-kernel-mpparse.c-debugging-printk.patch --]
[-- Type: text/plain, Size: 1310 bytes --]

>From 00acea21e579ea0853baba25420f373ea83533a3 Mon Sep 17 00:00:00 2001
From: Rene Herman <rene.herman@keyaccess.nl>
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.

Signed-off-by: Rene Herman <rene.herman@gmail.com>
---
 arch/x86/kernel/mpparse.c |    1 -
 1 files changed, 0 insertions(+), 1 deletions(-)

diff --git a/arch/x86/kernel/mpparse.c b/arch/x86/kernel/mpparse.c
index 6ae005c..519a6a9 100644
--- a/arch/x86/kernel/mpparse.c
+++ b/arch/x86/kernel/mpparse.c
@@ -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

Re: [PATCH] x86: kill arch/x86/kernel/mpparse.c debugging printk.

[Ingo Molnar]

* Rene Herman <rene.herman@keyaccess.nl> wrote:

> Hi Andrew.
>
> <repeat below changelog>
>
> Rene.

> >From 00acea21e579ea0853baba25420f373ea83533a3 Mon Sep 17 00:00:00 2001
> From: Rene Herman <rene.herman@keyaccess.nl>
> 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.

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

Re: [PATCH] x86: kill arch/x86/kernel/mpparse.c debugging printk.

[Rene Herman]

[-- Attachment #1: Type: text/plain, Size: 1707 bytes --]

On 11-08-08 14:20, Ingo Molnar wrote:

>> From: Rene Herman <rene.herman@keyaccess.nl>
>> 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.
> 
> 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?

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.

[-- Attachment #2: 0001-x86-make-arch-x86-kernel-mpparse.c-debugging-printk.patch --]
[-- Type: text/plain, Size: 3250 bytes --]

>From 3d6ab02d08c3597cd24581968dd0b41f3c264716 Mon Sep 17 00:00:00 2001
From: Rene Herman <rene.herman@gmail.com>
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.

Signed-off-by: Rene Herman <rene.herman@gmail.com>
---
 arch/x86/kernel/mpparse.c |   11 ++++++-----
 1 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/arch/x86/kernel/mpparse.c b/arch/x86/kernel/mpparse.c
index 6ae005c..6780905 100644
--- a/arch/x86/kernel/mpparse.c
+++ b/arch/x86/kernel/mpparse.c
@@ -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

Re: [PATCH] x86: kill arch/x86/kernel/mpparse.c debugging printk.

[Rene Herman]

[-- Attachment #1: Type: text/plain, Size: 214 bytes --]

On 11-08-08 17:44, Rene Herman wrote:

> 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).

Ie:

Rene.

[-- Attachment #2: 0002-x86-make-apic-an-early_param-on-32-bit.patch --]
[-- Type: text/plain, Size: 1074 bytes --]

>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

Re: [PATCH] x86: kill arch/x86/kernel/mpparse.c debugging printk.

[Ingo Molnar]

* Rene Herman <rene.herman@keyaccess.nl> wrote:

>> 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?
>
> I wouldn't. Like this? This turns the printk's that used to be 
> Dprintk's into apic_printk's.

applied to tip/x86/urgent - thanks Rene.

> 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).

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

	Ingo

Re: [PATCH] x86: kill arch/x86/kernel/mpparse.c debugging printk.

[Cyrill Gorcunov]

[Rene Herman - Mon, Aug 11, 2008 at 05:45:53PM +0200]
> On 11-08-08 17:44, Rene Herman wrote:
>
>> 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).
>
> Ie:
>
> Rene.

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

Re: [PATCH] x86: kill arch/x86/kernel/mpparse.c debugging printk.

[Rene Herman]

[-- Attachment #1: Type: text/plain, Size: 451 bytes --]

On 11-08-08 18:45, Cyrill Gorcunov wrote:

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

[ ... ]

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

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

Rene.

[-- Attachment #2: 0001-x86-make-apic-an-early_param-on-32-bit.patch --]
[-- Type: text/plain, Size: 1398 bytes --]

>From 98cf69c9acbc2c1a29fdaa6fc8c29f1bacae8316 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 |   14 +++++++++-----
 1 files changed, 9 insertions(+), 5 deletions(-)

diff --git a/arch/x86/kernel/apic_32.c b/arch/x86/kernel/apic_32.c
index d6c8983..039a8d4 100644
--- a/arch/x86/kernel/apic_32.c
+++ b/arch/x86/kernel/apic_32.c
@@ -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

Re: [PATCH] x86: kill arch/x86/kernel/mpparse.c debugging printk.

[Ingo Molnar]

* Rene Herman <rene.herman@keyaccess.nl> wrote:

> On 11-08-08 18:45, Cyrill Gorcunov wrote:
>
>> | 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
>
> [ ... ]
>
>> you turned it into early_param so now it's NULL injecting vulnerabled.
>> Could you please add checking for NULL str param?
>
> 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.

	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

Cyrill Gorcunov observed:

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

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);

Re: [PATCH] x86: kill arch/x86/kernel/mpparse.c debugging printk.

[Rene Herman]

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... :-)

Rene.

Re: [PATCH] x86: kill arch/x86/kernel/mpparse.c debugging printk.

[Ingo Molnar]

* 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.

- 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

Re: [PATCH] x86: kill arch/x86/kernel/mpparse.c debugging printk.

[Cyrill Gorcunov]

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

Re: [PATCH] x86: kill arch/x86/kernel/mpparse.c debugging printk.

[Cyrill Gorcunov]

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

Re: [PATCH] x86: kill arch/x86/kernel/mpparse.c debugging printk.

[Ingo Molnar]

* 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

Re: [PATCH] x86: kill arch/x86/kernel/mpparse.c debugging printk.

[Rene Herman]

On 11-08-08 20:33, Ingo Molnar wrote:

> * Rene Herman <rene.herman@keyaccess.nl> wrote:

>> 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 ;-)

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.

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

Yes, I'm endpoint.

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

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

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

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

Rene

Re: [PATCH] x86: kill arch/x86/kernel/mpparse.c debugging printk.

[Cyrill Gorcunov]

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

		- Cyrill -