| Message ID | 20230615003832.161134-1-namit@vmware.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [kvm-unit-tests] arm64: timer: ignore ISTATUS with disabled timer | expand |
On Wed, Jun 14, 2023 at 05:38:32PM -0700, Nadav Amit wrote: > From: Nadav Amit <nadav.amit@gmail.com> > > According to ARM specifications for the vtimer (CNTV_CTL_EL0): "When the > value of the ENABLE bit is 0, the ISTATUS field is UNKNOWN." > > Currently the test, however, does check that ISTATUS is cleared when the > ENABLE bit is zero. Remove this check as the value is unknown. Hmm, does it? timer_pending() is /* Check that the timer condition is met. */ static bool timer_pending(struct timer_info *info) { return (info->read_ctl() & ARCH_TIMER_CTL_ENABLE) && (info->read_ctl() & ARCH_TIMER_CTL_ISTATUS); } which I would expect to short-circuit when ENABLE is zero. Thanks, drew > > Signed-off-by: Nadav Amit <nadav.amit@gmail.com> > --- > arm/timer.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/arm/timer.c b/arm/timer.c > index 2cb8051..0b976a7 100644 > --- a/arm/timer.c > +++ b/arm/timer.c > @@ -256,7 +256,7 @@ static void test_timer_pending(struct timer_info *info) > set_timer_irq_enabled(info, true); > > report(!info->irq_received, "no interrupt when timer is disabled"); > - report(!timer_pending(info) && gic_timer_check_state(info, GIC_IRQ_STATE_INACTIVE), > + report(gic_timer_check_state(info, GIC_IRQ_STATE_INACTIVE), > "interrupt signal no longer pending"); > > info->write_cval(now - 1); > -- > 2.34.1 >
> On Jun 28, 2023, at 4:40 AM, Andrew Jones <andrew.jones@linux.dev> wrote: > > On Wed, Jun 14, 2023 at 05:38:32PM -0700, Nadav Amit wrote: >> From: Nadav Amit <nadav.amit@gmail.com> >> >> According to ARM specifications for the vtimer (CNTV_CTL_EL0): "When the >> value of the ENABLE bit is 0, the ISTATUS field is UNKNOWN." >> >> Currently the test, however, does check that ISTATUS is cleared when the >> ENABLE bit is zero. Remove this check as the value is unknown. > > Hmm, does it? timer_pending() is > > /* Check that the timer condition is met. */ > static bool timer_pending(struct timer_info *info) > { > return (info->read_ctl() & ARCH_TIMER_CTL_ENABLE) && > (info->read_ctl() & ARCH_TIMER_CTL_ISTATUS); > } > > which I would expect to short-circuit when ENABLE is zero. Yes, my bad. Drop it. I had a different unrelated interrupt before that turned on the ENABLE. This just mask an additional failure. Obviously we can reset the ENABLE before each test, but I presume it is fair to say that in general if a certain test fails, subsequent failures are prone to be false-positives.
diff --git a/arm/timer.c b/arm/timer.c index 2cb8051..0b976a7 100644 --- a/arm/timer.c +++ b/arm/timer.c @@ -256,7 +256,7 @@ static void test_timer_pending(struct timer_info *info) set_timer_irq_enabled(info, true); report(!info->irq_received, "no interrupt when timer is disabled"); - report(!timer_pending(info) && gic_timer_check_state(info, GIC_IRQ_STATE_INACTIVE), + report(gic_timer_check_state(info, GIC_IRQ_STATE_INACTIVE), "interrupt signal no longer pending"); info->write_cval(now - 1);