Skip to content

Commit 159d552

Browse files
kilograhamWren6991
kilograham
and
Wren6991
authored
Fix bug in irq_remove_shared_handler and add test #823 (#825)
* Fix bug in irq_remove_shared_handler and add test #823 * Add comments to irq_handler_chain.S Co-authored-by: Luke Wren <[email protected]>
1 parent 3a3d5fe commit 159d552

File tree

7 files changed

+303
-53
lines changed

7 files changed

+303
-53
lines changed

‎src/rp2_common/hardware_irq/include/hardware/irq.h‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -103,6 +103,9 @@
103103
#define PICO_SHARED_IRQ_HANDLER_DEFAULT_ORDER_PRIORITY 0x80
104104
#endif
105105

106+
#define PICO_SHARED_IRQ_HANDLER_HIGHEST_ORDER_PRIORITY 0xff
107+
#define PICO_SHARED_IRQ_HANDLER_LOWEST_ORDER_PRIORITY 0x00
108+
106109
// PICO_CONFIG: PARAM_ASSERTIONS_ENABLED_IRQ, Enable/disable assertions in the IRQ module, type=bool, default=0, group=hardware_irq
107110
#ifndef PARAM_ASSERTIONS_ENABLED_IRQ
108111
#define PARAM_ASSERTIONS_ENABLED_IRQ 0
@@ -224,6 +227,9 @@ irq_handler_t irq_get_exclusive_handler(uint num);
224227
* rule of thumb is to use PICO_SHARED_IRQ_HANDLER_DEFAULT_ORDER_PRIORITY if you don't much care, as it is in the middle of
225228
* the priority range by default.
226229
*
230+
* \note The order_priority uses \em higher values for higher priorities which is the \em opposite of the CPU interrupt priorities passed
231+
* to irq_set_priority() which use lower values for higher priorities.
232+
*
227233
* \see irq_set_exclusive_handler()
228234
*/
229235
void irq_add_shared_handler(uint num, irq_handler_t handler, uint8_t order_priority);

‎src/rp2_common/hardware_irq/irq.c‎

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -287,7 +287,6 @@ void irq_remove_handler(uint num, irq_handler_t handler) {
287287
struct irq_handler_chain_slot *prev_slot = NULL;
288288
struct irq_handler_chain_slot *existing_vtable_slot = remove_thumb_bit(vtable_handler);
289289
struct irq_handler_chain_slot *to_free_slot = existing_vtable_slot;
290-
int8_t to_free_slot_index = get_slot_index(to_free_slot);
291290
while (to_free_slot->handler != handler) {
292291
prev_slot = to_free_slot;
293292
if (to_free_slot->link < 0) break;
@@ -325,7 +324,7 @@ void irq_remove_handler(uint num, irq_handler_t handler) {
325324
}
326325
// add slot back to free list
327326
to_free_slot->link = irq_hander_chain_free_slot_head;
328-
irq_hander_chain_free_slot_head = to_free_slot_index;
327+
irq_hander_chain_free_slot_head = get_slot_index(to_free_slot);
329328
} else {
330329
// since we are the last slot we know that our inst3 hasn't executed yet, so we change
331330
// it to bl to irq_handler_chain_remove_tail which will remove the slot.

‎src/rp2_common/hardware_irq/irq_handler_chain.S‎

Lines changed: 9 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -54,17 +54,18 @@ irq_handler_chain_slots:
5454
.endr
5555

5656
irq_handler_chain_first_slot:
57-
push {lr}
58-
ldr r0, [r1, #4]
59-
adds r1, #1
60-
mov lr, r1
61-
bx r0
57+
push {lr} // Save EXC_RETURN token, so `pop {pc}` will return from interrupt
58+
ldr r0, [r1, #4] // Get `handler` field of irq_handler_chain_slot
59+
adds r1, #1 // r1 points to `inst3` field of slot struct. Set Thumb bit on r1,
60+
mov lr, r1 // and copy to lr, so `inst3` is executed on return from handler
61+
bx r0 // Enter handler
62+
6263
irq_handler_chain_remove_tail:
63-
mov r0, lr
64-
subs r0, #9
64+
mov r0, lr // Get start of struct. This function was called by a bl at offset +4,
65+
subs r0, #9 // so lr points to offset +8. Note also lr has its Thumb bit set!
6566
ldr r1, =irq_add_tail_to_free_list
6667
blx r1
67-
pop {pc}
68+
pop {pc} // Top of stack is EXC_RETURN
6869

6970

7071
#endif

‎test/CMakeLists.txt‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@ add_subdirectory(pico_divider_test)
66
if (PICO_ON_DEVICE)
77
add_subdirectory(pico_float_test)
88
add_subdirectory(kitchen_sink)
9+
add_subdirectory(hardware_irq_test)
910
add_subdirectory(hardware_pwm_test)
1011
add_subdirectory(cmsis_test)
1112
endif()

‎test/hardware_irq_test/CMakeLists.txt‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,4 @@
1+
add_executable(hardware_irq_test hardware_irq_test.c)
2+
3+
target_link_libraries(hardware_irq_test PRIVATE pico_test hardware_irq hardware_dma)
4+
pico_add_extra_outputs(hardware_irq_test)

‎test/hardware_irq_test/hardware_irq_test.c‎

Lines changed: 282 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,282 @@
1+
/**
2+
* Copyright (c) 2020 Raspberry Pi (Trading) Ltd.
3+
*
4+
* SPDX-License-Identifier: BSD-3-Clause
5+
*/
6+
7+
#include <stdio.h>
8+
#include <stdarg.h>
9+
#include "pico/stdlib.h"
10+
#include "pico/test.h"
11+
#include "pico/time.h"
12+
#include "hardware/irq.h"
13+
#include "hardware/dma.h"
14+
#include "hardware/structs/scb.h"
15+
16+
PICOTEST_MODULE_NAME("IRQ", "IRQ Handler test");
17+
18+
extern void __unhandled_user_irq(void);
19+
20+
static bool remove_handler1_in_dma;
21+
static bool remove_handler2_in_dma;
22+
static bool remove_handler3_in_dma;
23+
static dma_channel_config config;
24+
25+
#define MAX_FIRE_COUNT 4
26+
static int fire_count;
27+
static int fired[MAX_FIRE_COUNT];
28+
29+
static uint32_t dma_to = 0;
30+
static uint32_t dma_from = 0xaaaa5555;
31+
32+
int record_fire(int which) {
33+
PICOTEST_CHECK(fire_count < MAX_FIRE_COUNT, "too many firings");
34+
fired[fire_count++] = which;
35+
return 0;
36+
}
37+
38+
void __isr handler1(void) {
39+
record_fire(1);
40+
dma_channel_acknowledge_irq0(0);
41+
if (remove_handler1_in_dma) {
42+
irq_remove_handler(DMA_IRQ_0, handler1);
43+
remove_handler1_in_dma = false;
44+
}
45+
}
46+
47+
void __isr handler2(void) {
48+
record_fire(2);
49+
dma_channel_acknowledge_irq0(0);
50+
if (remove_handler2_in_dma) {
51+
irq_remove_handler(DMA_IRQ_0, handler2);
52+
remove_handler2_in_dma = false;
53+
}
54+
}
55+
56+
void __isr handler3(void) {
57+
record_fire(3);
58+
dma_channel_acknowledge_irq0(0);
59+
if (remove_handler3_in_dma) {
60+
irq_remove_handler(DMA_IRQ_0, handler3);
61+
remove_handler3_in_dma = false;
62+
}
63+
}
64+
65+
static inline irq_handler_t *get_vtable(void) {
66+
return (irq_handler_t *) scb_hw->vtor;
67+
}
68+
69+
int dma_check(int expected, ...) {
70+
if (expected == 0) {
71+
// doing the DMA if there are no IRQ handlers will cause a hard fault, so we just check we are pointing at the handler which does this.
72+
PICOTEST_CHECK_AND_ABORT(get_vtable()[16 + DMA_IRQ_0] == __unhandled_user_irq, "Expected there to be no IRQ handlers");
73+
return 0;
74+
}
75+
fire_count = 0;
76+
dma_channel_configure(0, &config, &dma_to, &dma_from, 1, true);
77+
sleep_ms(100);
78+
va_list args;
79+
va_start(args, expected);
80+
bool ok = expected == fire_count;
81+
for(int i=0;ok && i<expected;i++) {
82+
if (fired[i] != va_arg(args, int)) {
83+
ok = false;
84+
break;
85+
}
86+
}
87+
va_end(args);
88+
if (!ok) {
89+
PICOTEST_CHECK(ok, "DMA handlers were not called in the order expected");
90+
printf(" EXPECTED handlers: ");
91+
va_start(args, expected);
92+
for(int i=0;i<expected;i++) {
93+
if (i) printf(", ");
94+
printf("%d", va_arg(args, int));
95+
}
96+
printf("\n");
97+
va_end(args);
98+
printf(" CALLED handlers: ");
99+
for(int i=0;i<fire_count;i++) {
100+
if (i) printf(", ");
101+
printf("%d", fired[i]);
102+
}
103+
printf("\n");
104+
return -1;
105+
}
106+
return 0;
107+
}
108+
109+
int main() {
110+
stdio_init_all();
111+
112+
PICOTEST_START();
113+
config = dma_channel_get_default_config(0);
114+
dma_channel_set_irq0_enabled(0, true);
115+
irq_set_enabled(DMA_IRQ_0, true);
116+
117+
// part I, just add/remove two handlers
118+
PICOTEST_START_SECTION("I: Add handler1 default priority");
119+
irq_add_shared_handler(DMA_IRQ_0, handler1, PICO_SHARED_IRQ_HANDLER_DEFAULT_ORDER_PRIORITY);
120+
dma_check(1, 1);
121+
PICOTEST_END_SECTION();
122+
123+
PICOTEST_START_SECTION("I: Add handler2 default priority");
124+
irq_add_shared_handler(DMA_IRQ_0, handler2, PICO_SHARED_IRQ_HANDLER_DEFAULT_ORDER_PRIORITY);
125+
dma_check(2, 2, 1);
126+
PICOTEST_END_SECTION();
127+
128+
PICOTEST_START_SECTION("I: Remove handler1");
129+
irq_remove_handler(DMA_IRQ_0, handler1);
130+
dma_check(1, 2);
131+
PICOTEST_END_SECTION();
132+
133+
PICOTEST_START_SECTION("I: Remove handler2");
134+
irq_remove_handler(DMA_IRQ_0, handler2);
135+
dma_check(0);
136+
PICOTEST_END_SECTION();
137+
138+
// part II, add/remove three handlers including one in the middle
139+
140+
PICOTEST_START_SECTION("II: Add handler3 default priority");
141+
irq_add_shared_handler(DMA_IRQ_0, handler3, PICO_SHARED_IRQ_HANDLER_DEFAULT_ORDER_PRIORITY);
142+
dma_check(1, 3);
143+
PICOTEST_END_SECTION();
144+
145+
PICOTEST_START_SECTION("II: Add handler2 default priority");
146+
irq_add_shared_handler(DMA_IRQ_0, handler2, PICO_SHARED_IRQ_HANDLER_DEFAULT_ORDER_PRIORITY);
147+
dma_check(2, 2, 3);
148+
PICOTEST_END_SECTION();
149+
150+
PICOTEST_START_SECTION("II: Add handler1 default priority");
151+
irq_add_shared_handler(DMA_IRQ_0, handler1, PICO_SHARED_IRQ_HANDLER_DEFAULT_ORDER_PRIORITY);
152+
dma_check(3, 1, 2, 3);
153+
PICOTEST_END_SECTION();
154+
155+
PICOTEST_START_SECTION("II: Remove handler2");
156+
irq_remove_handler(DMA_IRQ_0, handler2);
157+
dma_check(2, 1, 3);
158+
PICOTEST_END_SECTION();
159+
160+
PICOTEST_START_SECTION("II: Remove handler3");
161+
irq_remove_handler(DMA_IRQ_0, handler3);
162+
dma_check(1, 1);
163+
PICOTEST_END_SECTION();
164+
165+
PICOTEST_START_SECTION("II: Remove handler1");
166+
irq_remove_handler(DMA_IRQ_0, handler1);
167+
dma_check(0);
168+
PICOTEST_END_SECTION();
169+
170+
// part III, the same as part II, but removing the handlers during the IRQ
171+
172+
PICOTEST_START_SECTION("III: Add handler3 default priority");
173+
irq_add_shared_handler(DMA_IRQ_0, handler3, PICO_SHARED_IRQ_HANDLER_DEFAULT_ORDER_PRIORITY);
174+
dma_check(1, 3);
175+
PICOTEST_END_SECTION();
176+
177+
PICOTEST_START_SECTION("III: Add handler2 default priority");
178+
irq_add_shared_handler(DMA_IRQ_0, handler2, PICO_SHARED_IRQ_HANDLER_DEFAULT_ORDER_PRIORITY);
179+
dma_check(2, 2, 3);
180+
PICOTEST_END_SECTION();
181+
182+
PICOTEST_START_SECTION("III: Add handler1 default priority");
183+
irq_add_shared_handler(DMA_IRQ_0, handler1, PICO_SHARED_IRQ_HANDLER_DEFAULT_ORDER_PRIORITY);
184+
dma_check(3, 1, 2, 3);
185+
PICOTEST_END_SECTION();
186+
187+
PICOTEST_START_SECTION("III: Remove handler2");
188+
remove_handler2_in_dma = true;
189+
// note that this is the defined behavior, that any handlers after the removed handler are not called. the
190+
// reasoning is that the same IRQ will immediately be re-entered, as we have missed clearing an interrupt (in whichever
191+
// handlers were not called), and we will call any remaining handlers then. Because each IRQ handler is clearing the same
192+
// interrupt handler in our case, this does not happen.
193+
dma_check(2, 1, 2);
194+
// but we call again to check
195+
dma_check(2, 1, 3);
196+
PICOTEST_END_SECTION();
197+
198+
PICOTEST_START_SECTION("III: Remove handler3");
199+
remove_handler3_in_dma = true;
200+
// note that this is the defined behavior, that any handlers after the removed handler are not called. the
201+
// reasoning is that the same IRQ will immediately be re-entered, as we have missed clearing an interrupt (in whichever
202+
// handlers were not called), and we will call any remaining handlers then. Because each IRQ handler is clearing the same
203+
// interrupt handler in our case, this does not happen.
204+
dma_check(2, 1, 3);
205+
// but we call again to check
206+
dma_check(1, 1);
207+
PICOTEST_END_SECTION();
208+
209+
PICOTEST_START_SECTION("III: Remove handler3");
210+
remove_handler1_in_dma = true;
211+
// note that this is the defined behavior, that any handlers after the removed handler are not called. the
212+
// reasoning is that the same IRQ will immediately be re-entered, as we have missed clearing an interrupt (in whichever
213+
// handlers were not called), and we will call any remaining handlers then. Because each IRQ handler is clearing the same
214+
// interrupt handler in our case, this does not happen.
215+
dma_check(1, 1);
216+
// but we call again to check
217+
dma_check(0);
218+
PICOTEST_END_SECTION();
219+
220+
// part IV, checking priorities
221+
PICOTEST_START_SECTION("IV: Add handler1 high priority");
222+
irq_add_shared_handler(DMA_IRQ_0, handler1, PICO_SHARED_IRQ_HANDLER_HIGHEST_ORDER_PRIORITY);
223+
dma_check(1, 1);
224+
PICOTEST_END_SECTION();
225+
226+
PICOTEST_START_SECTION("IV: Add handler2 normal priority");
227+
irq_add_shared_handler(DMA_IRQ_0, handler2, PICO_SHARED_IRQ_HANDLER_DEFAULT_ORDER_PRIORITY);
228+
dma_check(2, 1, 2);
229+
PICOTEST_END_SECTION();
230+
231+
PICOTEST_START_SECTION("IV: Add handler3 lowest priority");
232+
irq_add_shared_handler(DMA_IRQ_0, handler3, PICO_SHARED_IRQ_HANDLER_LOWEST_ORDER_PRIORITY);
233+
dma_check(3, 1, 2, 3);
234+
PICOTEST_END_SECTION();
235+
236+
PICOTEST_START_SECTION("IV: Remove handler3");
237+
irq_remove_handler(DMA_IRQ_0, handler3);
238+
dma_check(2, 1, 2);
239+
PICOTEST_END_SECTION();
240+
241+
PICOTEST_START_SECTION("IV: Add handler3 normal priority");
242+
irq_add_shared_handler(DMA_IRQ_0, handler3, PICO_SHARED_IRQ_HANDLER_DEFAULT_ORDER_PRIORITY);
243+
dma_check(3, 1, 3, 2);
244+
PICOTEST_END_SECTION();
245+
246+
PICOTEST_START_SECTION("IV: Remove handler2");
247+
irq_remove_handler(DMA_IRQ_0, handler2);
248+
dma_check(2, 1, 3);
249+
PICOTEST_END_SECTION();
250+
251+
PICOTEST_START_SECTION("IV: Add handler2 normal priority - 2");
252+
irq_add_shared_handler(DMA_IRQ_0, handler2, PICO_SHARED_IRQ_HANDLER_DEFAULT_ORDER_PRIORITY - 2);
253+
dma_check(3, 1, 3, 2);
254+
PICOTEST_END_SECTION();
255+
256+
PICOTEST_START_SECTION("IV: Remove handler1");
257+
irq_remove_handler(DMA_IRQ_0, handler1);
258+
dma_check(2, 3, 2);
259+
PICOTEST_END_SECTION();
260+
261+
PICOTEST_START_SECTION("IV: Add handler1 normal priority - 1");
262+
irq_add_shared_handler(DMA_IRQ_0, handler1, PICO_SHARED_IRQ_HANDLER_DEFAULT_ORDER_PRIORITY - 1);
263+
dma_check(3, 3, 1, 2);
264+
PICOTEST_END_SECTION();
265+
266+
PICOTEST_START_SECTION("IV: Remove handler2 again");
267+
irq_remove_handler(DMA_IRQ_0, handler2);
268+
dma_check(2, 3, 1);
269+
270+
PICOTEST_END_SECTION();
271+
PICOTEST_START_SECTION("IV: Remove handler3 again");
272+
irq_remove_handler(DMA_IRQ_0, handler3);
273+
dma_check(1, 1);
274+
PICOTEST_END_SECTION();
275+
276+
PICOTEST_START_SECTION("IV: Remove handler1 again");
277+
irq_remove_handler(DMA_IRQ_0, handler1);
278+
dma_check(0);
279+
PICOTEST_END_SECTION();
280+
PICOTEST_END_TEST();
281+
}
282+

0 commit comments

Comments
 (0)