Re: [PATCH v2] x86/apic: Move pending intr check code into it's own function
Hi Dou,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on tip/auto-latest]
[also build test ERROR on v4.16-rc2 next-20180223]
[cannot apply to tip/x86/core]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system]
url:
https://github.com/0day-ci/linux/commits/Dou-Liyang/x86-apic-Move-pending-intr-check-code-into-it-s-own-function/20180224-004030
config: i386-randconfig-x079-201807 (attached as .config)
compiler: gcc-7 (Debian 7.3.0-1) 7.3.0
reproduce:
# save the attached .config to linux build tree
make ARCH=i386
All errors (new ones prefixed by >>):
arch/x86/kernel/apic/apic.c: In function 'setup_local_APIC':
>> arch/x86/kernel/apic/apic.c:1508:2: error: 'i' undeclared (first use in this
>> function); did you mean 'if'?
i = early_per_cpu(x86_cpu_to_logical_apicid, cpu);
^
if
arch/x86/kernel/apic/apic.c:1508:2: note: each undeclared identifier is
reported only once for each function it appears in
vim +1508 arch/x86/kernel/apic/apic.c
0d827285 arch/x86/kernel/apic/apic.c Dou Liyang 2018-02-23 1461
0e078e2f arch/x86/kernel/apic_64.c Thomas Gleixner2008-01-30 1462 /**
0e078e2f arch/x86/kernel/apic_64.c Thomas Gleixner2008-01-30 1463 *
setup_local_APIC - setup the local APIC
0aa002fe arch/x86/kernel/apic/apic.c Tejun Heo 2010-12-09 1464 *
543113d2 arch/x86/kernel/apic/apic.c Dou Liyang 2017-02-07 1465 *
Used to setup local APIC while initializing BSP or bringing up APs.
0aa002fe arch/x86/kernel/apic/apic.c Tejun Heo 2010-12-09 1466 *
Always called with preemption disabled.
0e078e2f arch/x86/kernel/apic_64.c Thomas Gleixner2008-01-30 1467 */
b753a2b7 arch/x86/kernel/apic/apic.c Dou Liyang 2018-02-14 1468
static void setup_local_APIC(void)
^1da177e arch/x86_64/kernel/apic.c Linus Torvalds 2005-04-16 1469 {
0aa002fe arch/x86/kernel/apic/apic.c Tejun Heo 2010-12-09 1470
int cpu = smp_processor_id();
0d827285 arch/x86/kernel/apic/apic.c Dou Liyang 2018-02-23 1471
unsigned int value;
^1da177e arch/x86_64/kernel/apic.c Linus Torvalds 2005-04-16 1472
f1182638 arch/x86/kernel/apic.c Jan Beulich2009-01-14 1473
if (disable_apic) {
7167d08e arch/x86/kernel/apic/apic.c Henrik Kretzschmar 2011-02-22 1474
disable_ioapic_support();
f1182638 arch/x86/kernel/apic.c Jan Beulich2009-01-14 1475
return;
f1182638 arch/x86/kernel/apic.c Jan Beulich2009-01-14 1476
}
f1182638 arch/x86/kernel/apic.c Jan Beulich2009-01-14 1477
89c38c28 arch/x86/kernel/apic_64.c Cyrill Gorcunov2008-08-24 1478
#ifdef CONFIG_X86_32
89c38c28 arch/x86/kernel/apic_64.c Cyrill Gorcunov2008-08-24 1479
/* Pound the ESR really hard over the head with a big hammer - mbligh */
08125d3e arch/x86/kernel/apic.c Ingo Molnar2009-01-28 1480
if (lapic_is_integrated() && apic->disable_esr) {
89c38c28 arch/x86/kernel/apic_64.c Cyrill Gorcunov2008-08-24 1481
apic_write(APIC_ESR, 0);
89c38c28 arch/x86/kernel/apic_64.c Cyrill Gorcunov2008-08-24 1482
apic_write(APIC_ESR, 0);
89c38c28 arch/x86/kernel/apic_64.c Cyrill Gorcunov2008-08-24 1483
apic_write(APIC_ESR, 0);
89c38c28 arch/x86/kernel/apic_64.c Cyrill Gorcunov2008-08-24 1484
apic_write(APIC_ESR, 0);
89c38c28 arch/x86/kernel/apic_64.c Cyrill Gorcunov2008-08-24 1485
}
89c38c28 arch/x86/kernel/apic_64.c Cyrill Gorcunov2008-08-24 1486 #endif
cdd6c482 arch/x86/kernel/apic/apic.c Ingo Molnar2009-09-21 1487
perf_events_lapic_init();
89c38c28 arch/x86/kernel/apic_64.c Cyrill Gorcunov2008-08-24 1488
^1da177e arch/x86_64/kernel/apic.c Linus Torvalds 2005-04-16 1489
/*
^1da177e arch/x86_64/kernel/apic.c Linus Torvalds 2005-04-16 1490
* Double-check whether this APIC is really registered.
^1da177e arch/x86_64/kernel/apic.c Linus Torvalds 2005-04-16 1491
* This is meaningless in clustered apic mode, so we skip it.
^1da177e arch/x86_64/kernel/apic.c Linus Torvalds 2005-04-16 1492
*/
c2777f98 arch/x86/kernel/apic/apic.c Daniel Walker 2009-09-12 1493
BUG_ON(!apic->apic_id_registered());
^1da177e arch/x86_64/kernel/apic.c Linus Torvalds 2005-04-16 1494
^1da177e arch/x86_64/kernel/apic.c Linus Torvalds 2005-04-16 1495
/*
^1da177e arch/x86_64/kernel/apic.c Linus Torvalds 2005-04-16 1496
* Intel recommends to set DFR, LDR and TPR before enabling
^1da177e arch/x86_64/kernel/apic.c Linus Torvalds 2005-04-16 1497
* an APIC. See e.g. "AP-388 82489DX User's Manual" (Intel
^1da177e arch/x86_64/kernel/apic.c Linus Torvalds 2005-04-16 1498
* document number 29211
Re: [PATCH v2] x86/apic: Move pending intr check code into it's own function
Hi Dou,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on tip/auto-latest]
[also build test ERROR on v4.16-rc2 next-20180223]
[cannot apply to tip/x86/core]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system]
url:
https://github.com/0day-ci/linux/commits/Dou-Liyang/x86-apic-Move-pending-intr-check-code-into-it-s-own-function/20180224-004030
config: i386-randconfig-x010-201807 (attached as .config)
compiler: gcc-7 (Debian 7.3.0-1) 7.3.0
reproduce:
# save the attached .config to linux build tree
make ARCH=i386
All errors (new ones prefixed by >>):
arch/x86/kernel/apic/apic.c: In function 'setup_local_APIC':
>> arch/x86/kernel/apic/apic.c:1508:2: error: 'i' undeclared (first use in this
>> function)
i = early_per_cpu(x86_cpu_to_logical_apicid, cpu);
^
arch/x86/kernel/apic/apic.c:1508:2: note: each undeclared identifier is
reported only once for each function it appears in
vim +/i +1508 arch/x86/kernel/apic/apic.c
0d827285 arch/x86/kernel/apic/apic.c Dou Liyang 2018-02-23 1461
0e078e2f arch/x86/kernel/apic_64.c Thomas Gleixner2008-01-30 1462 /**
0e078e2f arch/x86/kernel/apic_64.c Thomas Gleixner2008-01-30 1463 *
setup_local_APIC - setup the local APIC
0aa002fe arch/x86/kernel/apic/apic.c Tejun Heo 2010-12-09 1464 *
543113d2 arch/x86/kernel/apic/apic.c Dou Liyang 2017-02-07 1465 *
Used to setup local APIC while initializing BSP or bringing up APs.
0aa002fe arch/x86/kernel/apic/apic.c Tejun Heo 2010-12-09 1466 *
Always called with preemption disabled.
0e078e2f arch/x86/kernel/apic_64.c Thomas Gleixner2008-01-30 1467 */
b753a2b7 arch/x86/kernel/apic/apic.c Dou Liyang 2018-02-14 1468
static void setup_local_APIC(void)
^1da177e arch/x86_64/kernel/apic.c Linus Torvalds 2005-04-16 1469 {
0aa002fe arch/x86/kernel/apic/apic.c Tejun Heo 2010-12-09 1470
int cpu = smp_processor_id();
0d827285 arch/x86/kernel/apic/apic.c Dou Liyang 2018-02-23 1471
unsigned int value;
^1da177e arch/x86_64/kernel/apic.c Linus Torvalds 2005-04-16 1472
f1182638 arch/x86/kernel/apic.c Jan Beulich2009-01-14 1473
if (disable_apic) {
7167d08e arch/x86/kernel/apic/apic.c Henrik Kretzschmar 2011-02-22 1474
disable_ioapic_support();
f1182638 arch/x86/kernel/apic.c Jan Beulich2009-01-14 1475
return;
f1182638 arch/x86/kernel/apic.c Jan Beulich2009-01-14 1476
}
f1182638 arch/x86/kernel/apic.c Jan Beulich2009-01-14 1477
89c38c28 arch/x86/kernel/apic_64.c Cyrill Gorcunov2008-08-24 1478
#ifdef CONFIG_X86_32
89c38c28 arch/x86/kernel/apic_64.c Cyrill Gorcunov2008-08-24 1479
/* Pound the ESR really hard over the head with a big hammer - mbligh */
08125d3e arch/x86/kernel/apic.c Ingo Molnar2009-01-28 1480
if (lapic_is_integrated() && apic->disable_esr) {
89c38c28 arch/x86/kernel/apic_64.c Cyrill Gorcunov2008-08-24 1481
apic_write(APIC_ESR, 0);
89c38c28 arch/x86/kernel/apic_64.c Cyrill Gorcunov2008-08-24 1482
apic_write(APIC_ESR, 0);
89c38c28 arch/x86/kernel/apic_64.c Cyrill Gorcunov2008-08-24 1483
apic_write(APIC_ESR, 0);
89c38c28 arch/x86/kernel/apic_64.c Cyrill Gorcunov2008-08-24 1484
apic_write(APIC_ESR, 0);
89c38c28 arch/x86/kernel/apic_64.c Cyrill Gorcunov2008-08-24 1485
}
89c38c28 arch/x86/kernel/apic_64.c Cyrill Gorcunov2008-08-24 1486 #endif
cdd6c482 arch/x86/kernel/apic/apic.c Ingo Molnar2009-09-21 1487
perf_events_lapic_init();
89c38c28 arch/x86/kernel/apic_64.c Cyrill Gorcunov2008-08-24 1488
^1da177e arch/x86_64/kernel/apic.c Linus Torvalds 2005-04-16 1489
/*
^1da177e arch/x86_64/kernel/apic.c Linus Torvalds 2005-04-16 1490
* Double-check whether this APIC is really registered.
^1da177e arch/x86_64/kernel/apic.c Linus Torvalds 2005-04-16 1491
* This is meaningless in clustered apic mode, so we skip it.
^1da177e arch/x86_64/kernel/apic.c Linus Torvalds 2005-04-16 1492
*/
c2777f98 arch/x86/kernel/apic/apic.c Daniel Walker 2009-09-12 1493
BUG_ON(!apic->apic_id_registered());
^1da177e arch/x86_64/kernel/apic.c Linus Torvalds 2005-04-16 1494
^1da177e arch/x86_64/kernel/apic.c Linus Torvalds 2005-04-16 1495
/*
^1da177e arch/x86_64/kernel/apic.c Linus Torvalds 2005-04-16 1496
* Intel recommends to set DFR, LDR and TPR before enabling
^1da177e arch/x86_64/kernel/apic.c Linus Torvalds 2005-04-16 1497
* an APIC. See e.g. "AP-388 82489DX User's Manual" (Intel
^1da177e arch/x86_64/kernel/apic.c Linus Torvalds 2005-04-16 1498
* document number 292116). So here it goes...
Re: [PATCH v2] x86/apic: Move pending intr check code into it's own function
* Dou Liyang 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
[PATCH v2] x86/apic: Move pending intr check code into it's own function
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()
Signed-off-by: Dou Liyang
---
changlog:
v1 --> v2:
-do some cleanup suggested by Andy
-rebase the patch on the tip/master tree
arch/x86/kernel/apic/apic.c | 99 -
1 file changed, 53 insertions(+), 46 deletions(-)
diff --git a/arch/x86/kernel/apic/apic.c b/arch/x86/kernel/apic/apic.c
index 84b244ec49ed..687457d9df41 100644
--- a/arch/x86/kernel/apic/apic.c
+++ b/arch/x86/kernel/apic/apic.c
@@ -1408,6 +1408,57 @@ static void lapic_setup_esr(void)
oldvalue, value);
}
+static void apic_pending_intr_clear(void)
+{
+ long long max_loops = cpu_khz ? cpu_khz : 100;
+ unsigned long long tsc = 0, ntsc;
+ unsigned int queued;
+ unsigned long value;
+ int i, j, acked = 0;
+
+ if (boot_cpu_has(X86_FEATURE_TSC))
+ tsc = rdtsc();
+ /*
+* 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);
+ break;
+ }
+ if (queued) {
+ if (boot_cpu_has(X86_FEATURE_TSC) && cpu_khz) {
+ ntsc = rdtsc();
+ max_loops = (cpu_khz << 10) - (ntsc - tsc);
+ } else
+ max_loops--;
+ }
+ } while (queued && max_loops > 0);
+ WARN_ON(max_loops <= 0);
+}
+
/**
* setup_local_APIC - setup the local APIC
*
@@ -1417,13 +1468,7 @@ static void lapic_setup_esr(void)
static void setup_local_APIC(void)
{
int cpu = smp_processor_id();
- unsigned int value, queued;
- int i, j, acked = 0;
- unsigned long long tsc = 0, ntsc;
- long long max_loops = cpu_khz ? cpu_khz : 100;
-
- if (boot_cpu_has(X86_FEATURE_TSC))
- tsc = rdtsc();
+ unsigned int value;
if (disable_apic) {
disable_ioapic_support();
@@ -1475,45 +1520,7 @@ static void setup_local_APIC(void)
value &= ~APIC_TPRI_MASK;
apic_write(APIC_TASKPRI, value);
- /*
-* 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 (j = 31; j >= 0; j--) {
- if (value & (1< 256) {
- printk(KERN_ERR "LAPIC pending interrupts after %d
EOI\n",
- acked);
- break;
- }
- if (queued) {
- if (boot_cpu_has(X86_FEATURE_TSC) && cpu_khz) {
- ntsc = rdtsc();
- max_loops = (cpu_khz
