Skip to content

Commit 3474abb

Browse files
committed
Fix ISR-safe critical sections and document APIs
The CRITICAL_ENTER/LEAVE macros unconditionally enabled interrupts on leave, which corrupts interrupt state when called from ISR context (where MIE is already 0). Replace with nesting-aware save/restore: CRITICAL_ENTER saves MIE on first entry, CRITICAL_LEAVE restores it on last exit. Add underflow guard to CRITICAL_LEAVE. This fixes mo_logger_enqueue() to use CRITICAL_ENTER instead of mo_mutex_lock so the [ISR-SAFE] contract is actually honored -- mutex blocks, which deadlocks in ISR context. It also fixes realloc() missing CRITICAL_ENTER before fast paths that called CRITICAL_LEAVE, and add defensive CRITICAL_LEAVE before panic() in malloc(). ISR Context Safety Guide is added to hal-interrupt.md with function tables, callback patterns, and ISR-to-task communication examples.
1 parent f063a38 commit 3474abb

14 files changed

Lines changed: 475 additions & 61 deletions

File tree

Documentation/hal-interrupt.md

Lines changed: 136 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -209,9 +209,144 @@ void bad_critical_section(void) {
209209
/* CORRECT */
210210
void good_pattern(void) {
211211
mo_sem_wait(semaphore); /* Block outside critical section */
212-
212+
213213
CRITICAL_ENTER();
214214
/* Quick critical work */
215215
CRITICAL_LEAVE();
216216
}
217217
```
218+
219+
## ISR Context Safety Guide
220+
221+
### Overview
222+
Interrupt Service Routines (ISRs), including timer callbacks, execute in interrupt context with special restrictions.
223+
Violating these restrictions causes heap corruption, deadlocks, or undefined behavior.
224+
225+
### ISR-Safe vs Task-Only Functions
226+
227+
#### ISR-Safe Functions
228+
These functions can be called from ISR context (timer callbacks, trap handlers):
229+
230+
| Function | Protection | Notes |
231+
|----------|------------|-------|
232+
| `mo_task_id()` | None needed | Read-only |
233+
| `mo_task_count()` | None needed | Read-only |
234+
| `mo_ticks()` | None needed | Volatile read |
235+
| `mo_uptime()` | None needed | Calculated |
236+
| `mo_timer_start/cancel()` | NOSCHED | Timer control only |
237+
| `mo_sem_trywait()` | None | Non-blocking |
238+
| `mo_sem_signal()` | NOSCHED | Wakes waiting tasks |
239+
| `mo_mutex_trylock()` | None | Non-blocking |
240+
| `mo_mutex_unlock()` | NOSCHED | Releases lock |
241+
| `mo_cond_signal/broadcast()` | NOSCHED | Wakes waiters |
242+
| `mo_pipe_nbread/nbwrite()` | None | Non-blocking I/O |
243+
| `mo_logger_enqueue()` | CRITICAL | Deferred logging |
244+
| `isr_puts()` | None | Direct UART output |
245+
| `isr_putx()` | None | Direct UART hex output |
246+
247+
#### Task-Only Functions
248+
These functions must NOT be called from ISR context:
249+
250+
| Function | Reason | Alternative |
251+
|----------|--------|-------------|
252+
| `mo_task_spawn()` | Uses malloc | Pre-create tasks |
253+
| `mo_task_cancel()` | Uses free | Defer to task |
254+
| `mo_timer_create()` | Uses malloc | Pre-create timers |
255+
| `mo_timer_destroy()` | Uses free | Defer to task |
256+
| `mo_task_delay/yield()` | Invokes scheduler | Use timer callback |
257+
| `mo_task_suspend/resume()` | Modifies scheduler | Use semaphore |
258+
| `mo_sem_wait()` | Blocks caller | Use `mo_sem_trywait()` |
259+
| `mo_mutex_lock()` | Blocks caller | Use `mo_mutex_trylock()` |
260+
| `mo_cond_wait()` | Blocks caller | Signal from ISR, wait in task |
261+
| `mo_pipe_read/write()` | Blocks caller | Use `mo_pipe_nbread/nbwrite()` |
262+
| `mo_mq_enqueue/dequeue()` | Uses malloc | Use pipe instead |
263+
| `printf()` | May deadlock | Use `mo_logger_enqueue()` |
264+
265+
### Timer Callback Patterns
266+
267+
Timer callbacks execute in ISR context. Example safe callback:
268+
```c
269+
void *my_callback(void *arg) {
270+
/* Safe: ISR-protected logging */
271+
mo_logger_enqueue("Timer fired\n", 12);
272+
273+
/* Safe: Non-blocking semaphore signal to wake task */
274+
mo_sem_signal(signal_sem);
275+
276+
/* Safe: Non-blocking pipe write */
277+
char msg[] = "event";
278+
mo_pipe_nbwrite(event_pipe, msg, sizeof(msg));
279+
280+
/* UNSAFE - do not use in callbacks:
281+
* mo_task_spawn(...); // Uses malloc
282+
* mo_task_delay(10); // Invokes scheduler
283+
* mo_sem_wait(sem); // Blocks caller
284+
* printf(...); // May deadlock
285+
*/
286+
287+
return NULL;
288+
}
289+
```
290+
291+
### ISR-to-Task Communication Patterns
292+
293+
#### Pattern 1: Semaphore Signaling
294+
```c
295+
/* ISR/callback: Signal event */
296+
void *timer_callback(void *arg) {
297+
mo_sem_signal(event_sem);
298+
return NULL;
299+
}
300+
301+
/* Task: Wait for event */
302+
void event_handler_task(void) {
303+
while (1) {
304+
mo_sem_wait(event_sem); /* Blocks until ISR signals */
305+
process_event();
306+
}
307+
}
308+
```
309+
310+
#### Pattern 2: Non-Blocking Pipe
311+
```c
312+
/* ISR/callback: Send data */
313+
void *sensor_callback(void *arg) {
314+
sensor_data_t data = read_sensor();
315+
mo_pipe_nbwrite(sensor_pipe, &data, sizeof(data));
316+
return NULL;
317+
}
318+
319+
/* Task: Process data (non-blocking read, polls or uses semaphore wakeup) */
320+
void sensor_task(void) {
321+
sensor_data_t data;
322+
while (1) {
323+
if (mo_pipe_nbread(sensor_pipe, &data, sizeof(data)) > 0)
324+
process_sensor_data(&data);
325+
mo_task_delay(1); /* Yield or use semaphore for event-driven */
326+
}
327+
}
328+
```
329+
330+
#### Pattern 3: Deferred Logging
331+
```c
332+
/* ISR/callback: Queue log message */
333+
void *debug_callback(void *arg) {
334+
mo_logger_enqueue("Tick!\n", 6);
335+
return NULL;
336+
}
337+
/* Logger task automatically outputs queued messages */
338+
```
339+
340+
### Emergency Debug Output
341+
342+
For debugging trap handlers or when the logger is unavailable, use direct UART:
343+
```c
344+
void *debug_isr(void *arg) {
345+
isr_puts("[ISR] Value=0x");
346+
isr_putx(some_value);
347+
isr_puts("\r\n");
348+
return NULL;
349+
}
350+
```
351+
352+
Warning: Direct UART output is blocking and slow. Use only for emergency debugging.

app/timer.c

Lines changed: 13 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -2,11 +2,18 @@
22
*
33
* It creates several timers with different periods and modes to verify
44
* their correct operation.
5+
*
6+
* Note: Timer callbacks execute in ISR context. This test uses printf()
7+
* which may fall back to blocking direct output when the queue is full
8+
* or the message exceeds LOG_ENTRY_SZ. For production ISR callbacks,
9+
* prefer mo_logger_enqueue() or isr_puts() which are always ISR-safe.
510
*/
611
#include <linmo.h>
712

8-
/* Helper function to print the current system uptime. */
9-
void print_time()
13+
/* Helper function to print the current system uptime.
14+
* Called from timer callback (ISR context) - uses ISR-safe logging.
15+
*/
16+
static void print_time(void)
1017
{
1118
/* mo_uptime() returns a 64-bit value to prevent rollover. */
1219
uint64_t time_ms = mo_uptime();
@@ -17,8 +24,10 @@ void print_time()
1724
}
1825

1926
/* Generic timer callback.
20-
* We can use a single callback for multiple timers by passing a unique
21-
* identifier as the argument.
27+
* Executes in ISR context - only ISR-safe functions are permitted here.
28+
* This example uses printf() for convenience in a test app; it may block
29+
* on direct output fallback. For guaranteed ISR-safe logging, use
30+
* mo_logger_enqueue(), isr_puts(), or isr_putx().
2231
*/
2332
void *timer_callback(void *arg)
2433
{

include/lib/libc.h

Lines changed: 42 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -143,6 +143,48 @@ int random_r(struct random_data *buf, int32_t *result);
143143
int32_t puts(const char *str);
144144
int _putchar(int c); /* Low-level character output (used by logger) */
145145

146+
/* ISR-Safe I/O Functions
147+
* [ISR-SAFE] These functions are safe to call from interrupt context.
148+
* They perform direct UART output without locks, buffering, or malloc.
149+
* Use for emergency debug output in timer callbacks and trap handlers.
150+
*
151+
* Warning: Direct UART output is slow and blocks until complete.
152+
* For normal logging, prefer mo_logger_enqueue() which is non-blocking.
153+
*/
154+
155+
/* Maximum characters isr_puts() will emit per call.
156+
* Prevents unbounded blocking in ISR context on long strings.
157+
*/
158+
#define ISR_OUTPUT_MAX 128
159+
160+
/* ISR-safe string output - direct UART, no buffering.
161+
* [ISR-SAFE] Safe to call from any context including ISR and timer callbacks.
162+
* Output is capped at ISR_OUTPUT_MAX characters to bound ISR latency.
163+
*
164+
* @s : Null-terminated string to output
165+
*/
166+
static inline void isr_puts(const char *s)
167+
{
168+
for (uint32_t i = 0; s && *s && i < ISR_OUTPUT_MAX; i++)
169+
_putchar(*s++);
170+
}
171+
172+
/* ISR-safe hexadecimal output - direct UART, no buffering.
173+
* [ISR-SAFE] Safe to call from any context including ISR and timer callbacks.
174+
*
175+
* Outputs a 32-bit value as 8 hex digits (e.g., "DEADBEEF").
176+
* Useful for printing addresses and error codes in trap handlers.
177+
*
178+
* @val : 32-bit value to output as hexadecimal
179+
*/
180+
static inline void isr_putx(uint32_t val)
181+
{
182+
for (int i = 28; i >= 0; i -= 4) {
183+
uint32_t nibble = (val >> i) & 0xF;
184+
_putchar(nibble < 10 ? '0' + nibble : 'A' + nibble - 10);
185+
}
186+
}
187+
146188
/* Character and string input */
147189
int getchar(void);
148190
char *gets(char *s);

include/linmo.h

Lines changed: 47 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,52 @@
11
#pragma once
22

3+
/* Linmo Operating System - Main API Header
4+
*
5+
* This header includes all kernel APIs for task management, synchronization,
6+
* IPC, and system services.
7+
*
8+
* Interrupt Service Routines (ISRs), including timer callbacks, execute in
9+
* interrupt context with special restrictions. Violating these restrictions
10+
* causes heap corruption, deadlocks, or undefined behavior.
11+
*
12+
* [ISR-SAFE] Functions (callable from interrupt context):
13+
* -------------------------------------------------------
14+
* System Info: mo_task_id(), mo_task_count(), mo_ticks(), mo_uptime()
15+
* Timer API: mo_timer_create/destroy/start/cancel() [NOSCHED protected]
16+
* Semaphore: mo_sem_trywait(), mo_sem_signal() [non-blocking]
17+
* Mutex: mo_mutex_trylock(), mo_mutex_unlock() [non-blocking]
18+
* Condition Var: mo_cond_signal(), mo_cond_broadcast() [non-blocking]
19+
* Pipe I/O: mo_pipe_nbread(), mo_pipe_nbwrite() [non-blocking]
20+
* Logging: mo_logger_enqueue() [CRITICAL protected]
21+
* Direct I/O: _putchar() for emergency/debug output
22+
*
23+
* [TASK-ONLY] Functions (must NOT call from ISR context):
24+
* -------------------------------------------------------
25+
* Memory: mo_task_spawn(), mo_task_cancel() - use malloc/free
26+
* Blocking: mo_task_delay(), mo_task_yield(), mo_task_suspend()
27+
* Blocking Sync: mo_sem_wait(), mo_mutex_lock(), mo_cond_wait()
28+
* Blocking I/O: mo_pipe_read(), mo_pipe_write()
29+
* Message Queue: mo_mq_enqueue(), mo_mq_dequeue() - unprotected malloc
30+
* Stdio: printf(), puts() - may deadlock in preemptive mode
31+
*
32+
* Timer Callback Rules:
33+
* ---------------------
34+
* Timer callbacks execute in ISR context. Example safe callback:
35+
*
36+
* void *my_callback(void *arg) {
37+
* mo_logger_enqueue("Timer fired\n", 12); // Safe: ISR-protected
38+
* mo_sem_signal(signal_sem); // Safe: non-blocking
39+
* // UNSAFE: mo_task_spawn(...); // Uses malloc
40+
* // UNSAFE: printf(...); // May deadlock
41+
* return NULL;
42+
* }
43+
*
44+
* For emergency debug output in ISR, use the ISR-safe I/O functions:
45+
* isr_puts("message"); // Direct UART string output
46+
* isr_putx(0xDEADBEEF); // Direct UART hex output
47+
* These are defined in lib/libc.h and available via linmo.h.
48+
*/
49+
350
#include <types.h>
451

552
#include <lib/libc.h>

include/sys/logger.h

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -37,8 +37,15 @@
3737
int32_t mo_logger_init(void);
3838

3939
/* Enqueue a log message for deferred output.
40+
* [ISR-SAFE] Protected by CRITICAL_ENTER - safe to call from ISR context
41+
*
4042
* Non-blocking: if queue is full, message is dropped.
4143
* Thread-safe: protected by short critical section.
44+
*
45+
* This is the RECOMMENDED way to log from timer callbacks and ISRs.
46+
* The message is queued and output later by the logger task, avoiding
47+
* the risk of deadlock from direct UART output in ISR context.
48+
*
4249
* @msg : Null-terminated message string
4350
* @length : Length of message (excluding null terminator)
4451
*

include/sys/mqueue.h

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,17 @@
77
* Provides FIFO message passing between tasks with type-safe message
88
* containers. Messages consist of a user-allocated payload, a type
99
* discriminator, and size information.
10+
*
11+
* ISR-SAFETY WARNING:
12+
* [TASK-ONLY] All message queue operations are task-context only.
13+
* Message queue operations use underlying queue functions that may perform
14+
* memory allocation. These operations are NOT protected by CRITICAL_ENTER
15+
* and are NOT safe to call from ISR context.
16+
*
17+
* For ISR-to-task communication, use:
18+
* - mo_pipe_nbwrite() for simple byte streams
19+
* - mo_sem_signal() to wake a task
20+
* - mo_logger_enqueue() for logging
1021
*/
1122

1223
/* Message container structure */

include/sys/mutex.h

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -54,6 +54,8 @@ int32_t mo_mutex_destroy(mutex_t *m);
5454
/* Mutex Locking Operations */
5555

5656
/* Acquire mutex lock, blocking if necessary.
57+
* [TASK-ONLY] May block caller - NOT safe from ISR context
58+
*
5759
* If the mutex is free, acquires it immediately. If owned by another task,
5860
* the calling task is placed in the wait queue and blocked until the mutex
5961
* becomes available. Non-recursive - returns error if caller already owns
@@ -65,6 +67,8 @@ int32_t mo_mutex_destroy(mutex_t *m);
6567
int32_t mo_mutex_lock(mutex_t *m);
6668

6769
/* Attempt to acquire mutex lock without blocking.
70+
* [ISR-SAFE] Non-blocking, returns immediately with success/failure
71+
*
6872
* Returns immediately whether the lock was acquired or not.
6973
* @m : Pointer to mutex structure (must be valid)
7074
*
@@ -73,6 +77,8 @@ int32_t mo_mutex_lock(mutex_t *m);
7377
int32_t mo_mutex_trylock(mutex_t *m);
7478

7579
/* Attempt to acquire mutex lock with timeout.
80+
* [TASK-ONLY] May block caller - NOT safe from ISR context
81+
*
7682
* Blocks for up to 'ticks' scheduler ticks waiting for the mutex.
7783
* @m : Pointer to mutex structure (must be valid)
7884
* @ticks : Maximum time to wait in scheduler ticks (0 = trylock behavior)
@@ -83,6 +89,8 @@ int32_t mo_mutex_trylock(mutex_t *m);
8389
int32_t mo_mutex_timedlock(mutex_t *m, uint32_t ticks);
8490

8591
/* Release mutex lock.
92+
* [ISR-SAFE] Protected by NOSCHED_ENTER - safe from any context
93+
*
8694
* If tasks are waiting, ownership is transferred to the next task in FIFO
8795
* order. The released task is marked ready but may not run immediately
8896
* depending on scheduler priority.
@@ -142,6 +150,8 @@ int32_t mo_cond_destroy(cond_t *c);
142150
/* Condition Variable Wait Operations */
143151

144152
/* Wait on condition variable (atomically releases mutex).
153+
* [TASK-ONLY] Blocks caller - NOT safe from ISR context
154+
*
145155
* The calling task must own the specified mutex. The mutex is atomically
146156
* released and the task blocks until another task signals the condition.
147157
* Upon waking, the mutex is re-acquired before returning.
@@ -153,6 +163,8 @@ int32_t mo_cond_destroy(cond_t *c);
153163
int32_t mo_cond_wait(cond_t *c, mutex_t *m);
154164

155165
/* Wait on condition variable with timeout.
166+
* [TASK-ONLY] Blocks caller - NOT safe from ISR context
167+
*
156168
* Like mo_cond_wait(), but limits wait time to specified number of ticks.
157169
* @c : Pointer to condition variable structure (must be valid)
158170
* @m : Pointer to mutex structure that caller must own
@@ -166,6 +178,8 @@ int32_t mo_cond_timedwait(cond_t *c, mutex_t *m, uint32_t ticks);
166178
/* Condition Variable Signal Operations */
167179

168180
/* Signal one waiting task.
181+
* [ISR-SAFE] Protected by NOSCHED_ENTER - safe for ISR-to-task signaling
182+
*
169183
* Wakes up the oldest task waiting on the condition variable (FIFO order).
170184
* The signaled task will attempt to re-acquire the associated mutex.
171185
* @c : Pointer to condition variable structure (must be valid)
@@ -175,6 +189,8 @@ int32_t mo_cond_timedwait(cond_t *c, mutex_t *m, uint32_t ticks);
175189
int32_t mo_cond_signal(cond_t *c);
176190

177191
/* Signal all waiting tasks.
192+
* [ISR-SAFE] Protected by NOSCHED_ENTER - safe from any context
193+
*
178194
* Wakes up all tasks waiting on the condition variable. All tasks will
179195
* attempt to re-acquire their associated mutexes, but only one will succeed
180196
* immediately (others will block on the mutex).

0 commit comments

Comments
 (0)