From: Alexandru Elisei <alexandru.elisei@arm.com> To: Andre Przywara <andre.przywara@arm.com>, Andrew Jones <drjones@redhat.com>, Paolo Bonzini <pbonzini@redhat.com> Cc: Marc Zyngier <maz@kernel.org>, kvmarm@lists.cs.columbia.edu, linux-arm-kernel@lists.infradead.org, kvm@vger.kernel.org Subject: Re: [kvm-unit-tests PATCH 07/17] arm: gic: Extend check_acked() to allow silent call Date: Tue, 12 Nov 2019 15:23:04 +0000 Message-ID: <25598849-b195-3411-8092-b0656bcfb762@arm.com> (raw) In-Reply-To: <20191108144240.204202-8-andre.przywara@arm.com> 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) _______________________________________________ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
next prev parent reply index Thread overview: 51+ messages / expand[flat|nested] mbox.gz Atom feed top 2019-11-08 14:42 [kvm-unit-tests PATCH 00/17] arm: gic: Test SPIs and interrupt groups Andre Przywara 2019-11-08 14:42 ` [kvm-unit-tests PATCH 01/17] arm: gic: Enable GIC MMIO tests for GICv3 as well Andre Przywara 2019-11-08 17:28 ` Alexandru Elisei 2019-11-12 12:49 ` Auger Eric 2019-11-08 14:42 ` [kvm-unit-tests PATCH 02/17] arm: gic: Generalise function names Andre Przywara 2019-11-12 11:11 ` Alexandru Elisei 2019-11-12 12:49 ` Auger Eric 2019-11-08 14:42 ` [kvm-unit-tests PATCH 03/17] arm: gic: Provide per-IRQ helper functions Andre Przywara 2019-11-12 12:51 ` Alexandru Elisei 2019-11-12 15:53 ` Auger Eric 2019-11-12 16:53 ` Alexandru Elisei 2019-11-12 13:49 ` Auger Eric 2019-11-08 14:42 ` [kvm-unit-tests PATCH 04/17] arm: gic: Support no IRQs test case Andre Przywara 2019-11-12 13:26 ` Alexandru Elisei 2019-11-12 21:14 ` Auger Eric 2019-11-08 14:42 ` [kvm-unit-tests PATCH 05/17] arm: gic: Prepare IRQ handler for handling SPIs Andre Przywara 2019-11-12 13:36 ` Alexandru Elisei 2019-11-12 20:56 ` Auger Eric 2019-11-08 14:42 ` [kvm-unit-tests PATCH 06/17] arm: gic: Add simple shared IRQ test Andre Przywara 2019-11-12 13:54 ` Alexandru Elisei 2019-11-08 14:42 ` [kvm-unit-tests PATCH 07/17] arm: gic: Extend check_acked() to allow silent call Andre Przywara 2019-11-12 15:23 ` Alexandru Elisei [this message] 2019-11-14 12:32 ` Andrew Jones 2019-11-15 11:32 ` Alexandru Elisei 2019-11-08 14:42 ` [kvm-unit-tests PATCH 08/17] arm: gic: Add simple SPI MP test Andre Przywara 2019-11-12 15:41 ` Alexandru Elisei 2019-11-08 14:42 ` [kvm-unit-tests PATCH 09/17] arm: gic: Add test for flipping GICD_CTLR.DS Andre Przywara 2019-11-12 16:42 ` Alexandru Elisei 2019-11-14 13:39 ` Vladimir Murzin 2019-11-14 14:17 ` Andre Przywara 2019-11-14 14:50 ` Vladimir Murzin 2019-11-14 15:21 ` Alexandru Elisei 2019-11-14 15:27 ` Peter Maydell 2019-11-14 15:47 ` Alexandru Elisei 2019-11-14 15:56 ` Peter Maydell 2019-11-08 14:42 ` [kvm-unit-tests PATCH 10/17] arm: gic: Check for writable IGROUPR registers Andre Przywara 2019-11-12 16:51 ` Alexandru Elisei 2019-11-08 14:42 ` [kvm-unit-tests PATCH 11/17] arm: gic: Check for validity of both group enable bits Andre Przywara 2019-11-12 16:58 ` Alexandru Elisei 2019-11-08 14:42 ` [kvm-unit-tests PATCH 12/17] arm: gic: Change gic_read_iar() to take group parameter Andre Przywara 2019-11-12 17:19 ` Alexandru Elisei 2019-11-14 12:50 ` Andrew Jones 2019-11-08 14:42 ` [kvm-unit-tests PATCH 13/17] arm: gic: Change write_eoir() " Andre Przywara 2019-11-08 14:42 ` [kvm-unit-tests PATCH 14/17] arm: gic: Prepare for receiving GIC group 0 interrupts via FIQs Andre Przywara 2019-11-12 17:30 ` Alexandru Elisei 2019-11-08 14:42 ` [kvm-unit-tests PATCH 15/17] arm: gic: Provide FIQ handler Andre Przywara 2019-11-13 10:14 ` Alexandru Elisei 2019-11-08 14:42 ` [kvm-unit-tests PATCH 16/17] arm: gic: Prepare interrupt statistics for both groups Andre Przywara 2019-11-13 10:44 ` Alexandru Elisei 2019-11-08 14:42 ` [kvm-unit-tests PATCH 17/17] arm: gic: Test Group0 SPIs Andre Przywara 2019-11-13 11:26 ` Alexandru Elisei
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=25598849-b195-3411-8092-b0656bcfb762@arm.com \ --to=alexandru.elisei@arm.com \ --cc=andre.przywara@arm.com \ --cc=drjones@redhat.com \ --cc=kvm@vger.kernel.org \ --cc=kvmarm@lists.cs.columbia.edu \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=maz@kernel.org \ --cc=pbonzini@redhat.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: link
KVM ARM Archive on lore.kernel.org Archives are clonable: git clone --mirror https://lore.kernel.org/kvmarm/0 kvmarm/git/0.git # If you have public-inbox 1.1+ installed, you may # initialize and index your mirror using the following commands: public-inbox-init -V2 kvmarm kvmarm/ https://lore.kernel.org/kvmarm \ kvmarm@lists.cs.columbia.edu public-inbox-index kvmarm Example config snippet for mirrors Newsgroup available over NNTP: nntp://nntp.lore.kernel.org/edu.columbia.cs.lists.kvmarm AGPL code for this site: git clone https://public-inbox.org/public-inbox.git