Re: [PATCH v2] x86/apic: Move pending intr check code into it's own function

From: Ingo Molnar
Date: Fri Feb 23 2018 - 02:09:04 EST



* Dou Liyang <douly.fnst@xxxxxxxxxxxxxx> wrote:

> the pending interrupt check code is mixed with the local APIC setup code,
> that looks messy.
>
> Extract the related code, move it into a new function named
> apic_pending_intr_clear().
>
> bonus cleanups from Andy Shevchenko's suggestions:
>
> - for() -> for_each_set_bit()
> - printk() -> pr_err()

Please split the cleanups (and the cleanups suggested further below) into a
separate patch, so that there's a pure 'code movement' patch plus another patch
that is easy to review.

> + /*
> + * After a crash, we no longer service the interrupts and a pending
> + * interrupt from previous kernel might still have ISR bit set.
> + *
> + * Most probably by now CPU has serviced that pending interrupt and
> + * it might not have done the ack_APIC_irq() because it thought,
> + * interrupt came from i8259 as ExtInt. LAPIC did not get EOI so it
> + * does not clear the ISR bit and cpu thinks it has already serivced
> + * the interrupt. Hence a vector might get locked. It was noticed
> + * for timer irq (vector 0x31). Issue an extra EOI to clear ISR.
> + */
> + do {
> + queued = 0;
> + for (i = APIC_ISR_NR - 1; i >= 0; i--)
> + queued |= apic_read(APIC_IRR + i*0x10);
> +
> + for (i = APIC_ISR_NR - 1; i >= 0; i--) {
> + value = apic_read(APIC_ISR + i*0x10);
> + for_each_set_bit(j, &value, 32) {
> + if (j) {
> + ack_APIC_irq();
> + acked++;
> + }
> + }
> + }
> + if (acked > 256) {
> + pr_err("LAPIC pending interrupts after %d EOI\n",
> + acked);

Please don't
break the line of
printk's.


> + if (queued) {
> + if (boot_cpu_has(X86_FEATURE_TSC) && cpu_khz) {
> + ntsc = rdtsc();
> + max_loops = (cpu_khz << 10) - (ntsc - tsc);
> + } else
> + max_loops--;

unbalanced curly braces.

Thanks,

Ingo