Skip to content

Upstream #39

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
NathanNeurotic merged 3 commits into from
May 19, 2026
+49 −0
Merged

Upstream #39

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
5331d59
atad: implement readiness checks during init
mystyq May 2, 2026
d3cdb16
fix: formatting
mystyq May 2, 2026
db4cb1f
Merge pull request #1717 from mystyq/atad/init-stable
rickgaiser May 14, 2026
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
49 changes: 49 additions & 0 deletions modules/iopcore/cdvdman/atad.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -120,6 +120,47 @@ static void ata_shutdown_cb(void);

int ata_device_sector_io_internal(int device, void *buf, u64 lba, u32 nsectors, int dir);

/* Wait until the ATA device becomes basically ready for commands.
BUSY must be clear (spin-up / firmware init done).
DRDY is validated later by ata_wait_for_ready() according to ATA spec.
*/
static int ata_wait_for_busy_clear(int timeout)
{
USE_ATA_REGS;

for (int waited = 0; waited < timeout; waited++) {
Comment on lines +128 to +131
Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist Bot May 19, 2026

Choose a reason for hiding this comment

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

medium

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.

Suggested change
{
USE_ATA_REGS;
for (int waited = 0; waited < timeout; waited++) {
{
USE_ATA_REGS;
int waited;
for (waited = 0; waited < timeout; waited++) {

if (!(ata_hwport->r_status & ATA_STAT_BUSY))
return 0;

DelayThread(1000);
}

M_PRINTF("ata_wait_for_busy_clear TIMEOUT after %d ms\n", timeout);
return ATA_RES_ERR_TIMEOUT;
}

static int ata_wait_for_ready(int timeout)
{
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);
}
Comment on lines +143 to +158
Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist Bot May 19, 2026

Choose a reason for hiding this comment

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

high

This block has two issues:

  1. Logic Bug: Per the ATA specification, status register bits (including ERR) are indeterminate when the BUSY bit is set. Checking ATA_STAT_ERR while ATA_STAT_BUSY is active can lead to false failure reports. The error check should only occur when BUSY is 0.
  2. Style: The loop variable waited should 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);
    }


M_PRINTF("ata_wait_for_ready TIMEOUT after %d ms\n", timeout);
return ATA_RES_ERR_TIMEOUT;
}

/* In v1.04, DMA was enabled in ata_set_dir() instead. */
static void ata_pre_dma_cb(int bcr, int dir)
{
Expand Down Expand Up @@ -205,6 +246,14 @@ int atad_start(void)
bdm_connect_bd(&g_ata_bd);
#endif

res = ata_wait_for_busy_clear(30000);
if (res < 0)
goto out;
Comment on lines +250 to +251
Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist Bot May 19, 2026

Choose a reason for hiding this comment

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

critical

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.


res = ata_wait_for_ready(5000);
if (res < 0)
goto out;

res = 0;
M_PRINTF("Driver loaded.\n");
out:
Expand Down