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


\
 
 \ /
  Last update: 2017-07-03 03:45    [W:0.062 / U:1.964 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site