fix(hdd): fork-vendored atad with a retryable probe -- the all-session APA death (HW-confirmed) - #250
Conversation
…n "40x: HardDisk Drive not detected" APA death (HW-confirmed residual) Maintainer HW retest on 8ac163f: APA still empty both views, with the #249 retry loop toasting "40x: HardDisk Drive not detected" -- the diagnostics working as designed and pointing at the ONE place no EE code can reach: the SDK ata_bd driver latches ata_devinfo_init=1 BEFORE probing, and _start() probes at module load. A drive still spinning up at that instant stays "dead" for the whole session; every later sceAtaInit (the EE retry, xhdd's re-probe devctl) is a no-op returning the corpse. This was #249's documented residual; the retest confirmed it, so the driver fix lands. VENDORED: modules/hdd/atad = ps2dev/ps2sdk iop/dev9/atad at pin 426de26cc (2026-07-20), built with the ata_bd flag set, in-tree like xhdd so ALL THREE flavours ship the identical fixed driver. Four local changes, all /* FORK */-marked (full rationale + re-vendor procedure in ORIGIN.txt): 1. sceAtaInit latches init-done only on SUCCESS -> failed probes become retryable. 2. ata_init_devices returns ATA_RES_ERR_NODEV when device 0 answered the probe but failed IDENTIFY (the spin-up race) -- stock returned 0 and latched a dead devinfo. 3. g_ata_bd_connected[] guard: bdm_connect_bd at most once per unit across re-probes (no double massN: after a heal). 4. The g_ata_bd[i].path assignment dropped: the field is absent from the PS2MAXSDK container's bdm.h (appended 2026-02-26, ABI-safe to omit; bdmfs registers "mass" separately and the fork forces massN: -- doctrine). Bonus picked up from the pin: upstream probe fixes ab3c5a883 + 221f6e27a, missing from BOTH pinned containers. HDPro's hdpro_atad stays the SDK prebuilt (own latch; vendor only if an HDPro rig ever reports this). Root Makefile now builds + embeds ours (generated name stays ps2atad.c -> symbol ps2atad_irx -> zero EE-side changes). Known trade-off: a genuinely absent drive pays a real re-probe per EE retry instead of a latched fast NULL -- the retry loop is bounded by design. EXPECTED ON HW: the existing 40x retry loop now HEALS within a few passes once the drive is up -- same rig, same boot, APA populates. If BOTH HDD pages come up after this, "APA and ATA blocking each other" was this same latched corpse wearing two hats. Builds clean (vendored ata_bd.irx 11933 B; make clean + make opl.elf, exit 0). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
📝 WalkthroughWalkthroughA vendored ATAD IOP module was added, including build rules, IRX exports/imports, ATA command execution, controller support, device discovery, sector transfers, security operations, shutdown handling, and BDM integration. The top-level build now produces and consumes this local IRX. ChangesATAD driver
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant IOPModule
participant DEV9AIFDEV9
participant ATADrive
participant BDM
IOPModule->>DEV9AIFDEV9: initialize callbacks and event signaling
IOPModule->>ATADrive: probe and identify device
IOPModule->>ATADrive: issue sector I/O command
ATADrive-->>DEV9AIFDEV9: signal transfer completion
DEV9AIFDEV9-->>IOPModule: deliver completion event
IOPModule->>BDM: register ATA block device
BDM->>IOPModule: request read or write
IOPModule->>ATADrive: perform sector transfer
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![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 integrates a fork-vendored version of the atad (ATA device driver) module into the project, modifying it to allow retryable initialization on failed probes and to prevent duplicate block device connections. The review feedback highlights a critical bug where passing NULL to ata_get_security_status in sceAtaSecurityEraseUnit causes a NULL pointer dereference and crashes the IOP. Additionally, several functions lack validation for the device parameter, which could result in out-of-bounds array accesses on atad_devinfo; adding defensive bounds checks is highly recommended to improve robustness.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| static void ata_get_security_status(int device, ata_devinfo_t *devinfo, u16 *param) | ||
| { | ||
| if (ata_device_identify(device, param) == 0) | ||
| devinfo[device].security_status = param[ATA_ID_SECURITY_STATUS]; | ||
| } |
There was a problem hiding this comment.
Passing NULL as the param argument to ata_get_security_status (as done in sceAtaSecurityEraseUnit at line 1167) will cause a critical NULL pointer dereference. This happens because ata_device_identify will pass NULL down to sceAtaExecCmd and ata_pio_transfer, which attempts to write the PIO data directly to the NULL buffer, crashing the IOP. Additionally, param[ATA_ID_SECURITY_STATUS] will also dereference NULL.
To prevent this, fallback to the static ata_param buffer if param is NULL.
| static void ata_get_security_status(int device, ata_devinfo_t *devinfo, u16 *param) | |
| { | |
| if (ata_device_identify(device, param) == 0) | |
| devinfo[device].security_status = param[ATA_ID_SECURITY_STATUS]; | |
| } | |
| static void ata_get_security_status(int device, ata_devinfo_t *devinfo, u16 *param) | |
| { | |
| u16 *buf = param ? param : ata_param; | |
| if (ata_device_identify(device, buf) == 0) | |
| devinfo[device].security_status = buf[ATA_ID_SECURITY_STATUS]; | |
| } |
| ata_devinfo_t *sceAtaInit(int device) | ||
| { | ||
| if (!ata_devinfo_init) { |
There was a problem hiding this comment.
The device parameter is not validated. If an invalid device index (e.g., negative or >= 2) is passed to sceAtaInit, it will result in an out-of-bounds array access on atad_devinfo when returning &atad_devinfo[device]. Adding a bounds check ensures defensive programming and robustness against invalid inputs.
ata_devinfo_t *sceAtaInit(int device)
{
if (device < 0 || device >= 2)
return NULL;
if (!ata_devinfo_init) {| int ata_device_sector_io64(int device, void *buf, u64 lba, u32 nsectors, int dir) | ||
| { | ||
| USE_SPD_REGS; | ||
| int res = 0, retries; | ||
| u16 sector, lcyl, hcyl, select, command, len; | ||
|
|
||
| while (res == 0 && nsectors > 0) { |
There was a problem hiding this comment.
The device parameter is not validated before being used as an index for atad_devinfo. If an invalid device index is passed, it will cause an out-of-bounds array access. Add a defensive bounds check at the beginning of the function.
int ata_device_sector_io64(int device, void *buf, u64 lba, u32 nsectors, int dir)
{
USE_SPD_REGS;
int res = 0, retries;
u16 sector, lcyl, hcyl, select, command, len;
if (device < 0 || device >= 2)
return ATA_RES_ERR_NODEV;
while (res == 0 && nsectors > 0) {| int sceAtaFlushCache(int device) | ||
| { | ||
| int res; | ||
|
|
||
| if (!(res = sceAtaExecCmd(NULL, 1, 0, 0, 0, 0, 0, (device << 4) & 0xffff, (atad_devinfo[device].lba48 && !ata_dvrp_workaround) ? ATA_C_FLUSH_CACHE_EXT : ATA_C_FLUSH_CACHE))) |
There was a problem hiding this comment.
The device parameter is used directly as an index for atad_devinfo without validation, which can lead to out-of-bounds memory access if an invalid index is passed. Add a defensive bounds check.
int sceAtaFlushCache(int device)
{
int res;
if (device < 0 || device >= 2)
return ATA_RES_ERR_NODEV;
if (!(res = sceAtaExecCmd(NULL, 1, 0, 0, 0, 0, 0, (device << 4) & 0xffff, (atad_devinfo[device].lba48 && !ata_dvrp_workaround) ? ATA_C_FLUSH_CACHE_EXT : ATA_C_FLUSH_CACHE)))| int sceAtaSecuritySetPassword(int device, void *password) | ||
| { | ||
| ata_devinfo_t *devinfo = atad_devinfo; | ||
| u16 *param = ata_param; | ||
| int res; | ||
|
|
||
| if (devinfo[device].security_status & ATA_F_SEC_ENABLED) |
There was a problem hiding this comment.
The device parameter is used directly as an index for devinfo without validation, which can lead to out-of-bounds memory access if an invalid index is passed. Add a defensive bounds check.
| int sceAtaSecuritySetPassword(int device, void *password) | |
| { | |
| ata_devinfo_t *devinfo = atad_devinfo; | |
| u16 *param = ata_param; | |
| int res; | |
| if (devinfo[device].security_status & ATA_F_SEC_ENABLED) | |
| int sceAtaSecuritySetPassword(int device, void *password) | |
| { | |
| ata_devinfo_t *devinfo = atad_devinfo; | |
| u16 *param = ata_param; | |
| int res; | |
| if (device < 0 || device >= 2) | |
| return ATA_RES_ERR_NODEV; | |
| if (devinfo[device].security_status & ATA_F_SEC_ENABLED) |
| int sceAtaSecurityUnLock(int device, void *password) | ||
| { | ||
| ata_devinfo_t *devinfo = atad_devinfo; | ||
| u16 *param = ata_param; | ||
| int res; | ||
|
|
||
| if (!(devinfo[device].security_status & ATA_F_SEC_LOCKED)) |
There was a problem hiding this comment.
The device parameter is used directly as an index for devinfo without validation, which can lead to out-of-bounds memory access if an invalid index is passed. Add a defensive bounds check.
| int sceAtaSecurityUnLock(int device, void *password) | |
| { | |
| ata_devinfo_t *devinfo = atad_devinfo; | |
| u16 *param = ata_param; | |
| int res; | |
| if (!(devinfo[device].security_status & ATA_F_SEC_LOCKED)) | |
| int sceAtaSecurityUnLock(int device, void *password) | |
| { | |
| ata_devinfo_t *devinfo = atad_devinfo; | |
| u16 *param = ata_param; | |
| int res; | |
| if (device < 0 || device >= 2) | |
| return ATA_RES_ERR_NODEV; | |
| if (!(devinfo[device].security_status & ATA_F_SEC_LOCKED)) |
| { | ||
| ata_devinfo_t *devinfo = atad_devinfo; | ||
| int res; | ||
|
|
||
| if (!(devinfo[device].security_status & ATA_F_SEC_ENABLED) || !(devinfo[device].security_status & ATA_F_SEC_LOCKED)) | ||
| return 0; |
There was a problem hiding this comment.
The device parameter is used directly as an index for devinfo without validation, which can lead to out-of-bounds memory access if an invalid index is passed. Add a defensive bounds check.
int sceAtaSecurityEraseUnit(int device)
{
ata_devinfo_t *devinfo = atad_devinfo;
int res;
if (device < 0 || device >= 2)
return ATA_RES_ERR_NODEV;
if (!(devinfo[device].security_status & ATA_F_SEC_ENABLED) || !(devinfo[device].security_status & ATA_F_SEC_LOCKED))…pstream style flattened -- re-vendor procedure in ORIGIN.txt now implies re-formatting too) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
modules/hdd/atad/src/ps2atad.c (1)
1024-1093: 🩺 Stability & Availability | 🔴 Critical | ⚡ Quick winKeep the host-side transfer count wide
sceAtaDmaTransfer()takes a 32-bit sector count, andxhddforwardsbuflen / 512directly, so a 32 MiB LBA48 read can hit this path. Withlenasu16,65536wraps to0,nsectorsnever decreases, and the outer loop spins forever.Proposed fix
- int res = 0, retries; - u16 sector, lcyl, hcyl, select, command, len; + int res = 0, retries; + u16 sector, lcyl, hcyl, select, command; + u32 len; @@ - len = (u16)((nsectors > 65536) ? 65536 : nsectors); /* 0 means 65536 in LBA48 */ + len = (nsectors > 65536) ? 65536 : nsectors; /* 0 means 65536 in LBA48 */🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@modules/hdd/atad/src/ps2atad.c` around lines 1024 - 1093, Use a wide host-side type for the transfer count in ata_device_sector_io64, including len and the nsectors-to-len assignments, so a 65536-sector LBA48 chunk remains 65536 instead of wrapping to zero. Preserve the ATA register field types and ensure pointer, LBA, and nsectors advancement use the full chunk count so the outer loop terminates.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@modules/hdd/atad/src/ps2atad.c`:
- Around line 1024-1093: Use a wide host-side type for the transfer count in
ata_device_sector_io64, including len and the nsectors-to-len assignments, so a
65536-sector LBA48 chunk remains 65536 instead of wrapping to zero. Preserve the
ATA register field types and ensure pointer, LBA, and nsectors advancement use
the full chunk count so the outer loop terminates.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: aa223e2f-74cc-4850-b388-8fac4b93c954
📒 Files selected for processing (1)
modules/hdd/atad/src/ps2atad.c
📜 Review details
⏰ Context from checks skipped due to timeout. (11)
- GitHub Check: build-debug-ps2dev-latest (iopcore_debug)
- GitHub Check: build-debug (ingame_ppctty_debug, :v20250725-2)
- GitHub Check: build-debug (DTL_T10000=1, :v20250725-2)
- GitHub Check: build-debug (ingame_debug, :v20250725-2)
- GitHub Check: build-lang
- GitHub Check: build-debug-ps2dev-latest (DTL_T10000=1)
- GitHub Check: build-debug (iopcore_debug, :v20250725-2)
- GitHub Check: build-debug (ingame_ppctty_debug, :v20250725-2)
- GitHub Check: build-debug (eesio_debug, :v20250725-2)
- GitHub Check: build-debug (DTL_T10000=1, :v20250725-2)
- GitHub Check: build-debug-ps2dev-latest (eesio_debug)
🔇 Additional comments (7)
modules/hdd/atad/src/ps2atad.c (7)
357-369: LGTM!
520-651: LGTM!Also applies to: 654-703, 707-757
1102-1169: LGTM!
1181-1211: LGTM!Also applies to: 1267-1283, 1355-1357
1378-1492: LGTM!
1355-1358: 🩺 Stability & Availability
sceAtaInit()already latches only after successful probing. It returnsNULLuntilata_bus_reset()andata_init_devices()both succeed, then setsata_devinfo_init = 1.> Likely an incorrect or invalid review comment.
1347-1353: 🩺 Stability & AvailabilityNo issue:
bdm_connect_bd()isvoid. There’s no success return to gateg_ata_bd_connected[i]on, and this call pattern matches the other BDM users.> Likely an incorrect or invalid review comment.
- vcdWriteBufFile deleted: the EXISTING vcdSafeWriteFile (free-space check + partial-write cleanup) stages the embedded pair (Gemini -- real, the helper was redundant and weaker). - Staging failures propagate their REAL class: -2 card-full / -3 IO keep their specific toasts; only no-pair/unpack-failure maps to -4 (Gemini -- real, a full card mis-toasted as 'source absent'). - The -4 contract comment in vcdsupport.h updated for the embedded-fallback era (CodeRabbit -- real). #250's Gemini findings (NULL param + device bounds in the vendored atad) are DECLINED under the vendoring doctrine: all target upstream-verbatim code, byte-faithful to the pin except the four documented FORK hunks; no OPL caller passes invalid units, and drive-by hardening of vendored source makes every re-vendor a conflict festival. Recorded in ORIGIN.txt's spirit -- upstream fixes belong upstream. Builds clean. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Gemini's findings (NULL |
1 participant
Your retest confirmed #249's documented residual exactly: the 40x popup is the retry loop running into the SDK atad's permanently latched failed probe — unreachable from EE code. So the driver is now fork-vendored (
modules/hdd/atad, ps2sdk pin426de26cc, built in-tree like xhdd → identical across all three flavours) with four/* FORK */changes: latch-on-success-only, IDENTIFY-race returns an error instead of latching a dead devinfo, idempotent BDM connect across re-probes, and the.pathassignment dropped for PS2MAXSDK header compat (ABI-safe, doctrine-aligned). Full provenance + re-vendor procedure inORIGIN.txt; bonus: two upstream probe fixes both pinned containers lack.Expected on your rig: same boot, the retry loop now heals within a few passes once the drive spins up — APA populates. If both HDD pages come alive, the "blocking each other" was this one corpse wearing two hats.
Builds clean (vendored
ata_bd.irx11,933 B). The BDMA-embed PR (your POPSLoader directive) is next.🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
New Features
Documentation / Build