lkml.org 
[lkml]   [2017]   [Jul]   [2]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH v5 01/12] x86/apic: Construct a selector for the interrupt delivery mode
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

\
 
 \ /
  Last update: 2017-07-02 19:38    [W:0.192 / U:27.932 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site