>From 00acea21e579ea0853baba25420f373ea83533a3 Mon Sep 17 00:00:00 2001
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.
Signed-off-by: Rene Herman <[email protected]>
---
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
* Rene Herman <[email protected]> wrote:
> Hi Andrew.
>
> <repeat below changelog>
>
> Rene.
> >From 00acea21e579ea0853baba25420f373ea83533a3 Mon Sep 17 00:00:00 2001
> 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.
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
>From 3d6ab02d08c3597cd24581968dd0b41f3c264716 Mon Sep 17 00:00:00 2001
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.
Signed-off-by: Rene Herman <[email protected]>
---
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
>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
* Rene Herman <[email protected]> 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
[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 <[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 -
>From 98cf69c9acbc2c1a29fdaa6fc8c29f1bacae8316 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 | 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
* Rene Herman <[email protected]> wrote:
> On 11-08-08 18:45, Cyrill Gorcunov wrote:
>
>> | 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
>
> [ ... ]
>
>> 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 <[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:
> 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 <[email protected]>
Signed-off-by: Rene Herman <[email protected]>
Signed-off-by: Ingo Molnar <[email protected]>
---
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);
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... :-)
Rene.
* 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.
- 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 -
* 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
On 11-08-08 20:33, Ingo Molnar wrote:
> * Rene Herman <[email protected]> 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
[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 -