[kvm-unit-tests,04/17] arm: gic: Support no IRQs test case
diff mbox series

Message ID 20191108144240.204202-5-andre.przywara@arm.com
State New
Headers show
Series
  • arm: gic: Test SPIs and interrupt groups
Related show

Commit Message

Andre Przywara Nov. 8, 2019, 2:42 p.m. UTC
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(-)

Comments

Alexandru Elisei Nov. 12, 2019, 1:26 p.m. UTC | #1
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])
Auger Eric Nov. 12, 2019, 9:14 p.m. UTC | #2
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
>

Patch
diff mbox series

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])