| Message ID | 20191108144240.204202-5-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 some tests it would be important to check that an IRQ was *not* > triggered, for instance to test certain masking operations. > > Extend the check_added() function to recognise an empty cpumask to > detect this situation. The timeout duration is reduced, and the "no IRQs Why is the timeout duration reduced? > triggered" case is actually reported as a success in this case. > > Signed-off-by: Andre Przywara <andre.przywara@arm.com> > --- > arm/gic.c | 10 ++++++++-- > 1 file changed, 8 insertions(+), 2 deletions(-) > > diff --git a/arm/gic.c b/arm/gic.c > index a114009..eca9188 100644 > --- a/arm/gic.c > +++ b/arm/gic.c > @@ -66,9 +66,10 @@ static void check_acked(const char *testname, cpumask_t *mask) > int missing = 0, extra = 0, unexpected = 0; > int nr_pass, cpu, i; > bool bad = false; > + bool noirqs = cpumask_empty(mask); > > /* Wait up to 5s for all interrupts to be delivered */ This comment needs updating. > - for (i = 0; i < 50; ++i) { > + for (i = 0; i < (noirqs ? 15 : 50); ++i) { > mdelay(100); > nr_pass = 0; > for_each_present_cpu(cpu) { > @@ -88,7 +89,7 @@ static void check_acked(const char *testname, cpumask_t *mask) > bad = true; > } > } > - if (nr_pass == nr_cpus) { > + if (!noirqs && nr_pass == nr_cpus) { This condition is pretty hard to read - what you are doing here is making sure that when check_acked tests that no irqs have been received, you do the entire for loop and wait the entire timeout duration. Did I get that right? How about this (compile tested only): + if (noirqs) + /* Wait for the entire timeout duration. */ + continue; + if (nr_pass == nr_cpus) { report("%s", !bad, testname); if (i) > report("%s", !bad, testname); > if (i) > report_info("took more than %d ms", i * 100); > @@ -96,6 +97,11 @@ static void check_acked(const char *testname, cpumask_t *mask) > } > } > > + if (noirqs && nr_pass == nr_cpus) { > + report("%s", !bad, testname); bad is true only when bad_sender[cpu] != -1 or bad_irq[cpu] != -1, which only get set in the irq or ipi handlesr, meaning when you do get an interrupt. If nr_pass == nr_cpus and noirqs, then you shouldn't have gotten an interrupt. I think it's safe to write it as report("%s", true, testname). I think a short comment above explaining why we do this check (timeout expired and we haven't gotten any interrupts) would also improve readability of the code, but that's up to you. Thanks, Alex > + return; > + } > + > for_each_present_cpu(cpu) { > if (cpumask_test_cpu(cpu, mask)) { > if (!acked[cpu])
Hi, On 11/12/19 2:26 PM, Alexandru Elisei wrote: > Hi, > > On 11/8/19 2:42 PM, Andre Przywara wrote: >> For some tests it would be important to check that an IRQ was *not* >> triggered, for instance to test certain masking operations. >> >> Extend the check_added() function to recognise an empty cpumask to >> detect this situation. The timeout duration is reduced, and the "no IRQs > > Why is the timeout duration reduced? > >> triggered" case is actually reported as a success in this case. >> >> Signed-off-by: Andre Przywara <andre.przywara@arm.com> >> --- >> arm/gic.c | 10 ++++++++-- >> 1 file changed, 8 insertions(+), 2 deletions(-) >> >> diff --git a/arm/gic.c b/arm/gic.c >> index a114009..eca9188 100644 >> --- a/arm/gic.c >> +++ b/arm/gic.c >> @@ -66,9 +66,10 @@ static void check_acked(const char *testname, cpumask_t *mask) >> int missing = 0, extra = 0, unexpected = 0; >> int nr_pass, cpu, i; >> bool bad = false; >> + bool noirqs = cpumask_empty(mask); >> >> /* Wait up to 5s for all interrupts to be delivered */ > > This comment needs updating. > >> - for (i = 0; i < 50; ++i) { >> + for (i = 0; i < (noirqs ? 15 : 50); ++i) { >> mdelay(100); >> nr_pass = 0; >> for_each_present_cpu(cpu) { >> @@ -88,7 +89,7 @@ static void check_acked(const char *testname, cpumask_t *mask) >> bad = true; >> } >> } >> - if (nr_pass == nr_cpus) { >> + if (!noirqs && nr_pass == nr_cpus) { > > This condition is pretty hard to read - what you are doing here is making sure > that when check_acked tests that no irqs have been received, you do the entire for > loop and wait the entire timeout duration. Did I get that right? > > How about this (compile tested only): > > + if (noirqs) > + /* Wait for the entire timeout duration. */ > + continue; > + > if (nr_pass == nr_cpus) { > report("%s", !bad, testname); > if (i) > >> report("%s", !bad, testname); >> if (i>> report_info("took more than %d ms", i * 100); >> @@ -96,6 +97,11 @@ static void check_acked(const char *testname, cpumask_t *mask) >> } >> } >> >> + if (noirqs && nr_pass == nr_cpus) { >> + report("%s", !bad, testname); This one looks at the result of the last iteration (on timeout). In case of noirqs I think we should be able to return a failure as soon as an irq is detected where we do not expect it, without waiting for the full delay? Thanks Eric > > bad is true only when bad_sender[cpu] != -1 or bad_irq[cpu] != -1, which only get > set in the irq or ipi handlesr, meaning when you do get an interrupt. If nr_pass > == nr_cpus and noirqs, then you shouldn't have gotten an interrupt. I think it's > safe to write it as report("%s", true, testname). I think a short comment above > explaining why we do this check (timeout expired and we haven't gotten any > interrupts) would also improve readability of the code, but that's up to you. > > Thanks, > Alex >> + return; >> + } >> + >> for_each_present_cpu(cpu) { >> if (cpumask_test_cpu(cpu, mask)) { >> if (!acked[cpu]) > _______________________________________________ > kvmarm mailing list > kvmarm@lists.cs.columbia.edu > https://lists.cs.columbia.edu/mailman/listinfo/kvmarm >
diff --git a/arm/gic.c b/arm/gic.c index a114009..eca9188 100644 --- a/arm/gic.c +++ b/arm/gic.c @@ -66,9 +66,10 @@ static void check_acked(const char *testname, cpumask_t *mask) int missing = 0, extra = 0, unexpected = 0; int nr_pass, cpu, i; bool bad = false; + bool noirqs = cpumask_empty(mask); /* Wait up to 5s for all interrupts to be delivered */ - for (i = 0; i < 50; ++i) { + for (i = 0; i < (noirqs ? 15 : 50); ++i) { mdelay(100); nr_pass = 0; for_each_present_cpu(cpu) { @@ -88,7 +89,7 @@ static void check_acked(const char *testname, cpumask_t *mask) bad = true; } } - if (nr_pass == nr_cpus) { + if (!noirqs && nr_pass == nr_cpus) { report("%s", !bad, testname); if (i) report_info("took more than %d ms", i * 100); @@ -96,6 +97,11 @@ static void check_acked(const char *testname, cpumask_t *mask) } } + if (noirqs && nr_pass == nr_cpus) { + report("%s", !bad, testname); + return; + } + for_each_present_cpu(cpu) { if (cpumask_test_cpu(cpu, mask)) { if (!acked[cpu])
For some tests it would be important to check that an IRQ was *not* triggered, for instance to test certain masking operations. Extend the check_added() function to recognise an empty cpumask to detect this situation. The timeout duration is reduced, and the "no IRQs triggered" case is actually reported as a success in this case. Signed-off-by: Andre Przywara <andre.przywara@arm.com> --- arm/gic.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-)