Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752342AbdGCBoR (ORCPT ); Sun, 2 Jul 2017 21:44:17 -0400 Received: from cn.fujitsu.com ([59.151.112.132]:42600 "EHLO heian.cn.fujitsu.com" rhost-flags-OK-FAIL-OK-FAIL) by vger.kernel.org with ESMTP id S1752100AbdGCBoP (ORCPT ); Sun, 2 Jul 2017 21:44:15 -0400 X-IronPort-AV: E=Sophos;i="5.22,518,1449504000"; d="scan'208";a="20758566" Subject: Re: [PATCH v5 01/12] x86/apic: Construct a selector for the interrupt delivery mode To: Thomas Gleixner References: <15d82d40aeec8d5935cca2c64ead8710861cd5f8.1498795030.git.douly.fnst@cn.fujitsu.com> CC: , , , , , , , , , From: Dou Liyang Message-ID: <6bbe35a1-2d8b-4891-1b0d-f0e4b54008f3@cn.fujitsu.com> Date: Mon, 3 Jul 2017 09:44:09 +0800 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.4.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset="windows-1252"; format=flowed Content-Transfer-Encoding: 7bit X-Originating-IP: [10.167.226.106] X-yoursite-MailScanner-ID: 8C3A647E9887.AD2D4 X-yoursite-MailScanner: Found to be clean X-yoursite-MailScanner-From: douly.fnst@cn.fujitsu.com Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2433 Lines: 103 Hi Thomas, At 07/03/2017 01:37 AM, Thomas Gleixner wrote: > 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, I see, it an unnecessary and surplus thing I did. but it's completely non obvious and does > not really make the code more understandable. Quite the contrary. You are right. > >> + 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. Will keep the order like the existing logic you gave above. > >> +#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. Yes, remove right now. Thanks, dou. > >> + return APIC_SYMMETRIC_IO; >> +} > > Thanks, > > tglx > > >