Upstream#39
Conversation
atad: implement readiness checks during init
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces ATA device readiness checks during driver initialization by adding ata_wait_for_busy_clear and ata_wait_for_ready functions. These are called within atad_start to ensure the hardware is ready before proceeding. Feedback identifies a critical issue where failing these checks leads to an error return without cleaning up previously registered resources (such as interrupt handlers and BDM connections), which could cause system crashes. Additionally, the review points out a logic error regarding the ATA specification for status register bits and requests that variable declarations be moved to the top of functions to maintain C89 compatibility.
| if (res < 0) | ||
| goto out; |
There was a problem hiding this comment.
This error path is critical. If atad_start returns a negative error code, the module may be unloaded, but several resources (interrupt handlers, power-off handler, semaphore, event flag, and BDM connection) have already been registered and are not cleaned up. This will cause a system crash when an interrupt occurs or during shutdown as the kernel attempts to call code in freed memory. Consider moving these readiness checks to the beginning of the function before resource allocation, or implementing a proper cleanup sequence.
| { | ||
| USE_ATA_REGS; | ||
|
|
||
| for (int waited = 0; waited < timeout; waited++) { | ||
| u8 status = ata_hwport->r_status; | ||
|
|
||
| if (!(status & ATA_STAT_BUSY) && (status & ATA_STAT_READY)) | ||
| return 0; | ||
|
|
||
| if (status & ATA_STAT_ERR) { | ||
| M_PRINTF("ata_wait_for_ready ERR: status=0x%02x err=0x%02x\n", status, sceAtaGetError()); | ||
| return ATA_RES_ERR_IO; | ||
| } | ||
|
|
||
| DelayThread(1000); | ||
| } |
There was a problem hiding this comment.
This block has two issues:
- Logic Bug: Per the ATA specification, status register bits (including
ERR) are indeterminate when theBUSYbit is set. CheckingATA_STAT_ERRwhileATA_STAT_BUSYis active can lead to false failure reports. The error check should only occur whenBUSYis 0. - Style: The loop variable
waitedshould be declared at the top of the function to match the project's C89 style.
{
USE_ATA_REGS;
int waited;
u8 status;
for (waited = 0; waited < timeout; waited++) {
status = ata_hwport->r_status;
if (!(status & ATA_STAT_BUSY)) {
if (status & ATA_STAT_READY)
return 0;
if (status & ATA_STAT_ERR) {
M_PRINTF("ata_wait_for_ready ERR: status=0x%02x err=0x%02x\n", status, sceAtaGetError());
return ATA_RES_ERR_IO;
}
}
DelayThread(1000);
}| { | ||
| USE_ATA_REGS; | ||
|
|
||
| for (int waited = 0; waited < timeout; waited++) { |
There was a problem hiding this comment.
The variable waited is declared inside the for loop, which is a C99 feature. To maintain consistency with the C89 style used throughout the rest of this file, please move the declaration to the top of the function.
| { | |
| USE_ATA_REGS; | |
| for (int waited = 0; waited < timeout; waited++) { | |
| { | |
| USE_ATA_REGS; | |
| int waited; | |
| for (waited = 0; waited < timeout; waited++) { |
3 participants
Pull Request
Summary
Provide a clear and concise description of the change.
Type of Change
Select all that apply:
Related Issues
Link related issues using:
Closes #
Fixes #
Related to #
Scope
If this PR modifies multiple subsystems, explain why that is necessary:
Technical Details
Describe:
If modifying core logic, explain potential side effects.
Testing
Describe how you tested this change:
Breaking Changes
If this is a breaking change, describe:
If not applicable, state: "No breaking changes."
Security Considerations
Does this change affect:
If yes, explain impact.
Checklist
Additional Notes
Provide any additional context here.