| Message ID | 20201125155113.192079-9-alexandru.elisei@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | GIC fixes and improvements | expand |
On 11/25/20 4:51 PM, Alexandru Elisei wrote: > check_acked() has several peculiarities: is the only function among the > check_* functions which calls report() directly, it does two things > (waits for interrupts and checks for misfired interrupts) and it also > mixes printf, report_info and report calls. > > check_acked() also reports a pass and returns as soon all the target CPUs > have received interrupts, However, a CPU not having received an interrupt > *now* does not guarantee not receiving an eroneous interrupt if we wait erroneous > long enough. > > Rework the function by splitting it into two separate functions, each with > a single responsability: wait_for_interrupts(), which waits for the > expected interrupts to fire, and check_acked() which checks that interrupts > have been received as expected. > > wait_for_interrupts() also waits an extra 100 milliseconds after the > expected interrupts have been received in an effort to make sure we don't > miss misfiring interrupts. > > Splitting check_acked() into two functions will also allow us to > customize the behavior of each function in the future more easily > without using an unnecessarily long list of arguments for check_acked(). > > CC: Andre Przywara <andre.przywara@arm.com> > Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com> > --- > arm/gic.c | 73 +++++++++++++++++++++++++++++++++++-------------------- > 1 file changed, 47 insertions(+), 26 deletions(-) > > diff --git a/arm/gic.c b/arm/gic.c > index 544c283f5f47..dcdab7d5f39a 100644 > --- a/arm/gic.c > +++ b/arm/gic.c > @@ -62,41 +62,42 @@ static void stats_reset(void) > } > } > > -static void check_acked(const char *testname, cpumask_t *mask) > +static void wait_for_interrupts(cpumask_t *mask) > { > - int missing = 0, extra = 0, unexpected = 0; > int nr_pass, cpu, i; > - bool bad = false; > > /* Wait up to 5s for all interrupts to be delivered */ > - for (i = 0; i < 50; ++i) { > + for (i = 0; i < 50; i++) { > mdelay(100); > nr_pass = 0; > for_each_present_cpu(cpu) { > + /* > + * A CPU having receied more than one interrupts will received > + * show up in check_acked(), and no matter how long we > + * wait it cannot un-receive it. Consier at least one consider > + * interrupt as a pass. > + */ > nr_pass += cpumask_test_cpu(cpu, mask) ? > - acked[cpu] == 1 : acked[cpu] == 0; > - smp_rmb(); /* pairs with smp_wmb in ipi_handler */ > - > - if (bad_sender[cpu] != -1) { > - printf("cpu%d received IPI from wrong sender %d\n", > - cpu, bad_sender[cpu]); > - bad = true; > - } > - > - if (bad_irq[cpu] != -1) { > - printf("cpu%d received wrong irq %d\n", > - cpu, bad_irq[cpu]); > - bad = true; > - } > + acked[cpu] >= 1 : acked[cpu] == 0; > } > + > if (nr_pass == nr_cpus) { > - report(!bad, "%s", testname); > if (i) > - report_info("took more than %d ms", i * 100); > + report_info("interrupts took more than %d ms", i * 100); > + mdelay(100); > return; > } > } > > + report_info("interrupts timed-out (5s)"); > +} > + > +static bool check_acked(cpumask_t *mask) > +{ > + int missing = 0, extra = 0, unexpected = 0; > + bool pass = true; > + int cpu; > + > for_each_present_cpu(cpu) { > if (cpumask_test_cpu(cpu, mask)) { > if (!acked[cpu]) > @@ -107,11 +108,28 @@ static void check_acked(const char *testname, cpumask_t *mask) > if (acked[cpu]) > ++unexpected; > } > + smp_rmb(); /* pairs with smp_wmb in ipi_handler */ > + > + if (bad_sender[cpu] != -1) { > + report_info("cpu%d received IPI from wrong sender %d", > + cpu, bad_sender[cpu]); > + pass = false; > + } > + > + if (bad_irq[cpu] != -1) { > + report_info("cpu%d received wrong irq %d", > + cpu, bad_irq[cpu]); > + pass = false; > + } > + } > + > + if (missing || extra || unexpected) { > + report_info("ACKS: missing=%d extra=%d unexpected=%d", > + missing, extra, unexpected); > + pass = false; > } > > - report(false, "%s", testname); > - report_info("Timed-out (5s). ACKS: missing=%d extra=%d unexpected=%d", > - missing, extra, unexpected); > + return pass; > } > > static void check_spurious(void) > @@ -300,7 +318,8 @@ static void ipi_test_self(void) > cpumask_clear(&mask); > cpumask_set_cpu(smp_processor_id(), &mask); > gic->ipi.send_self(); > - check_acked("IPI: self", &mask); > + wait_for_interrupts(&mask); > + report(check_acked(&mask), "Interrupts received"); > report_prefix_pop(); > } > > @@ -315,7 +334,8 @@ static void ipi_test_smp(void) > for (i = smp_processor_id() & 1; i < nr_cpus; i += 2) > cpumask_clear_cpu(i, &mask); > gic_ipi_send_mask(IPI_IRQ, &mask); > - check_acked("IPI: directed", &mask); > + wait_for_interrupts(&mask); > + report(check_acked(&mask), "Interrupts received"); both ipi_test_smp and ipi_test_self are called from the same test so better to use different error messages like it was done originally. > report_prefix_pop(); > > report_prefix_push("broadcast"); > @@ -323,7 +343,8 @@ static void ipi_test_smp(void) > cpumask_copy(&mask, &cpu_present_mask); > cpumask_clear_cpu(smp_processor_id(), &mask); > gic->ipi.send_broadcast(); > - check_acked("IPI: broadcast", &mask); > + wait_for_interrupts(&mask); > + report(check_acked(&mask), "Interrupts received"); > report_prefix_pop(); > } > > Otherwise looks good to me Thanks Eric
Hi Eric, On 12/3/20 1:39 PM, Auger Eric wrote: > > On 11/25/20 4:51 PM, Alexandru Elisei wrote: >> check_acked() has several peculiarities: is the only function among the >> check_* functions which calls report() directly, it does two things >> (waits for interrupts and checks for misfired interrupts) and it also >> mixes printf, report_info and report calls. >> >> check_acked() also reports a pass and returns as soon all the target CPUs >> have received interrupts, However, a CPU not having received an interrupt >> *now* does not guarantee not receiving an eroneous interrupt if we wait > erroneous >> long enough. >> >> Rework the function by splitting it into two separate functions, each with >> a single responsability: wait_for_interrupts(), which waits for the >> expected interrupts to fire, and check_acked() which checks that interrupts >> have been received as expected. >> >> wait_for_interrupts() also waits an extra 100 milliseconds after the >> expected interrupts have been received in an effort to make sure we don't >> miss misfiring interrupts. >> >> Splitting check_acked() into two functions will also allow us to >> customize the behavior of each function in the future more easily >> without using an unnecessarily long list of arguments for check_acked(). >> >> CC: Andre Przywara <andre.przywara@arm.com> >> Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com> >> --- >> arm/gic.c | 73 +++++++++++++++++++++++++++++++++++-------------------- >> 1 file changed, 47 insertions(+), 26 deletions(-) >> >> diff --git a/arm/gic.c b/arm/gic.c >> index 544c283f5f47..dcdab7d5f39a 100644 >> --- a/arm/gic.c >> +++ b/arm/gic.c >> @@ -62,41 +62,42 @@ static void stats_reset(void) >> } >> } >> >> -static void check_acked(const char *testname, cpumask_t *mask) >> +static void wait_for_interrupts(cpumask_t *mask) >> { >> - int missing = 0, extra = 0, unexpected = 0; >> int nr_pass, cpu, i; >> - bool bad = false; >> >> /* Wait up to 5s for all interrupts to be delivered */ >> - for (i = 0; i < 50; ++i) { >> + for (i = 0; i < 50; i++) { >> mdelay(100); >> nr_pass = 0; >> for_each_present_cpu(cpu) { >> + /* >> + * A CPU having receied more than one interrupts will > received >> + * show up in check_acked(), and no matter how long we >> + * wait it cannot un-receive it. Consier at least one > consider Will fix all three typos, thanks. >> + * interrupt as a pass. >> + */ >> nr_pass += cpumask_test_cpu(cpu, mask) ? >> - acked[cpu] == 1 : acked[cpu] == 0; >> - smp_rmb(); /* pairs with smp_wmb in ipi_handler */ >> - >> - if (bad_sender[cpu] != -1) { >> - printf("cpu%d received IPI from wrong sender %d\n", >> - cpu, bad_sender[cpu]); >> - bad = true; >> - } >> - >> - if (bad_irq[cpu] != -1) { >> - printf("cpu%d received wrong irq %d\n", >> - cpu, bad_irq[cpu]); >> - bad = true; >> - } >> + acked[cpu] >= 1 : acked[cpu] == 0; >> } >> + >> if (nr_pass == nr_cpus) { >> - report(!bad, "%s", testname); >> if (i) >> - report_info("took more than %d ms", i * 100); >> + report_info("interrupts took more than %d ms", i * 100); >> + mdelay(100); >> return; >> } >> } >> >> + report_info("interrupts timed-out (5s)"); >> +} >> + >> +static bool check_acked(cpumask_t *mask) >> +{ >> + int missing = 0, extra = 0, unexpected = 0; >> + bool pass = true; >> + int cpu; >> + >> for_each_present_cpu(cpu) { >> if (cpumask_test_cpu(cpu, mask)) { >> if (!acked[cpu]) >> @@ -107,11 +108,28 @@ static void check_acked(const char *testname, cpumask_t *mask) >> if (acked[cpu]) >> ++unexpected; >> } >> + smp_rmb(); /* pairs with smp_wmb in ipi_handler */ >> + >> + if (bad_sender[cpu] != -1) { >> + report_info("cpu%d received IPI from wrong sender %d", >> + cpu, bad_sender[cpu]); >> + pass = false; >> + } >> + >> + if (bad_irq[cpu] != -1) { >> + report_info("cpu%d received wrong irq %d", >> + cpu, bad_irq[cpu]); >> + pass = false; >> + } >> + } >> + >> + if (missing || extra || unexpected) { >> + report_info("ACKS: missing=%d extra=%d unexpected=%d", >> + missing, extra, unexpected); >> + pass = false; >> } >> >> - report(false, "%s", testname); >> - report_info("Timed-out (5s). ACKS: missing=%d extra=%d unexpected=%d", >> - missing, extra, unexpected); >> + return pass; >> } >> >> static void check_spurious(void) >> @@ -300,7 +318,8 @@ static void ipi_test_self(void) >> cpumask_clear(&mask); >> cpumask_set_cpu(smp_processor_id(), &mask); >> gic->ipi.send_self(); >> - check_acked("IPI: self", &mask); >> + wait_for_interrupts(&mask); >> + report(check_acked(&mask), "Interrupts received"); >> report_prefix_pop(); >> } >> >> @@ -315,7 +334,8 @@ static void ipi_test_smp(void) >> for (i = smp_processor_id() & 1; i < nr_cpus; i += 2) >> cpumask_clear_cpu(i, &mask); >> gic_ipi_send_mask(IPI_IRQ, &mask); >> - check_acked("IPI: directed", &mask); >> + wait_for_interrupts(&mask); >> + report(check_acked(&mask), "Interrupts received"); > both ipi_test_smp and ipi_test_self are called from the same test so > better to use different error messages like it was done originally. I used the same error message because the tests have a different prefix ("target-list" versus "broadcast"). Do you think there are cases where that's not enough? Thanks, Alex
Hi Alexandru, On 12/10/20 3:45 PM, Alexandru Elisei wrote: > Hi Eric, > > On 12/3/20 1:39 PM, Auger Eric wrote: >> >> On 11/25/20 4:51 PM, Alexandru Elisei wrote: >>> check_acked() has several peculiarities: is the only function among the >>> check_* functions which calls report() directly, it does two things >>> (waits for interrupts and checks for misfired interrupts) and it also >>> mixes printf, report_info and report calls. >>> >>> check_acked() also reports a pass and returns as soon all the target CPUs >>> have received interrupts, However, a CPU not having received an interrupt >>> *now* does not guarantee not receiving an eroneous interrupt if we wait >> erroneous >>> long enough. >>> >>> Rework the function by splitting it into two separate functions, each with >>> a single responsability: wait_for_interrupts(), which waits for the >>> expected interrupts to fire, and check_acked() which checks that interrupts >>> have been received as expected. >>> >>> wait_for_interrupts() also waits an extra 100 milliseconds after the >>> expected interrupts have been received in an effort to make sure we don't >>> miss misfiring interrupts. >>> >>> Splitting check_acked() into two functions will also allow us to >>> customize the behavior of each function in the future more easily >>> without using an unnecessarily long list of arguments for check_acked(). >>> >>> CC: Andre Przywara <andre.przywara@arm.com> >>> Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com> >>> --- >>> arm/gic.c | 73 +++++++++++++++++++++++++++++++++++-------------------- >>> 1 file changed, 47 insertions(+), 26 deletions(-) >>> >>> diff --git a/arm/gic.c b/arm/gic.c >>> index 544c283f5f47..dcdab7d5f39a 100644 >>> --- a/arm/gic.c >>> +++ b/arm/gic.c >>> @@ -62,41 +62,42 @@ static void stats_reset(void) >>> } >>> } >>> >>> -static void check_acked(const char *testname, cpumask_t *mask) >>> +static void wait_for_interrupts(cpumask_t *mask) >>> { >>> - int missing = 0, extra = 0, unexpected = 0; >>> int nr_pass, cpu, i; >>> - bool bad = false; >>> >>> /* Wait up to 5s for all interrupts to be delivered */ >>> - for (i = 0; i < 50; ++i) { >>> + for (i = 0; i < 50; i++) { >>> mdelay(100); >>> nr_pass = 0; >>> for_each_present_cpu(cpu) { >>> + /* >>> + * A CPU having receied more than one interrupts will >> received >>> + * show up in check_acked(), and no matter how long we >>> + * wait it cannot un-receive it. Consier at least one >> consider > > Will fix all three typos, thanks. > >>> + * interrupt as a pass. >>> + */ >>> nr_pass += cpumask_test_cpu(cpu, mask) ? >>> - acked[cpu] == 1 : acked[cpu] == 0; >>> - smp_rmb(); /* pairs with smp_wmb in ipi_handler */ >>> - >>> - if (bad_sender[cpu] != -1) { >>> - printf("cpu%d received IPI from wrong sender %d\n", >>> - cpu, bad_sender[cpu]); >>> - bad = true; >>> - } >>> - >>> - if (bad_irq[cpu] != -1) { >>> - printf("cpu%d received wrong irq %d\n", >>> - cpu, bad_irq[cpu]); >>> - bad = true; >>> - } >>> + acked[cpu] >= 1 : acked[cpu] == 0; >>> } >>> + >>> if (nr_pass == nr_cpus) { >>> - report(!bad, "%s", testname); >>> if (i) >>> - report_info("took more than %d ms", i * 100); >>> + report_info("interrupts took more than %d ms", i * 100); >>> + mdelay(100); >>> return; >>> } >>> } >>> >>> + report_info("interrupts timed-out (5s)"); >>> +} >>> + >>> +static bool check_acked(cpumask_t *mask) >>> +{ >>> + int missing = 0, extra = 0, unexpected = 0; >>> + bool pass = true; >>> + int cpu; >>> + >>> for_each_present_cpu(cpu) { >>> if (cpumask_test_cpu(cpu, mask)) { >>> if (!acked[cpu]) >>> @@ -107,11 +108,28 @@ static void check_acked(const char *testname, cpumask_t *mask) >>> if (acked[cpu]) >>> ++unexpected; >>> } >>> + smp_rmb(); /* pairs with smp_wmb in ipi_handler */ >>> + >>> + if (bad_sender[cpu] != -1) { >>> + report_info("cpu%d received IPI from wrong sender %d", >>> + cpu, bad_sender[cpu]); >>> + pass = false; >>> + } >>> + >>> + if (bad_irq[cpu] != -1) { >>> + report_info("cpu%d received wrong irq %d", >>> + cpu, bad_irq[cpu]); >>> + pass = false; >>> + } >>> + } >>> + >>> + if (missing || extra || unexpected) { >>> + report_info("ACKS: missing=%d extra=%d unexpected=%d", >>> + missing, extra, unexpected); >>> + pass = false; >>> } >>> >>> - report(false, "%s", testname); >>> - report_info("Timed-out (5s). ACKS: missing=%d extra=%d unexpected=%d", >>> - missing, extra, unexpected); >>> + return pass; >>> } >>> >>> static void check_spurious(void) >>> @@ -300,7 +318,8 @@ static void ipi_test_self(void) >>> cpumask_clear(&mask); >>> cpumask_set_cpu(smp_processor_id(), &mask); >>> gic->ipi.send_self(); >>> - check_acked("IPI: self", &mask); >>> + wait_for_interrupts(&mask); >>> + report(check_acked(&mask), "Interrupts received"); >>> report_prefix_pop(); >>> } >>> >>> @@ -315,7 +334,8 @@ static void ipi_test_smp(void) >>> for (i = smp_processor_id() & 1; i < nr_cpus; i += 2) >>> cpumask_clear_cpu(i, &mask); >>> gic_ipi_send_mask(IPI_IRQ, &mask); >>> - check_acked("IPI: directed", &mask); >>> + wait_for_interrupts(&mask); >>> + report(check_acked(&mask), "Interrupts received"); >> both ipi_test_smp and ipi_test_self are called from the same test so >> better to use different error messages like it was done originally. > > I used the same error message because the tests have a different prefix > ("target-list" versus "broadcast"). Do you think there are cases where that's not > enough? I think in "ipi" test, ipi_test -> ipi_send -> ipi_test_self, ipi_test_smp Thanks Eric > > Thanks, > Alex >
Hi Eric, On 12/15/20 1:58 PM, Auger Eric wrote: > Hi Alexandru, > > On 12/10/20 3:45 PM, Alexandru Elisei wrote: >> Hi Eric, >> >> On 12/3/20 1:39 PM, Auger Eric wrote: >>> [..] >>> >>> static void check_spurious(void) >>> @@ -300,7 +318,8 @@ static void ipi_test_self(void) >>> cpumask_clear(&mask); >>> cpumask_set_cpu(smp_processor_id(), &mask); >>> gic->ipi.send_self(); >>> - check_acked("IPI: self", &mask); >>> + wait_for_interrupts(&mask); >>> + report(check_acked(&mask), "Interrupts received"); >>> report_prefix_pop(); >>> } >>> >>> @@ -315,7 +334,8 @@ static void ipi_test_smp(void) >>> for (i = smp_processor_id() & 1; i < nr_cpus; i += 2) >>> cpumask_clear_cpu(i, &mask); >>> gic_ipi_send_mask(IPI_IRQ, &mask); >>> - check_acked("IPI: directed", &mask); >>> + wait_for_interrupts(&mask); >>> + report(check_acked(&mask), "Interrupts received"); >>> both ipi_test_smp and ipi_test_self are called from the same test so >>> better to use different error messages like it was done originally. >> I used the same error message because the tests have a different prefix >> ("target-list" versus "broadcast"). Do you think there are cases where that's not >> enough? > I think in "ipi" test, > ipi_test -> ipi_send -> ipi_test_self, ipi_test_smp I'm afraid I don't understand what you are trying to say. This is the log for the gicv3-ipi test: $ cat logs/gicv3-ipi.log timeout -k 1s --foreground 90s /usr/bin/qemu-system-aarch64 -nodefaults -machine virt,gic-version=host,accel=kvm -cpu host -device virtio-serial-device -device virtconsole,chardev=ctd -chardev testdev,id=ctd -device pci-testdev -display none -serial stdio -kernel arm/gic.flat -smp 6 -machine gic-version=3 -append ipi # -initrd /tmp/tmp.trk6aAcaZx WARNING: early print support may not work. Found uart at 0x9000000, but early base is 0x3f8. PASS: gicv3: ipi: self: Interrupts received PASS: gicv3: ipi: target-list: Interrupts received PASS: gicv3: ipi: broadcast: Interrupts received SUMMARY: 3 tests The warning is because I forgot to reconfigure the tests with --vmm=qemu. Would you mind pointing out what you think is ambiguous? Thanks, Alex
Hi Alexandru, On 12/16/20 12:40 PM, Alexandru Elisei wrote: > Hi Eric, > > On 12/15/20 1:58 PM, Auger Eric wrote: >> Hi Alexandru, >> >> On 12/10/20 3:45 PM, Alexandru Elisei wrote: >>> Hi Eric, >>> >>> On 12/3/20 1:39 PM, Auger Eric wrote: >>>> [..] >>>> >>>> static void check_spurious(void) >>>> @@ -300,7 +318,8 @@ static void ipi_test_self(void) >>>> cpumask_clear(&mask); >>>> cpumask_set_cpu(smp_processor_id(), &mask); >>>> gic->ipi.send_self(); >>>> - check_acked("IPI: self", &mask); >>>> + wait_for_interrupts(&mask); >>>> + report(check_acked(&mask), "Interrupts received"); >>>> report_prefix_pop(); >>>> } >>>> >>>> @@ -315,7 +334,8 @@ static void ipi_test_smp(void) >>>> for (i = smp_processor_id() & 1; i < nr_cpus; i += 2) >>>> cpumask_clear_cpu(i, &mask); >>>> gic_ipi_send_mask(IPI_IRQ, &mask); >>>> - check_acked("IPI: directed", &mask); >>>> + wait_for_interrupts(&mask); >>>> + report(check_acked(&mask), "Interrupts received"); >>>> both ipi_test_smp and ipi_test_self are called from the same test so >>>> better to use different error messages like it was done originally. >>> I used the same error message because the tests have a different prefix >>> ("target-list" versus "broadcast"). Do you think there are cases where that's not >>> enough? >> I think in "ipi" test, >> ipi_test -> ipi_send -> ipi_test_self, ipi_test_smp > > I'm afraid I don't understand what you are trying to say. This is the log for the > gicv3-ipi test: > > $ cat logs/gicv3-ipi.log > timeout -k 1s --foreground 90s /usr/bin/qemu-system-aarch64 -nodefaults -machine > virt,gic-version=host,accel=kvm -cpu host -device virtio-serial-device -device > virtconsole,chardev=ctd -chardev testdev,id=ctd -device pci-testdev -display none > -serial stdio -kernel arm/gic.flat -smp 6 -machine gic-version=3 -append ipi # > -initrd /tmp/tmp.trk6aAcaZx > WARNING: early print support may not work. Found uart at 0x9000000, but early base > is 0x3f8. > PASS: gicv3: ipi: self: Interrupts received > PASS: gicv3: ipi: target-list: Interrupts received > PASS: gicv3: ipi: broadcast: Interrupts received > SUMMARY: 3 tests > > The warning is because I forgot to reconfigure the tests with --vmm=qemu. > > Would you mind pointing out what you think is ambiguous? Hum sorry I did not pay attention to the report_prefix_push() within ipi_test_self & ipi_test_smp. I had in mind those were only in the main(). Forgive me for the noise Eric > > Thanks, > > Alex >
diff --git a/arm/gic.c b/arm/gic.c index 544c283f5f47..dcdab7d5f39a 100644 --- a/arm/gic.c +++ b/arm/gic.c @@ -62,41 +62,42 @@ static void stats_reset(void) } } -static void check_acked(const char *testname, cpumask_t *mask) +static void wait_for_interrupts(cpumask_t *mask) { - int missing = 0, extra = 0, unexpected = 0; int nr_pass, cpu, i; - bool bad = false; /* Wait up to 5s for all interrupts to be delivered */ - for (i = 0; i < 50; ++i) { + for (i = 0; i < 50; i++) { mdelay(100); nr_pass = 0; for_each_present_cpu(cpu) { + /* + * A CPU having receied more than one interrupts will + * show up in check_acked(), and no matter how long we + * wait it cannot un-receive it. Consier at least one + * interrupt as a pass. + */ nr_pass += cpumask_test_cpu(cpu, mask) ? - acked[cpu] == 1 : acked[cpu] == 0; - smp_rmb(); /* pairs with smp_wmb in ipi_handler */ - - if (bad_sender[cpu] != -1) { - printf("cpu%d received IPI from wrong sender %d\n", - cpu, bad_sender[cpu]); - bad = true; - } - - if (bad_irq[cpu] != -1) { - printf("cpu%d received wrong irq %d\n", - cpu, bad_irq[cpu]); - bad = true; - } + acked[cpu] >= 1 : acked[cpu] == 0; } + if (nr_pass == nr_cpus) { - report(!bad, "%s", testname); if (i) - report_info("took more than %d ms", i * 100); + report_info("interrupts took more than %d ms", i * 100); + mdelay(100); return; } } + report_info("interrupts timed-out (5s)"); +} + +static bool check_acked(cpumask_t *mask) +{ + int missing = 0, extra = 0, unexpected = 0; + bool pass = true; + int cpu; + for_each_present_cpu(cpu) { if (cpumask_test_cpu(cpu, mask)) { if (!acked[cpu]) @@ -107,11 +108,28 @@ static void check_acked(const char *testname, cpumask_t *mask) if (acked[cpu]) ++unexpected; } + smp_rmb(); /* pairs with smp_wmb in ipi_handler */ + + if (bad_sender[cpu] != -1) { + report_info("cpu%d received IPI from wrong sender %d", + cpu, bad_sender[cpu]); + pass = false; + } + + if (bad_irq[cpu] != -1) { + report_info("cpu%d received wrong irq %d", + cpu, bad_irq[cpu]); + pass = false; + } + } + + if (missing || extra || unexpected) { + report_info("ACKS: missing=%d extra=%d unexpected=%d", + missing, extra, unexpected); + pass = false; } - report(false, "%s", testname); - report_info("Timed-out (5s). ACKS: missing=%d extra=%d unexpected=%d", - missing, extra, unexpected); + return pass; } static void check_spurious(void) @@ -300,7 +318,8 @@ static void ipi_test_self(void) cpumask_clear(&mask); cpumask_set_cpu(smp_processor_id(), &mask); gic->ipi.send_self(); - check_acked("IPI: self", &mask); + wait_for_interrupts(&mask); + report(check_acked(&mask), "Interrupts received"); report_prefix_pop(); } @@ -315,7 +334,8 @@ static void ipi_test_smp(void) for (i = smp_processor_id() & 1; i < nr_cpus; i += 2) cpumask_clear_cpu(i, &mask); gic_ipi_send_mask(IPI_IRQ, &mask); - check_acked("IPI: directed", &mask); + wait_for_interrupts(&mask); + report(check_acked(&mask), "Interrupts received"); report_prefix_pop(); report_prefix_push("broadcast"); @@ -323,7 +343,8 @@ static void ipi_test_smp(void) cpumask_copy(&mask, &cpu_present_mask); cpumask_clear_cpu(smp_processor_id(), &mask); gic->ipi.send_broadcast(); - check_acked("IPI: broadcast", &mask); + wait_for_interrupts(&mask); + report(check_acked(&mask), "Interrupts received"); report_prefix_pop(); }
check_acked() has several peculiarities: is the only function among the check_* functions which calls report() directly, it does two things (waits for interrupts and checks for misfired interrupts) and it also mixes printf, report_info and report calls. check_acked() also reports a pass and returns as soon all the target CPUs have received interrupts, However, a CPU not having received an interrupt *now* does not guarantee not receiving an eroneous interrupt if we wait long enough. Rework the function by splitting it into two separate functions, each with a single responsability: wait_for_interrupts(), which waits for the expected interrupts to fire, and check_acked() which checks that interrupts have been received as expected. wait_for_interrupts() also waits an extra 100 milliseconds after the expected interrupts have been received in an effort to make sure we don't miss misfiring interrupts. Splitting check_acked() into two functions will also allow us to customize the behavior of each function in the future more easily without using an unnecessarily long list of arguments for check_acked(). CC: Andre Przywara <andre.przywara@arm.com> Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com> --- arm/gic.c | 73 +++++++++++++++++++++++++++++++++++-------------------- 1 file changed, 47 insertions(+), 26 deletions(-)