Skip to content

Trying to drain the cd-rom response queue on interrupts. #1939

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.

Sign up for GitHub

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

Merged
nicolasnoble merged 1 commit into from
May 31, 2025
Merged

Trying to drain the cd-rom response queue on interrupts. #1939

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
127 changes: 66 additions & 61 deletions src/mips/psyqo/src/cdrom-device.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,82 +64,87 @@
}

void psyqo::CDRomDevice::irq() {
Kernel::assert(m_action != nullptr, "CDRomDevice::irq() called with no action - spurious interrupt?");
uint8_t cause = Hardware::CDRom::Cause;

if (cause & 7) {
Hardware::CDRom::Cause = 7;
}
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?");
Comment on lines +67 to +72
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot May 31, 2025

Choose a reason for hiding this comment

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

🛠️ 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.

Suggested change
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.


if (cause & 7) {
Hardware::CDRom::Cause = 7;
}

if (cause & 0x18) {
Hardware::CDRom::Cause = 0x18;
}
if (cause & 0x18) {
Hardware::CDRom::Cause = 0x18;
}

bool callCallback = false;
Response response;
while ((Hardware::CDRom::Ctrl.access() & 0x20) && (response.size() < 16)) {
response.push_back(Hardware::CDRom::Response);
}
bool callCallback = false;
Response response;
while ((Hardware::CDRom::Ctrl.access() & 0x20) && (response.size() < 16)) {
response.push_back(Hardware::CDRom::Response);
}

#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 {
Kernel::queueCallbackFromISR([response]() {
if (m_blocking) {
ramsyscall_printf("Got CD-Rom response:");
for (auto byte : response) {
ramsyscall_printf(" %02x", byte);
}
syscall_puts("\n");
});
}
#endif

switch (cause & 7) {
case 1:
callCallback = m_action->dataReady(response);
break;
case 2:
callCallback = m_action->complete(response);
break;
case 3:
callCallback = m_action->acknowledge(response);
break;
case 4:
callCallback = m_action->end(response);
break;
case 5: {
m_success = false;
callCallback = true;
#ifdef DEBUG_CDROM_ERRORS
m_callback = [callback = eastl::move(m_callback), name = m_action->name(),
response = eastl::move(response)](bool) {
ramsyscall_printf("Got CD-Rom error during action %s:", name);
} else {
Kernel::queueCallbackFromISR([response]() {
ramsyscall_printf("Got CD-Rom response:");
for (auto byte : response) {
ramsyscall_printf(" %02x", byte);
}
syscall_puts("\n");
callback(false);
};
});
}
#endif
} break;
default:
Kernel::abort("CDRomDevice::irq() invoked with unknown cause");
break;
}

if (callCallback) {
Kernel::assert(!!m_callback, "Wrong CDRomDevice state");
m_action = nullptr;
if (m_blocking) {
actionComplete();
} else {
eastl::atomic_signal_fence(eastl::memory_order_acquire);
Kernel::queueCallbackFromISR([this]() { actionComplete(); });
switch (cause & 7) {
case 1:
callCallback = m_action->dataReady(response);
break;
case 2:
callCallback = m_action->complete(response);
break;
case 3:
callCallback = m_action->acknowledge(response);
break;
case 4:
callCallback = m_action->end(response);
break;
case 5: {
m_success = false;
callCallback = true;
#ifdef DEBUG_CDROM_ERRORS
m_callback = [callback = eastl::move(m_callback), name = m_action->name(),
response = eastl::move(response)](bool) {
ramsyscall_printf("Got CD-Rom error during action %s:", name);
for (auto byte : response) {
ramsyscall_printf(" %02x", byte);
}
syscall_puts("\n");
callback(false);
};
#endif
} break;
default:
Kernel::abort("CDRomDevice::irq() invoked with unknown cause");
break;
}

if (callCallback) {
Kernel::assert(!!m_callback, "Wrong CDRomDevice state");
m_action = nullptr;
if (m_blocking) {
actionComplete();
} else {
eastl::atomic_signal_fence(eastl::memory_order_acquire);
Kernel::queueCallbackFromISR([this]() { actionComplete(); });
}

Check warning on line 147 in src/mips/psyqo/src/cdrom-device.cpp

View check run for this annotation

CodeScene Delta Analysis / CodeScene Cloud Delta Analysis (main)

❌ 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.

Check warning on line 147 in src/mips/psyqo/src/cdrom-device.cpp

View check run for this annotation

CodeScene Delta Analysis / CodeScene Cloud Delta Analysis (main)

❌ 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.
}
}
}
Expand Down
Loading