Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751983AbdGBRhT (ORCPT ); Sun, 2 Jul 2017 13:37:19 -0400 Received: from Galois.linutronix.de ([146.0.238.70]:40209 "EHLO Galois.linutronix.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751759AbdGBRhR (ORCPT ); Sun, 2 Jul 2017 13:37:17 -0400 Date: Sun, 2 Jul 2017 19:37:10 +0200 (CEST) From: Thomas Gleixner To: Dou Liyang cc: x86@kernel.org, linux-kernel@vger.kernel.org, xen-devel@lists.xenproject.org, mingo@kernel.org, hpa@zytor.com, ebiederm@xmission.com, bhe@redhat.com, boris.ostrovsky@oracle.com, peterz@infradead.org, izumi.taku@jp.fujitsu.com Subject: Re: [PATCH v5 01/12] x86/apic: Construct a selector for the interrupt delivery mode In-Reply-To: <15d82d40aeec8d5935cca2c64ead8710861cd5f8.1498795030.git.douly.fnst@cn.fujitsu.com> Message-ID: References: <15d82d40aeec8d5935cca2c64ead8710861cd5f8.1498795030.git.douly.fnst@cn.fujitsu.com> User-Agent: Alpine 2.20 (DEB 67 2015-01-07) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2098 Lines: 80 On Fri, 30 Jun 2017, Dou Liyang wrote: > +static int __init apic_intr_mode_select(void) > +{ > + /* Check kernel option */ > + if (disable_apic) { > + pr_info("APIC disabled via kernel command line\n"); > + return APIC_PIC; > + } > + > + /* Check BIOS */ > +#ifdef CONFIG_X86_64 > + /* On 64-bit, the APIC must be integrated, Check local APIC only */ > + if (!boot_cpu_has(X86_FEATURE_APIC)) { > + disable_apic = 1; > + pr_info("APIC disabled by BIOS\n"); > + return APIC_PIC; > + } > +#else > + /* > + * On 32-bit, check whether there is a separate chip or integrated > + * APIC > + */ > + > + /* Has a local APIC ? */ > + if (!boot_cpu_has(X86_FEATURE_APIC) && > + APIC_INTEGRATED(boot_cpu_apic_version)) { This looks wrong. The existing logic is: if (!boot_cpu_has(X86_FEATURE_APIC) && !smp_found_config) return -1; if (!boot_cpu_has(X86_FEATURE_APIC) && APIC_INTEGRATED(boot_cpu_apic_version)) { pr_err(....); I know that this is magically the same because boot_cpu_apic_version is 0 in the !boot_cpu_has(X86_FEATURE_APIC) && !smp_found_config case, so you don't fall into that conditional, but it's completely non obvious and does not really make the code more understandable. Quite the contrary. > + disable_apic = 1; > + pr_err(FW_BUG "Local APIC %d not detected, force emulation\n", > + boot_cpu_physical_apicid); > + return APIC_PIC; > + } > + > + /* Has a separate chip ? */ > + if (!boot_cpu_has(X86_FEATURE_APIC) && !smp_found_config) { > + disable_apic = 1; > + > + return APIC_PIC; > + } So if you move exactly that check above the other then it's clear what's going on. > +#endif > + > + /* Check MP table or ACPI MADT configuration */ > + if (!smp_found_config) { > + disable_ioapic_support(); > + > + if (!acpi_lapic) > + pr_info("APIC: ACPI MADT or MP tables are not detected\n"); > + > + return APIC_VIRTUAL_WIRE; > + } > + > + /* Other checks of APIC options will be done in each setup function */ > + Please remove the extra new line. It's not helping readability. > + return APIC_SYMMETRIC_IO; > +} Thanks, tglx