| Message ID | 20191108144240.204202-8-andre.przywara@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | arm: gic: Test SPIs and interrupt groups | expand |
Hi, On 11/8/19 2:42 PM, Andre Przywara wrote: > For future tests we will need to call check_acked() twice for the same > interrupt (to test delivery of Group 0 and Group 1 interrupts). > This should be reported as a single test, so allow check_acked() to be > called with a "NULL" test name, to suppress output. We report the test > result via the return value, so the outcome is not lost. > > Also this amends the new trigger_and_check_spi() wrapper, to propagate > the test result to callers of that function. > > Signed-off-by: Andre Przywara <andre.przywara@arm.com> > --- > arm/gic.c | 29 +++++++++++++++++++---------- > 1 file changed, 19 insertions(+), 10 deletions(-) > > diff --git a/arm/gic.c b/arm/gic.c > index 3be76cb..63aa9f4 100644 > --- a/arm/gic.c > +++ b/arm/gic.c > @@ -62,7 +62,7 @@ static void stats_reset(void) > smp_wmb(); > } > > -static void check_acked(const char *testname, cpumask_t *mask) > +static int check_acked(const char *testname, cpumask_t *mask) > { > int missing = 0, extra = 0, unexpected = 0; > int nr_pass, cpu, i; > @@ -91,16 +91,20 @@ static void check_acked(const char *testname, cpumask_t *mask) > } > } > if (!noirqs && nr_pass == nr_cpus) { > - report("%s", !bad, testname); > - if (i) > - report_info("took more than %d ms", i * 100); > - return; > + if (testname) { > + report("%s", !bad, testname); > + if (i) > + report_info("took more than %d ms", > + i * 100); > + } > + return i * 100; > } > } > > if (noirqs && nr_pass == nr_cpus) { > - report("%s", !bad, testname); > - return; > + if (testname) > + report("%s", !bad, testname); > + return i * 100; > } > > for_each_present_cpu(cpu) { > @@ -115,9 +119,11 @@ static void check_acked(const char *testname, cpumask_t *mask) > } > } > > - report("%s", false, testname); > + if (testname) > + report("%s", false, testname); > report_info("Timed-out (5s). ACKS: missing=%d extra=%d unexpected=%d", > missing, extra, unexpected); > + return -1; > } check_acked is starting to become hard to read. The function itself is rather inconsistent, as it mixes regular printf's with report_info's. The return value is also never used: $ awk '/check_acked\(/ && !/const/' arm/gic.c check_acked("IPI: self", &mask); check_acked("IPI: directed", &mask); check_acked("IPI: broadcast", &mask); What I'm thinking is that we can rewrite check_acked to return true/false (or 0/1), meaning success or failure, remove the testname parameter, replace the printfs to report_info, and have the caller do a report based on the value returned by check_acked. Rough version, compile tested only, I'm sure it can be improved: diff --git a/arm/gic.c b/arm/gic.c index adb6aa464513..5453f2fd3d5f 100644 --- a/arm/gic.c +++ b/arm/gic.c @@ -60,11 +60,11 @@ static void stats_reset(void) smp_wmb(); } -static void check_acked(const char *testname, cpumask_t *mask) +static bool check_acked(cpumask_t *mask) { int missing = 0, extra = 0, unexpected = 0; int nr_pass, cpu, i; - bool bad = false; + bool success = true; /* Wait up to 5s for all interrupts to be delivered */ for (i = 0; i < 50; ++i) { @@ -76,22 +76,21 @@ static void check_acked(const char *testname, cpumask_t *mask) acked[cpu] == 1 : acked[cpu] == 0; if (bad_sender[cpu] != -1) { - printf("cpu%d received IPI from wrong sender %d\n", + report_info("cpu%d received IPI from wrong sender %d\n", cpu, bad_sender[cpu]); - bad = true; + success = false; } if (bad_irq[cpu] != -1) { - printf("cpu%d received wrong irq %d\n", + report_info("cpu%d received wrong irq %d\n", cpu, bad_irq[cpu]); - bad = true; + success = false; } } if (nr_pass == nr_cpus) { - report("%s", !bad, testname); if (i) report_info("took more than %d ms", i * 100); - return; + return success; } } @@ -107,9 +106,9 @@ static void check_acked(const char *testname, cpumask_t *mask) } } - report("%s", false, testname); report_info("Timed-out (5s). ACKS: missing=%d extra=%d unexpected=%d", missing, extra, unexpected); + return false; } static void check_spurious(void) @@ -183,13 +182,11 @@ static void ipi_test_self(void) { cpumask_t mask; - report_prefix_push("self"); stats_reset(); cpumask_clear(&mask); cpumask_set_cpu(smp_processor_id(), &mask); gic->ipi.send_self(); - check_acked("IPI: self", &mask); - report_prefix_pop(); + report("self", check_acked(&mask)); } static void ipi_test_smp(void) @@ -203,7 +200,7 @@ 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); + report("directed", check_acked(&mask)); report_prefix_pop(); report_prefix_push("broadcast"); @@ -211,7 +208,7 @@ 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); + report("broadcast", check_acked(&mask)); report_prefix_pop(); } I've removed "IPI" from the report string because the prefixed was already pushed in main. Andrew, what do you think? Are we missing something obvious? Do you have a better idea? > static void check_spurious(void) > @@ -567,11 +573,12 @@ static void spi_configure_irq(int irq, int cpu) > * Wait for an SPI to fire (or not) on a certain CPU. > * Clears the pending bit if requested afterwards. > */ > -static void trigger_and_check_spi(const char *test_name, > +static bool trigger_and_check_spi(const char *test_name, > unsigned int irq_stat, > int cpu) Why did you change the return value from void to bool if you're not using it anywhere? If it's because you need it in the next patch (#8), please make the change there. Thanks, Alex > { > cpumask_t cpumask; > + bool ret = true; > > stats_reset(); > gic_spi_trigger(SPI_IRQ); > @@ -584,11 +591,13 @@ static void trigger_and_check_spi(const char *test_name, > break; > } > > - check_acked(test_name, &cpumask); > + ret = (check_acked(test_name, &cpumask) >= 0); > > /* Clean up pending bit in case this IRQ wasn't taken. */ > if (!(irq_stat & IRQ_STAT_NO_CLEAR)) > gic_set_irq_bit(SPI_IRQ, GICD_ICPENDR); > + > + return ret; > } > > static void spi_test_single(void)
On Tue, Nov 12, 2019 at 03:23:04PM +0000, Alexandru Elisei wrote: > check_acked is starting to become hard to read. Agreed. check_acked() could probably have some of its subtests factored out to improve its readability. > The function itself is rather inconsistent, as it mixes regular > printf's with report_info's. Sounds good > The return value is also never used: > > $ awk '/check_acked\(/ && !/const/' arm/gic.c > check_acked("IPI: self", &mask); > check_acked("IPI: directed", &mask); > check_acked("IPI: broadcast", &mask); That's good, since it's a void function :-) > > What I'm thinking is that we can rewrite check_acked to return true/false (or > 0/1), meaning success or failure, remove the testname parameter, replace the > printfs to report_info, and have the caller do a report based on the value > returned by check_acked. > > Rough version, compile tested only, I'm sure it can be improved: > > diff --git a/arm/gic.c b/arm/gic.c > index adb6aa464513..5453f2fd3d5f 100644 > --- a/arm/gic.c > +++ b/arm/gic.c > @@ -60,11 +60,11 @@ static void stats_reset(void) > smp_wmb(); > } > > -static void check_acked(const char *testname, cpumask_t *mask) > +static bool check_acked(cpumask_t *mask) We have several check_* functions in arm/gic.c, and they're all void functions. Changing this one to a bool would be inconsistent, but maybe that consistency isn't that important, or maybe they should all be bool? > { > int missing = 0, extra = 0, unexpected = 0; > int nr_pass, cpu, i; > - bool bad = false; > + bool success = true; > > /* Wait up to 5s for all interrupts to be delivered */ > for (i = 0; i < 50; ++i) { > @@ -76,22 +76,21 @@ static void check_acked(const char *testname, cpumask_t *mask) > acked[cpu] == 1 : acked[cpu] == 0; > > if (bad_sender[cpu] != -1) { > - printf("cpu%d received IPI from wrong sender %d\n", > + report_info("cpu%d received IPI from wrong sender > %d\n", > cpu, bad_sender[cpu]); > - bad = true; > + success = false; > } > > if (bad_irq[cpu] != -1) { > - printf("cpu%d received wrong irq %d\n", > + report_info("cpu%d received wrong irq %d\n", > cpu, bad_irq[cpu]); > - bad = true; > + success = false; > } > } > if (nr_pass == nr_cpus) { > - report("%s", !bad, testname); > if (i) > report_info("took more than %d ms", i * 100); > - return; > + return success; > } > } > > @@ -107,9 +106,9 @@ static void check_acked(const char *testname, cpumask_t *mask) > } > } > > - report("%s", false, testname); > report_info("Timed-out (5s). ACKS: missing=%d extra=%d unexpected=%d", > missing, extra, unexpected); > + return false; > } > > static void check_spurious(void) > @@ -183,13 +182,11 @@ static void ipi_test_self(void) > { > cpumask_t mask; > > - report_prefix_push("self"); > stats_reset(); > cpumask_clear(&mask); > cpumask_set_cpu(smp_processor_id(), &mask); > gic->ipi.send_self(); > - check_acked("IPI: self", &mask); > - report_prefix_pop(); > + report("self", check_acked(&mask)); > } > > static void ipi_test_smp(void) > @@ -203,7 +200,7 @@ 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); > + report("directed", check_acked(&mask)); > report_prefix_pop(); Shouldn't we also drop the "target-list" prefix push/pop? > > report_prefix_push("broadcast"); > @@ -211,7 +208,7 @@ 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); > + report("broadcast", check_acked(&mask)); > report_prefix_pop(); > } Shouldn't we also drop the "broadcast" prefix push/pop? > > I've removed "IPI" from the report string because the prefixed was already pushed > in main. > > Andrew, what do you think? Are we missing something obvious? Do you have a better > idea? I'm happy to see cleanups and haven't had a chance to look too closely at the gic tests in a while so I have no better ideas :-) Thanks, drew
Hi, On 11/14/19 12:32 PM, Andrew Jones wrote: > On Tue, Nov 12, 2019 at 03:23:04PM +0000, Alexandru Elisei wrote: >> check_acked is starting to become hard to read. > Agreed. check_acked() could probably have some of its subtests factored > out to improve its readability. > >> The function itself is rather inconsistent, as it mixes regular >> printf's with report_info's. > Sounds good > >> The return value is also never used: >> >> $ awk '/check_acked\(/ && !/const/' arm/gic.c >> check_acked("IPI: self", &mask); >> check_acked("IPI: directed", &mask); >> check_acked("IPI: broadcast", &mask); > That's good, since it's a void function :-) Sorry, got confused, this patch changes it to return a value, and that value is ignored in the existing functions (the ones I listed above), which would make the usage of check_acked very inconsistent. >> What I'm thinking is that we can rewrite check_acked to return true/false (or >> 0/1), meaning success or failure, remove the testname parameter, replace the >> printfs to report_info, and have the caller do a report based on the value >> returned by check_acked. >> >> Rough version, compile tested only, I'm sure it can be improved: >> >> diff --git a/arm/gic.c b/arm/gic.c >> index adb6aa464513..5453f2fd3d5f 100644 >> --- a/arm/gic.c >> +++ b/arm/gic.c >> @@ -60,11 +60,11 @@ static void stats_reset(void) >> smp_wmb(); >> } >> >> -static void check_acked(const char *testname, cpumask_t *mask) >> +static bool check_acked(cpumask_t *mask) > We have several check_* functions in arm/gic.c, and they're all void > functions. Changing this one to a bool would be inconsistent, but > maybe that consistency isn't that important, or maybe they should all > be bool? I think they should stay void, because they compute statistics, or print a warning if we got a spurious interrupt (check_spurious). I'm not really sure what to do about check_acked at the moment, I'll think about it some more. >> { >> int missing = 0, extra = 0, unexpected = 0; >> int nr_pass, cpu, i; >> - bool bad = false; >> + bool success = true; >> >> /* Wait up to 5s for all interrupts to be delivered */ >> for (i = 0; i < 50; ++i) { >> @@ -76,22 +76,21 @@ static void check_acked(const char *testname, cpumask_t *mask) >> acked[cpu] == 1 : acked[cpu] == 0; >> >> if (bad_sender[cpu] != -1) { >> - printf("cpu%d received IPI from wrong sender %d\n", >> + report_info("cpu%d received IPI from wrong sender >> %d\n", >> cpu, bad_sender[cpu]); >> - bad = true; >> + success = false; >> } >> >> if (bad_irq[cpu] != -1) { >> - printf("cpu%d received wrong irq %d\n", >> + report_info("cpu%d received wrong irq %d\n", >> cpu, bad_irq[cpu]); >> - bad = true; >> + success = false; >> } >> } >> if (nr_pass == nr_cpus) { >> - report("%s", !bad, testname); >> if (i) >> report_info("took more than %d ms", i * 100); >> - return; >> + return success; >> } >> } >> >> @@ -107,9 +106,9 @@ static void check_acked(const char *testname, cpumask_t *mask) >> } >> } >> >> - report("%s", false, testname); >> report_info("Timed-out (5s). ACKS: missing=%d extra=%d unexpected=%d", >> missing, extra, unexpected); >> + return false; >> } >> >> static void check_spurious(void) >> @@ -183,13 +182,11 @@ static void ipi_test_self(void) >> { >> cpumask_t mask; >> >> - report_prefix_push("self"); >> stats_reset(); >> cpumask_clear(&mask); >> cpumask_set_cpu(smp_processor_id(), &mask); >> gic->ipi.send_self(); >> - check_acked("IPI: self", &mask); >> - report_prefix_pop(); >> + report("self", check_acked(&mask)); >> } >> >> static void ipi_test_smp(void) >> @@ -203,7 +200,7 @@ 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); >> + report("directed", check_acked(&mask)); >> report_prefix_pop(); > Shouldn't we also drop the "target-list" prefix push/pop? > >> >> report_prefix_push("broadcast"); >> @@ -211,7 +208,7 @@ 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); >> + report("broadcast", check_acked(&mask)); >> report_prefix_pop(); >> } > Shouldn't we also drop the "broadcast" prefix push/pop? My suggestion was a quick hack to give an idea of how it might look, it can definitely be improved :) Thanks, Alex >> >> I've removed "IPI" from the report string because the prefixed was already pushed >> in main. >> >> Andrew, what do you think? Are we missing something obvious? Do you have a better >> idea? > I'm happy to see cleanups and haven't had a chance to look too closely at > the gic tests in a while so I have no better ideas :-) > > Thanks, > drew >
diff --git a/arm/gic.c b/arm/gic.c index 3be76cb..63aa9f4 100644 --- a/arm/gic.c +++ b/arm/gic.c @@ -62,7 +62,7 @@ static void stats_reset(void) smp_wmb(); } -static void check_acked(const char *testname, cpumask_t *mask) +static int check_acked(const char *testname, cpumask_t *mask) { int missing = 0, extra = 0, unexpected = 0; int nr_pass, cpu, i; @@ -91,16 +91,20 @@ static void check_acked(const char *testname, cpumask_t *mask) } } if (!noirqs && nr_pass == nr_cpus) { - report("%s", !bad, testname); - if (i) - report_info("took more than %d ms", i * 100); - return; + if (testname) { + report("%s", !bad, testname); + if (i) + report_info("took more than %d ms", + i * 100); + } + return i * 100; } } if (noirqs && nr_pass == nr_cpus) { - report("%s", !bad, testname); - return; + if (testname) + report("%s", !bad, testname); + return i * 100; } for_each_present_cpu(cpu) { @@ -115,9 +119,11 @@ static void check_acked(const char *testname, cpumask_t *mask) } } - report("%s", false, testname); + if (testname) + report("%s", false, testname); report_info("Timed-out (5s). ACKS: missing=%d extra=%d unexpected=%d", missing, extra, unexpected); + return -1; } static void check_spurious(void) @@ -567,11 +573,12 @@ static void spi_configure_irq(int irq, int cpu) * Wait for an SPI to fire (or not) on a certain CPU. * Clears the pending bit if requested afterwards. */ -static void trigger_and_check_spi(const char *test_name, +static bool trigger_and_check_spi(const char *test_name, unsigned int irq_stat, int cpu) { cpumask_t cpumask; + bool ret = true; stats_reset(); gic_spi_trigger(SPI_IRQ); @@ -584,11 +591,13 @@ static void trigger_and_check_spi(const char *test_name, break; } - check_acked(test_name, &cpumask); + ret = (check_acked(test_name, &cpumask) >= 0); /* Clean up pending bit in case this IRQ wasn't taken. */ if (!(irq_stat & IRQ_STAT_NO_CLEAR)) gic_set_irq_bit(SPI_IRQ, GICD_ICPENDR); + + return ret; } static void spi_test_single(void)
For future tests we will need to call check_acked() twice for the same interrupt (to test delivery of Group 0 and Group 1 interrupts). This should be reported as a single test, so allow check_acked() to be called with a "NULL" test name, to suppress output. We report the test result via the return value, so the outcome is not lost. Also this amends the new trigger_and_check_spi() wrapper, to propagate the test result to callers of that function. Signed-off-by: Andre Przywara <andre.przywara@arm.com> --- arm/gic.c | 29 +++++++++++++++++++---------- 1 file changed, 19 insertions(+), 10 deletions(-)