lkml.org 
[lkml]   [2006]   [Dec]   [1]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH 2/4] [APIC] Allow disabling of UP APIC/IO-APIC by default, with command line option to turn it on.

    * Ben Collins <bcollins@ubuntu.com> wrote:

    > +config X86_UP_APIC_DEFAULT_OFF
    > + bool "APIC support on uniprocessors defaults to off"
    > + depends on X86_UP_APIC
    > + default n

    'n' is the default

    > /*
    > * Knob to control our willingness to enable the local APIC.
    > + * -2=default-disable, -1=force-disable, 1=force-enable, 0=automatic
    > */
    > -static int enable_local_apic __initdata = 0; /* -1=force-disable, +1=force-enable */
    > +static int enable_local_apic __initdata = (X86_APIC_DEFAULT_OFF ? -2 : 0);

    i guess this begs for enums?

    > if (enable_local_apic <= 0) {
    > - printk("Local APIC disabled by BIOS -- "
    > + printk("Local APIC disabled by BIOS (or by default) -- "
    > "you can enable it with \"lapic\"\n");

    that message should be more intelligent, depending on whether the value
    is 0, -1 or -2.

    > + /* If local apic is off due to config_x86_apic_off option, jump
    > + * out here. */

    nitpick: proper comment style for new code is:

    /*
    * If local APIC is off due to config_x86_apic_off option, jump
    * out here.
    */

    > + if (enable_local_apic < -1) {
    > + printk(KERN_INFO "Local APIC disabled by default -- "
    > + "use 'lapic' to enable it.\n");
    > + return -1;
    > + }

    this should be enable_local_apic == -2. (and should use the enum)

    > -int skip_ioapic_setup;
    > +int skip_ioapic_setup = X86_APIC_DEFAULT_OFF;

    nitpick: should be X86_IOAPIC_DEFAULT_VALUE - if the config option is
    not set then this 'OFF' value will mean 'on' ...

    > +static int __init parse_apic(char *arg)
    > +{
    > + /* enable IO-APIC */
    > + enable_ioapic_setup();
    > + return 0;
    > +}
    > +early_param("apic", parse_apic);

    that should be "ioapic", not "apic". The CPU has a piece of silicon
    called the "local APIC" - enabled via the 'lapic' option, and disabled
    via noapic. What the option above wants to enable is the IO-APIC in the
    chipset (a different piece of silicon) and the interrupt routing
    capabilities attached to it. That piece is what is causing the installer
    problems.

    looks good in principle, but needs these cleanups.

    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/

    \
     
     \ /
      Last update: 2006-12-01 09:43    [W:2.379 / U:0.852 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site