Conversation
8869bb5 to
88de893
Compare
There was a problem hiding this comment.
3 issues found across 10 files
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="include/platform/irq.h">
<violation number="1" location="include/platform/irq.h:326">
P1: Unconditionally enabling interrupts with `cpsie i` may break critical sections. If this macro is called when interrupts are already disabled, it will incorrectly re-enable them. Use `irq_save_flags()`/`irq_restore_flags()` to preserve the previous interrupt state.</violation>
<violation number="2" location="include/platform/irq.h:333">
P1: Same issue as `irq_system_enter()`: unconditionally enabling interrupts may break critical sections. Use `irq_save_flags()`/`irq_restore_flags()` to preserve the previous interrupt state.</violation>
</file>
<file name="platform/irq-latency.c">
<violation number="1" location="platform/irq-latency.c:171">
P2: Data race: `min` and `max` are read outside the consistency loop, making the snapshot potentially inconsistent. These reads should be inside the do-while loop to ensure all fields are from the same state.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
There was a problem hiding this comment.
@cubic-dev-ai Analysis of the three issues:
irq_system_{enter,exit} macros:
You're correct that these macros unconditionally enable interrupts, which could be problematic. However, git grep confirms these macros are completely unused in the codebase - they're only defined but never called. These appear to be leftover diagnostic utilities. I'll remove them to eliminate the concern.
min/max outside consistency loop:
You're technically correct that min/max could be from a different snapshot than count/sum. However, this is a deliberate design choice:
- All reads are atomic (
__ATOMIC_RELAXED), so there's no memory model UB - The retry loop ensures count/sum consistency because
avg = sum/countdepends on their relationship - min/max are independent statistics that don't depend on count/sum
- This is diagnostic code (KDB display), not safety-critical
- The slight staleness (min/max potentially 1 sample newer/older) is acceptable for human consumption
That said, if you prefer perfect snapshot consistency, I can include min/max in the retry loop. The tradeoff is:
- Current approach: Simpler retry logic, slight potential staleness acceptable for diagnostics
- Including min/max: Perfect consistency, more complex retry condition (retry if count OR min OR max changed)
This implements BASEPRI-based critical sections to enable zero-latency ISRs (priority 0x0-0x2) that can preempt kernel operations. It adds cycle-accurate interrupt latency measurement infrastructure using DWT. - Add BASEPRI primitives (irq_kernel_critical_enter/exit) to irq.h - Convert scheduler critical sections from PRIMASK to BASEPRI - Convert IPC critical sections from PRIMASK to BASEPRI - Add TCB validation in IPC to prevent use-after-free during callbacks - Add irq_system_state tracking and in_isr_context() helper
88de893 to
61e5798
Compare
This implements BASEPRI-based critical sections to enable zero-latency ISRs (priority 0x0-0x2) that can preempt kernel operations. It adds cycle-accurate interrupt latency measurement infrastructure using DWT.