Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

tests: interrupt: refine the offload case not rely on delay timing #35302

Merged

Conversation

enjiamai
Copy link
Collaborator

@enjiamai enjiamai commented May 14, 2021

The interrupt offload testcases fail on some boards because the timing
of the delay is too short. Refine the testcases and make it not rely
on the delay timing.

Fixes #35097
Fixes #35241

These testcases reported failure in those board:

  • verified pass
    intel_adsp_cavs15

  • need help to verify, I do not have these boards, could you please help to verify? Appreciate your help.
    @MaureenHelm @hakehuang
    frdm_kl25z

    @ABOSTM
    nucleo_f091rc
    nucleo_l073
    nucleo_l152re

Signed-off-by: Enjia Mai enjiax.mai@intel.com

@github-actions github-actions bot added area: Kernel area: Tests Issues related to a particular existing or missing test labels May 14, 2021
@enjiamai enjiamai force-pushed the fix-offload-testcase-timing branch from 87a8ad9 to 569d421 Compare May 14, 2021 15:50
@enjiamai
Copy link
Collaborator Author

Excuse me, @MaureenHelm @hakehuang @ABOSTM , could you please help to verify if this issue got fixed on those boards when you got a chance, thank you so much!

And thanks for the solution that @MaureenHelm provided~

@hakehuang
Copy link
Collaborator

hakehuang commented May 15, 2021

Excuse me, @MaureenHelm @hakehuang @ABOSTM , could you please help to verify if this issue got fixed on those boards when you got a chance, thank you so much!

And thanks for the solution that @MaureenHelm provided~

update with your patch below

diff --git a/tests/kernel/interrupt/src/interrupt_offload.c b/tests/kernel/interrupt/src/interrupt_offload.c
index d00a6167c5..4b741dd813 100644
--- a/tests/kernel/interrupt/src/interrupt_offload.c
+++ b/tests/kernel/interrupt/src/interrupt_offload.c
@@ -11,6 +11,9 @@
 #define STACK_SIZE     1024
 #define NUM_WORK       4

+#define DELAY_US       (1000000UL * CONFIG_SYS_CLOCK_TICKS_PER_SEC  \
+                               / CONFIG_SYS_CLOCK_HW_CYCLES_PER_SEC)
+
 static struct k_work offload_work[NUM_WORK];
 static struct k_work_q wq_queue;
 static K_THREAD_STACK_DEFINE(wq_stack, STACK_SIZE);
@@ -127,7 +130,7 @@ static void t_running(void *p1, void *p2, void *p3)
 {
        while (1) {
                orig_t_keep_run = 1;
-               k_usleep(1);
+               k_usleep(DELAY_US);
        }
 }

@@ -173,7 +176,7 @@ static void run_test_offload(int case_type, int real_irq)
        }

        /* wait for thread start */
-       k_usleep(10);
+       k_usleep(DELAY_US);

        for (int i = 0; i < NUM_WORK; i++) {

@@ -194,7 +197,7 @@ static void run_test_offload(int case_type, int real_irq)
        /* wait for all offload job complete */
        k_sem_take(&end_sem, K_FOREVER);

-       k_usleep(1);
+       k_usleep(DELAY_US);

        zassert_equal(orig_t_keep_run, 1,
                        "offload job done, the original thread run");
@@ -219,6 +222,8 @@ static void run_test_offload(int case_type, int real_irq)
  */
 void test_isr_offload_job_multiple(void)
 {
+       TC_PRINT("delay unit is %ld us\n", DELAY_US);
+
        offload_job_prio_higher = false;
        run_test_offload(TEST_OFFLOAD_MULTI_JOBS, false);

the issues is still there

*** Booting Zephyr OS build zephyr-v2.5.0-3845-gf8ac3a49ec95  ***
Running test suite interrupt_feature
===================================================================
START - test_isr_dynamic
installing dynamic ISR for IRQ 0
 PASS - test_isr_dynamic in 0.3 seconds
===================================================================
START - test_nested_isr
Triggering irq : 29
isr0: Enter
Triggering irq : 28
isr1: Enter
isr1: Leave
isr0: Leave
 PASS - test_nested_isr in 0.19 seconds
===================================================================
START - test_prevent_interruption
locking interrupts
unlocking interrupts
timer fired
 PASS - test_prevent_interruption in 2.5 seconds
===================================================================
START - test_isr_regular
 SKIP - test_isr_regular in 0.1 seconds
===================================================================
START - test_isr_offload_job_multiple
delay unit is 29 us
case 0
case 0

ASSERTION FAIL [work->queue != ((void *)0)] @ WEST_TOPDIR/zephyr/kernel/work.c:334
E: ***** HARD FAULT *****
E: ARCH_EXCEPT with reason 4

E: r0/a1:  0x00000004  r1/a2:  0x0000014e  r2/a3:  0x00006e1c
E: r3/a4:  0x000035c5 r12/ip:  0x00000003 r14/lr:  0x000035cf
E:  xpsr:  0x0100000b
E: Faulting instruction address (r15/pc): 0x00005782
E: >>> ZEPHYR FATAL ERROR 4: Kernel panic on CPU 0
E: Fault during interrupt handling

E: Current thread: 0x1ffff480 (ztest_thread)
E: Halting system

and looks like the delay is not enough,

delay unit is 29 us

and the value is here for frdm_kl25z

#define CONFIG_SYS_CLOCK_TICKS_PER_SEC 10000
#define CONFIG_SYS_CLOCK_HW_CYCLES_PER_SEC 48000000

when I manually change below as Maureen suggested, it works.

@@ -194,7 +197,7 @@ static void run_test_offload(int case_type, int real_irq)
        /* wait for all offload job complete */
        k_sem_take(&end_sem, K_FOREVER);

-       k_usleep(1);
+       k_usleep(1000);

IMHO, the interrupt handling is better to controled by semephore, or lock, rather than use different delay, which will be difficult to maintain over time.

@enjiamai enjiamai marked this pull request as draft May 15, 2021 02:41
@enjiamai
Copy link
Collaborator Author

@hakehuang ok, thanks for the help, let me update it.

@enjiamai enjiamai force-pushed the fix-offload-testcase-timing branch 2 times, most recently from 4eae070 to 3c3670e Compare May 15, 2021 15:45
@enjiamai enjiamai changed the title tests: interrupt: adjust the delay time to fit different boards tests: interrupt: refine the offload case not rely on delay timing May 15, 2021
@enjiamai enjiamai force-pushed the fix-offload-testcase-timing branch from 3c3670e to a5e2e79 Compare May 16, 2021 12:12
@enjiamai enjiamai marked this pull request as ready for review May 16, 2021 12:31
@enjiamai
Copy link
Collaborator Author

@hakehuang I make the testcase not relying on the timing, base on your suggestion. Could you please help to verify it again, thanks!

Copy link
Collaborator

@hakehuang hakehuang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@ABOSTM
Copy link
Collaborator

ABOSTM commented May 17, 2021

@enjiamai ,
All 3 boards: nucleo_f091rc, nucleo_l073 and nucleo_l152re
are now passed.
Thanks

@enjiamai
Copy link
Collaborator Author

@hakehuang @ABOSTM Thanks for the help on verifying them!

@PerMac
Copy link
Member

PerMac commented May 17, 2021

I checked this PR at nrf boards and it solves the issue also on our platforms

@enjiamai
Copy link
Collaborator Author

I checked this PR at nrf boards and it solves the issue also on our platforms

@PerMac , thanks for your help with this!

@enjiamai enjiamai force-pushed the fix-offload-testcase-timing branch from a5e2e79 to fd3ec57 Compare May 17, 2021 12:21
@zephyrbot zephyrbot requested a review from ceolin May 17, 2021 13:16
@MaureenHelm
Copy link
Member

I verified that this PR allows the test to pass on frdm_kl25z, but would like @andyross or @ioannisg to review the functional changes to the test before we merge.

@MaureenHelm MaureenHelm added this to the v2.6.0 milestone May 18, 2021
@MaureenHelm MaureenHelm added the bug The issue is a bug, or the PR is fixing a bug label May 18, 2021
@MaureenHelm
Copy link
Member

@andyross will you please review?

@enjiamai enjiamai force-pushed the fix-offload-testcase-timing branch from fd3ec57 to 2a62616 Compare May 20, 2021 08:49
Copy link
Member

@stephanosio stephanosio left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

First pass

tests/kernel/interrupt/src/interrupt_offload.c Outdated Show resolved Hide resolved
tests/kernel/interrupt/src/interrupt_offload.c Outdated Show resolved Hide resolved
tests/kernel/interrupt/src/interrupt_offload.c Outdated Show resolved Hide resolved
tests/kernel/interrupt/src/interrupt_offload.c Outdated Show resolved Hide resolved
tests/kernel/interrupt/src/interrupt_offload.c Outdated Show resolved Hide resolved
tests/kernel/interrupt/src/interrupt_offload.c Outdated Show resolved Hide resolved
tests/kernel/interrupt/src/interrupt_offload.c Outdated Show resolved Hide resolved
@stephanosio stephanosio requested a review from pabigot May 20, 2021 11:02
@enjiamai enjiamai force-pushed the fix-offload-testcase-timing branch from 2a62616 to 00752a4 Compare May 20, 2021 15:02
The interrupt offload testcases fail on some boards because the timing
of the delay is too short. Refine the testcases and make it not rely
on the delay timing.

Fixes zephyrproject-rtos#35097
Fixes zephyrproject-rtos#35241

Signed-off-by: Enjia Mai <enjiax.mai@intel.com>
@enjiamai enjiamai force-pushed the fix-offload-testcase-timing branch from 00752a4 to fef3a88 Compare May 20, 2021 15:06
@enjiamai
Copy link
Collaborator Author

Hi, @stephanosio Thanks for your review! I have addressed the comments you gave. Could you please help to take look at this again?

@enjiamai enjiamai requested a review from stephanosio May 20, 2021 15:13
Copy link
Member

@stephanosio stephanosio left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks ok to me.

p.s. verified working on nrf52840dk_nrf52840 and nrf51dk_nrf51422.

@nashif nashif merged commit 04c736d into zephyrproject-rtos:main May 20, 2021
@enjiamai enjiamai deleted the fix-offload-testcase-timing branch May 21, 2021 01:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: Kernel area: Tests Issues related to a particular existing or missing test bug The issue is a bug, or the PR is fixing a bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

intel_adsp_cavs15: run interrupt testcases failed on ADSP arch.interrupt: fails on NXP Cortex-M0+ platforms
8 participants