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

This is a discussion on [PATCH] x86: kill arch/x86/kernel/mpparse.c debugging printk. - Kernel ; Hi Andrew. Rene. From 00acea21e579ea0853baba25420f373ea83533a3 Mon Sep 17 00:00:00 2001 From: Rene Herman 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. ...

+ Reply to Thread
Results 1 to 15 of 15

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

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

    Hi Andrew.



    Rene.

    From 00acea21e579ea0853baba25420f373ea83533a3 Mon Sep 17 00:00:00 2001
    From: Rene Herman
    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
    ---
    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



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


    * Rene Herman wrote:

    > Hi Andrew.
    >
    >
    >
    > Rene.


    > >From 00acea21e579ea0853baba25420f373ea83533a3 Mon Sep 17 00:00:00 2001

    > From: Rene Herman
    > 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
    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

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

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



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

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

    >> From: Rene Herman
    >> 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.

    From 3d6ab02d08c3597cd24581968dd0b41f3c264716 Mon Sep 17 00:00:00 2001
    From: Rene Herman
    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
    ---
    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



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


    * Rene Herman 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
    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

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

    [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
    | 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
    | ---
    | 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 -
    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

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

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

    > | From: Rene Herman
    > | 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.

    From 98cf69c9acbc2c1a29fdaa6fc8c29f1bacae8316 Mon Sep 17 00:00:00 2001
    From: Rene Herman
    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
    ---
    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



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


    * Rene Herman wrote:

    > On 11-08-08 18:45, Cyrill Gorcunov wrote:
    >
    >> | From: Rene Herman
    >> | 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
    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
    Signed-off-by: Rene Herman
    Signed-off-by: Ingo Molnar
    ---
    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);
    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

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

    On 11-08-08 19:41, Ingo Molnar wrote:

    > * Rene Herman 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^

    $ git commit -a -c ORIG_HEAD
    $ git am tmp/*
    $ rm -rf tmp

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

    Rene.
    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

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


    * Rene Herman wrote:

    > On 11-08-08 19:41, Ingo Molnar wrote:
    >
    >> * Rene Herman 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^
    >
    > $ 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
    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

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

    [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 -
    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

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


    * Cyrill Gorcunov 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
    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

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

    [Ingo Molnar - Mon, Aug 11, 2008 at 08:33:00PM +0200]
    |
    | * Rene Herman wrote:
    |
    | > On 11-08-08 19:41, Ingo Molnar wrote:
    | >
    | >> * Rene Herman 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^
    | >
    | > $ 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 -
    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

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

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

    > * Rene Herman 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^
    >>
    >> $ 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
    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

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

    [Ingo Molnar - Mon, Aug 11, 2008 at 08:49:03PM +0200]
    |
    | * Cyrill Gorcunov 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 -
    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

+ Reply to Thread