Messages in this thread | | | Date | Fri, 20 Feb 2009 10:51:27 +0100 | From | Ingo Molnar <> | Subject | Re: [PATCH] x86: enable x2apic early at the first point |
| |
* Yinghai Lu <yinghai@kernel.org> wrote:
> Impact: fix bug. > > otherwise will get panic from early_acpi_boot_init() > also make disable_x2apic global, so could use it it x2_apic_xxx.c > and can get warning if preenabled system using nox2apic. > > Signed-off-by: Yinghai Lu <yinghai@kernel.org> > > --- > arch/x86/kernel/apic/apic.c | 3 +-- > arch/x86/kernel/apic/x2apic_cluster.c | 5 ++++- > arch/x86/kernel/apic/x2apic_phys.c | 5 ++++- > arch/x86/kernel/apic/x2apic_uv_x.c | 4 +++- > drivers/pci/dmar.c | 3 ++- > 5 files changed, 14 insertions(+), 6 deletions(-)
I've applied it because it fixes a real bug, but this code really needs a cleanup. Look at the repeat patterns:
> Index: linux-2.6/arch/x86/kernel/apic/x2apic_cluster.c > =================================================================== > --- linux-2.6.orig/arch/x86/kernel/apic/x2apic_cluster.c > +++ linux-2.6/arch/x86/kernel/apic/x2apic_cluster.c > @@ -14,8 +14,11 @@ DEFINE_PER_CPU(u32, x86_cpu_to_logical_a > > static int x2apic_acpi_madt_oem_check(char *oem_id, char *oem_table_id) > { > - if (cpu_has_x2apic) > + if (cpu_has_x2apic && !disable_x2apic) { > + x2apic = 1; > + enable_x2apic(); > return 1; > + } > > return 0; > } > Index: linux-2.6/arch/x86/kernel/apic/x2apic_phys.c > =================================================================== > --- linux-2.6.orig/arch/x86/kernel/apic/x2apic_phys.c > +++ linux-2.6/arch/x86/kernel/apic/x2apic_phys.c > @@ -21,8 +21,11 @@ early_param("x2apic_phys", set_x2apic_ph > > static int x2apic_acpi_madt_oem_check(char *oem_id, char *oem_table_id) > { > - if (cpu_has_x2apic && x2apic_phys) > + if (cpu_has_x2apic && !disable_x2apic && x2apic_phys) { > + x2apic = 1; > + enable_x2apic(); > return 1; > + } > > return 0; > } > Index: linux-2.6/arch/x86/kernel/apic/x2apic_uv_x.c > =================================================================== > --- linux-2.6.orig/arch/x86/kernel/apic/x2apic_uv_x.c > +++ linux-2.6/arch/x86/kernel/apic/x2apic_uv_x.c > @@ -41,8 +41,10 @@ static int uv_acpi_madt_oem_check(char * > uv_system_type = UV_LEGACY_APIC; > else if (!strcmp(oem_table_id, "UVX")) > uv_system_type = UV_X2APIC; > - else if (!strcmp(oem_table_id, "UVH")) { > + else if (!strcmp(oem_table_id, "UVH") && !disable_x2apic) { > uv_system_type = UV_NON_UNIQUE_APIC; > + x2apic = 1; > + enable_x2apic(); > return 1; > } > }
Such repeat patterns with small variations are always the sign of an inefficient code structure.
The clean approach would be to have one generic helper function concentrated into enable_x2apic(). So instead of:
static int x2apic_acpi_madt_oem_check(char *oem_id, char *oem_table_id) { if (cpu_has_x2apic && !disable_x2apic && x2apic_phys) { x2apic = 1; enable_x2apic(); return 1; }
return 0; }
We'd have:
static int x2apic_acpi_madt_oem_check(char *oem_id, char *oem_table_id) { if (!x2apic_phys) return 0;
return x86_enable_x2apic(); }
That way all the repeat and common functionality (and the return code logic) is concentrated into enable_x2apic(), and the x2apic_acpi_madt_oem_check() function has _only_ its own special check open-coded. (namely, whether physical ID delivery mode is forced off.)
Okay?
Ingo
| |