| Message ID | 1fa26228fa94779d12d4089b83a43fe157b110fe.1498795030.git.douly.fnst@cn.fujitsu.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, 30 Jun 2017, Dou Liyang wrote: > static inline int apic_force_enable(unsigned long addr) > diff --git a/arch/x86/kernel/apic/apic.c b/arch/x86/kernel/apic/apic.c > index 0601054..9bf7e95 100644 > --- a/arch/x86/kernel/apic/apic.c > +++ b/arch/x86/kernel/apic/apic.c > @@ -1198,6 +1198,10 @@ static int __init apic_intr_mode_select(int *upmode) > } > #endif > > +#ifdef CONFIG_UP_LATE_INIT > + *upmode = true; > +#endif This is really wrong. The upmode decision, which is required for calling apic_bsp_setup() should not happen here, really. As I told you in the previous patch, use the return code and then you can make further decisions in apic_intr_mode_init(). And you do it there w/o any ifdeffery: static void apic_intr_mode_init(void) { bool upmode = IS_ENABLED(CONFIG_UP_LATE_INIT); switch (....) { case XXXX: upmode = true; .... } apic_bsp_setup(upmode); } Thanks, tglx
Hi Thomas, At 07/03/2017 02:19 AM, Thomas Gleixner wrote: > On Fri, 30 Jun 2017, Dou Liyang wrote: >> static inline int apic_force_enable(unsigned long addr) >> diff --git a/arch/x86/kernel/apic/apic.c b/arch/x86/kernel/apic/apic.c >> index 0601054..9bf7e95 100644 >> --- a/arch/x86/kernel/apic/apic.c >> +++ b/arch/x86/kernel/apic/apic.c >> @@ -1198,6 +1198,10 @@ static int __init apic_intr_mode_select(int *upmode) >> } >> #endif >> >> +#ifdef CONFIG_UP_LATE_INIT >> + *upmode = true; >> +#endif > > This is really wrong. The upmode decision, which is required for calling > apic_bsp_setup() should not happen here, really. As I told you in the > previous patch, use the return code and then you can make further decisions > in apic_intr_mode_init(). Really thanks for your detail explaining, I learned more than i read from books about the programming skill. > > And you do it there w/o any ifdeffery: > > static void apic_intr_mode_init(void) > { > bool upmode = IS_ENABLED(CONFIG_UP_LATE_INIT); > > switch (....) { > case XXXX: > upmode = true; > .... > } > apic_bsp_setup(upmode); > } This looks more beautiful than mine. I will use it in the next version. Thanks, dou. > > Thanks, > > tglx > > >
diff --git a/arch/x86/include/asm/apic.h b/arch/x86/include/asm/apic.h index bfbf715..b35bbbf 100644 --- a/arch/x86/include/asm/apic.h +++ b/arch/x86/include/asm/apic.h @@ -144,7 +144,6 @@ void register_lapic_address(unsigned long address); extern void setup_boot_APIC_clock(void); extern void setup_secondary_APIC_clock(void); extern void lapic_update_tsc_freq(void); -extern int APIC_init_uniprocessor(void); #ifdef CONFIG_X86_64 static inline int apic_force_enable(unsigned long addr) diff --git a/arch/x86/kernel/apic/apic.c b/arch/x86/kernel/apic/apic.c index 0601054..9bf7e95 100644 --- a/arch/x86/kernel/apic/apic.c +++ b/arch/x86/kernel/apic/apic.c @@ -1198,6 +1198,10 @@ static int __init apic_intr_mode_select(int *upmode) } #endif +#ifdef CONFIG_UP_LATE_INIT + *upmode = true; +#endif + /* Check MP table or ACPI MADT configuration */ if (!smp_found_config) { disable_ioapic_support(); @@ -2380,51 +2384,16 @@ void __init apic_bsp_setup(bool upmode) setup_IO_APIC(); } -/* - * This initializes the IO-APIC and APIC hardware if this is - * a UP kernel. - */ -int __init APIC_init_uniprocessor(void) +#ifdef CONFIG_UP_LATE_INIT +void __init up_late_init(void) { - if (disable_apic) { - pr_info("Apic disabled\n"); - return -1; - } -#ifdef CONFIG_X86_64 - if (!boot_cpu_has(X86_FEATURE_APIC)) { - disable_apic = 1; - pr_info("Apic disabled by BIOS\n"); - return -1; - } -#else - if (!smp_found_config && !boot_cpu_has(X86_FEATURE_APIC)) - return -1; - - /* - * Complain if the BIOS pretends there is one. - */ - if (!boot_cpu_has(X86_FEATURE_APIC) && - APIC_INTEGRATED(boot_cpu_apic_version)) { - pr_err("BIOS bug, local APIC 0x%x not detected!...\n", - boot_cpu_physical_apicid); - return -1; - } -#endif + apic_intr_mode_init(); - if (!smp_found_config) - disable_ioapic_support(); + if (apic_intr_mode == APIC_PIC) + return; - default_setup_apic_routing(); - apic_bsp_setup(true); /* Setup local timer */ x86_init.timers.setup_percpu_clockev(); - return 0; -} - -#ifdef CONFIG_UP_LATE_INIT -void __init up_late_init(void) -{ - APIC_init_uniprocessor(); } #endif
In UniProcessor kernel with UP_LATE_INIT=y, it enables and setups interrupt delivery mode in up_late_init(). Unify it to apic_intr_mode_init(), remove APIC_init_uniprocessor(). Signed-off-by: Dou Liyang <douly.fnst@cn.fujitsu.com> --- arch/x86/include/asm/apic.h | 1 - arch/x86/kernel/apic/apic.c | 49 +++++++++------------------------------------ 2 files changed, 9 insertions(+), 41 deletions(-)