Trying to drain the cd-rom response queue on interrupts. #1939
Conversation
WalkthroughThe CDRomDevice interrupt handler was refactored to process multiple interrupt causes within a single invocation by looping until all causes are cleared. The debug and callback logic was adjusted to operate within this loop, ensuring all pending interrupts are handled before returning. Changes
Poem
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
src/mips/psyqo/src/cdrom-device.cpp (3)
82-86: Consider potential response buffer overflow.The response reading logic limits to 16 bytes, which is good, but processing multiple interrupts in a loop could potentially lead to confusion about which response belongs to which interrupt cause.
The current implementation is functionally correct, but consider documenting the 16-byte limit rationale and ensuring the response is properly scoped to each interrupt iteration.
89-103: Potential callback queue flooding in debug mode.The debug printing logic now runs inside the loop, which could queue multiple debug callbacks during high interrupt activity, potentially overwhelming the callback system.
Consider batching debug output or adding a rate limit:
#ifdef DEBUG_CDROM_RESPONSES if (m_blocking) { ramsyscall_printf("Got CD-Rom response:"); for (auto byte : response) { ramsyscall_printf(" %02x", byte); } syscall_puts("\n"); } else { + static int debugCallbackCount = 0; + if (++debugCallbackCount < 10) { // Rate limit debug callbacks Kernel::queueCallbackFromISR([response]() { ramsyscall_printf("Got CD-Rom response:"); for (auto byte : response) { ramsyscall_printf(" %02x", byte); } syscall_puts("\n"); }); + } } #endif
67-149: Address static analysis complexity concerns.The static analysis correctly identifies increased cyclomatic complexity (17→20) and nesting depth. While the interrupt draining approach is correct, consider extracting helper methods to improve maintainability.
Consider extracting interrupt processing logic into a helper method:
+bool psyqo::CDRomDevice::processInterrupt(uint8_t cause) { + // Move lines 73-148 into this helper method + // Return true if callback was called (to break the loop) +} + void psyqo::CDRomDevice::irq() { bool ranOnce = false; while (true) { uint8_t cause = Hardware::CDRom::Cause; if (ranOnce && ((cause & 15) == 0)) break; ranOnce = true; Kernel::assert(m_action != nullptr, "CDRomDevice::irq() called with no action - spurious interrupt?"); + if (processInterrupt(cause)) break; - // Remove lines 73-148 and move to processInterrupt helper } }🧰 Tools
🪛 GitHub Check: CodeScene Cloud Delta Analysis (main)
[warning] 67-147: ❌ Getting worse: Complex Method
psyqo::CDRomDevice::irq increases in cyclomatic complexity from 17 to 20, threshold = 9. This function has many conditional statements (e.g. if, for, while), leading to lower code health. Avoid adding more conditionals and code to it without refactoring.
[warning] 67-147: ❌ Getting worse: Bumpy Road Ahead
psyqo::CDRomDevice::irq increases from 4 to 5 logical blocks with deeply nested code, threshold is one single block per function. The Bumpy Road code smell is a function that contains multiple chunks of nested conditional logic. The deeper the nesting and the more bumps, the lower the code health.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/mips/psyqo/src/cdrom-device.cpp(1 hunks)
🧰 Additional context used
🪛 GitHub Check: CodeScene Cloud Delta Analysis (main)
src/mips/psyqo/src/cdrom-device.cpp
[warning] 67-147: ❌ Getting worse: Complex Method
psyqo::CDRomDevice::irq increases in cyclomatic complexity from 17 to 20, threshold = 9. This function has many conditional statements (e.g. if, for, while), leading to lower code health. Avoid adding more conditionals and code to it without refactoring.
[warning] 67-147: ❌ Getting worse: Bumpy Road Ahead
psyqo::CDRomDevice::irq increases from 4 to 5 logical blocks with deeply nested code, threshold is one single block per function. The Bumpy Road code smell is a function that contains multiple chunks of nested conditional logic. The deeper the nesting and the more bumps, the lower the code health.
⏰ Context from checks skipped due to timeout of 90000ms (12)
- GitHub Check: pcsx-redux (x86_64-linux)
- GitHub Check: pcsx-redux (aarch64-linux)
- GitHub Check: build-openbios
- GitHub Check: macos-build-and-test-toolchain
- GitHub Check: cross-arm64
- GitHub Check: asan
- GitHub Check: coverage
- GitHub Check: build
- GitHub Check: toolchain
- GitHub Check: aur-build
- GitHub Check: pcsx-redux (aarch64-linux)
- GitHub Check: pcsx-redux (x86_64-linux)
🔇 Additional comments (3)
src/mips/psyqo/src/cdrom-device.cpp (3)
73-80: LGTM: Interrupt cause clearing logic is correct.The interrupt cause clearing has been correctly moved inside the loop and maintains the same bit manipulation logic as before.
106-137: LGTM: Switch statement logic preserved correctly.The switch statement handling different interrupt causes has been correctly moved inside the loop without changing the core logic. The error handling for cause 5 is properly implemented.
139-148: LGTM: Callback handling maintains proper state management.The callback handling correctly clears
m_actionand manages blocking vs non-blocking execution paths. The atomic fence usage is appropriate for the non-blocking case.
| bool ranOnce = false; | ||
| while (true) { | ||
| uint8_t cause = Hardware::CDRom::Cause; | ||
| if (ranOnce && ((cause & 15) == 0)) break; | ||
| ranOnce = true; | ||
| Kernel::assert(m_action != nullptr, "CDRomDevice::irq() called with no action - spurious interrupt?"); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Verify infinite loop protection and consider adding safety limits.
The loop control logic correctly implements interrupt draining, but lacks protection against potential infinite loops if hardware misbehaves.
Consider adding a safety counter to prevent infinite loops:
void psyqo::CDRomDevice::irq() {
bool ranOnce = false;
+ int maxIterations = 16; // Safety limit
while (true) {
uint8_t cause = Hardware::CDRom::Cause;
if (ranOnce && ((cause & 15) == 0)) break;
+ if (--maxIterations <= 0) {
+ Kernel::abort("CDRomDevice::irq() exceeded maximum iterations - hardware malfunction?");
+ break;
+ }
ranOnce = true;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| bool ranOnce = false; | |
| while (true) { | |
| uint8_t cause = Hardware::CDRom::Cause; | |
| if (ranOnce && ((cause & 15) == 0)) break; | |
| ranOnce = true; | |
| Kernel::assert(m_action != nullptr, "CDRomDevice::irq() called with no action - spurious interrupt?"); | |
| void psyqo::CDRomDevice::irq() { | |
| bool ranOnce = false; | |
| + int maxIterations = 16; // Safety limit | |
| while (true) { | |
| uint8_t cause = Hardware::CDRom::Cause; | |
| if (ranOnce && ((cause & 15) == 0)) break; | |
| + if (--maxIterations <= 0) { | |
| + Kernel::abort("CDRomDevice::irq() exceeded maximum iterations - hardware malfunction?"); | |
| + break; | |
| + } | |
| ranOnce = true; | |
| Kernel::assert(m_action != nullptr, "CDRomDevice::irq() called with no action - spurious interrupt?"); |
🧰 Tools
🪛 GitHub Check: CodeScene Cloud Delta Analysis (main)
[warning] 67-147: ❌ Getting worse: Complex Method
psyqo::CDRomDevice::irq increases in cyclomatic complexity from 17 to 20, threshold = 9. This function has many conditional statements (e.g. if, for, while), leading to lower code health. Avoid adding more conditionals and code to it without refactoring.
[warning] 67-147: ❌ Getting worse: Bumpy Road Ahead
psyqo::CDRomDevice::irq increases from 4 to 5 logical blocks with deeply nested code, threshold is one single block per function. The Bumpy Road code smell is a function that contains multiple chunks of nested conditional logic. The deeper the nesting and the more bumps, the lower the code health.
🤖 Prompt for AI Agents
In src/mips/psyqo/src/cdrom-device.cpp around lines 67 to 72, the infinite loop
draining interrupts lacks a safety limit, risking an infinite loop if hardware
misbehaves. Add a counter variable initialized before the loop and increment it
each iteration; break the loop if the counter exceeds a reasonable threshold to
ensure the loop always terminates even in error conditions.
1 participant
No description provided.