Skip to content

fix(hdd): un-poison the APA module-load latch + surface silent probe failures (Vapor) #241

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 2 commits into from
Jul 20, 2026
Merged

fix(hdd): un-poison the APA module-load latch + surface silent probe failures (Vapor) #241

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e1f04ce
fix(hdd): un-poison the APA module-load latch + surface silent probe …
NathanNeurotic Jul 20, 2026
df7e15e
fix(hdd): evaluate the loader result exactly once (CodeRabbit review …
NathanNeurotic Jul 20, 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
17 changes: 17 additions & 0 deletions include/hddsupport.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,12 @@ typedef enum {
HDD_LOADMODULES_STATUS_COUNT,
} hdd_loadmodules_status;

// See hddLoadModulesReady() below the prototypes -- the single evaluate-once way to ask "is the ATA
// stack actually resident?". Callers used to test `hddLoadModules() >= 0`, which also accepted
// BUSYLOADING(2) -- returned for the whole session after a FAILED first load consumed the one-shot
// count -- so "nothing is loaded" read as success and the APA page sat silently empty under both
// Auto and Manual (Vapor). Pair with the retryable-failure change in hddLoadModules.

int hddCheck(void);
u32 hddGetTotalSectors(void);
int hddIs48bit(void);
Expand All @@ -91,6 +97,17 @@ void hddVcdInvalidateCache(void);
void hddInit(item_list_t *itemList);
item_list_t *hddGetObject(int initOnly);
int hddLoadModules(void);

// Load (or confirm) the ATA stack and report residency, evaluating hddLoadModules EXACTLY ONCE.
// A function, not a macro taking the call as its argument: the earlier HDD_LOADMODULES_OK(
// hddLoadModules()) macro double-evaluated on any non-NOERROR first result -- and with the
// retryable-failure change, a FAILED load would have immediately re-run the whole load (second
// dev9 init + second error toast) inside one condition (CodeRabbit review of #241; vetted).
static inline int hddLoadModulesReady(void)
{
int r = hddLoadModules();
return r == HDD_LOADMODULES_STATUS_NOERROR || r == HDD_LOADMODULES_STATUS_ALREADYLOADED;
}
void hddLoadSupportModules(void);
void hddLaunchGame(item_list_t *itemList, int id, config_set_t *configSet);
int hddIsPresent();
Expand Down
4 changes: 2 additions & 2 deletions src/bdmsupport.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -353,7 +353,7 @@ static void bdmLoadBlockDeviceModules(void)
// Only mark loaded on success -- a failure (no HDD/interface) must not
// suppress future retries via bdmShouldQueueModuleLoad() (matches the
// conditional pattern used for USB/MX4SIO and opl.c's hddLoadModules gate).
if (hddLoadModules() >= 0)
if (hddLoadModulesReady())
hddModLoaded = 1;
}

Expand Down Expand Up @@ -1660,7 +1660,7 @@ static int bdmEnsureTransportLoaded(int bdmType)
}
return iLinkModLoaded;
case BDM_TYPE_ATA:
if (!hddModLoaded && hddLoadModules() >= 0)
if (!hddModLoaded && hddLoadModulesReady())
hddModLoaded = 1;
return hddModLoaded;
default:
Expand Down
21 changes: 19 additions & 2 deletions src/hddsupport.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -224,6 +224,15 @@ int hddLoadModules(void)
LOG("HDD: No HardDisk Drive detected.\n");
setErrorMessageWithCode(_STR_HDD_NOT_CONNECTED_ERROR, ERROR_HDD_IF_NOT_DETECTED);
retStatus = HDD_LOADMODULES_STATUS_ERROR;
// Make the failure RETRYABLE. Leaving the count consumed turned one bad first probe into a
// whole-session poison: every later call took the else branch and returned BUSYLOADING(2),
// which the >= 0 caller tests read as SUCCESS while nothing was loaded -- so the APA page
// sat silently empty under BOTH Auto and Manual (Vapor's report; the drive lists fine in
// wOPL/upstream because their earliest callers run at tab entry, when the drive is ready --
// our fork adds boot-time callers like bdmResolveBootDir's ATA escalation, seconds after
// power-on). sysInitDev9/sysLoadModuleBuffer are safe to re-run; a later caller (e.g. the
// HDD tab) now gets a real second attempt instead of a poisoned latch.
hddModulesLoadCount = 0;
Comment on lines +227 to +235

@coderabbitai coderabbitai Bot Jul 20, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🩺 Stability & Availability | 🟠 Major | ⚡ Quick win

Include XHDD failures in the retry decision.

The reset is reached only when the ATAD result in retLoadModule fails, but the xhdd_irx load result is discarded. If ATAD succeeds and XHDD fails, the success branch sets hddModulesLoaded = 1, so later callers return success/ALREADYLOADED and never retry even though xhdd0: is unusable. Check every required module load before marking the stack loaded.

🤖 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 `@src/hddsupport.c` around lines 227 - 235, Update the module-load success
logic around retLoadModule so both the ATAD and xhdd_irx load results must
succeed before setting hddModulesLoaded or returning success/ALREADYLOADED.
Preserve the retry reset path for any required-module failure, ensuring an XHDD
failure leaves hddModulesLoadCount retryable instead of marking the HDD stack
loaded.

} else {
retStatus = HDD_LOADMODULES_STATUS_NOERROR;
hddModulesLoaded = 1;
Expand Down Expand Up @@ -313,9 +322,17 @@ void hddLoadSupportModules(void)

// Check if the drive contains MBR/GPT partition data before we load the APA/PFS modules. If the drive is not
// APA then loading the APA irx modules can corrupt the drive as it will try to write APA partition data.
if (hddDetectNonSonyFileSystem() != 0) {
int nonSony = hddDetectNonSonyFileSystem();
if (nonSony != 0) {
// Drive is MBR/GPT style, or unknown, bail out or risk corrupting the drive.
LOG("HDDSUPPORT LoadSupportModules bailing out early...\n");
LOG("HDDSUPPORT LoadSupportModules bailing out early (%d)...\n", nonSony);
// A PROBE FAILURE (-1: the xhdd0: partition-sector devctl errored -- drive not ready, module
// missing, transient bus fault) was completely SILENT: no error box, no list, an APA page that
// just looks like a dead drive for the whole session. Surface it. The 1 (genuine MBR/GPT/exFAT)
// branch stays silent on purpose -- that is the NORMAL coexistence case for BDM-HDD users and
// must not toast at every boot.
if (nonSony < 0)
setErrorMessageWithCode(_STR_HDD_NOT_CONNECTED_ERROR, ERROR_HDD_NOT_DETECTED);
Comment on lines +325 to +335

@coderabbitai coderabbitai Bot Jul 20, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Distinguish probe errors from unknown partition data.

hddDetectNonSonyFileSystem() returns -1 both for allocation/devctl failures and for merely unrecognized partition data (Lines [284-288]). This new < 0 check therefore reports _STR_HDD_NOT_CONNECTED_ERROR for connected disks with an unknown layout, despite the comment describing only probe failures. Return distinct statuses and surface the error only for actual probe failures.

🤖 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 `@src/hddsupport.c` around lines 325 - 335, Update hddDetectNonSonyFileSystem()
to return distinct status values for probe failures versus successfully detected
but unrecognized partition data. Adjust the caller in LoadSupportModules so
setErrorMessageWithCode is invoked only for the probe-failure status, while
unknown layouts retain the existing silent behavior.

return;
}

Expand Down
10 changes: 8 additions & 2 deletions src/opl.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1313,7 +1313,7 @@ static int checkLoadConfigBDMHDD(int types)
int value;

// Bounded wait so BDM-on-HDD can be detected without long black-screen stalls.
if (hddLoadModules() >= 0 && bdmHDDIsPresent(500)) {
if (hddLoadModulesReady() && bdmHDDIsPresent(500)) {
if (bdmFindPartition(path, CONFIG_OPL_FILENAME, 0) || bdmFindPartition(path, CONFIG_OPL_FILENAME_LEGACY, 0)) {
configEnd();
configInit(path);
Expand Down Expand Up @@ -1580,6 +1580,12 @@ static void _loadConfig()
configGetInt(configOPL, CONFIG_OPL_ENABLE_ART_TAR, &gEnableArtTar);
configGetInt(configOPL, CONFIG_OPL_WIDESCREEN, &gWideScreen);
configGetInt(configOPL, CONFIG_OPL_DEFAULT_GAME_VIEW, &gDefaultGameView);
// Clamp: an out-of-enum value (hand-edited/corrupt config) is UNRECOVERABLE on-console --
// vcdViewActive() treats anything but BOTH as a lock, the L3 toggle goes inert and its hint
// is hidden, so every VCD-capable page silently serves one (possibly empty) list with no
// way back. The coverflow ints just below have always been clamped; this one was not.
if (gDefaultGameView < GAME_VIEW_BOTH || gDefaultGameView > GAME_VIEW_VCD)
gDefaultGameView = GAME_VIEW_BOTH;
// A boot default-view locked to one type (VCD or ISO) must force the same one-shot
// rescan the settings dialog does on a view change (gui.c). Without it, vcdViewActive()
// short-circuits mmce/bdm/hdd/eth NeedsUpdate before the initial-scan trigger and the
Expand Down Expand Up @@ -1895,7 +1901,7 @@ static int trySaveConfigBDMHDD(int types)
char path[64];

// Bounded wait so save can target BDM-on-HDD without long stalls.
if (hddLoadModules() >= 0 && bdmHDDIsPresent(500)) {
if (hddLoadModulesReady() && bdmHDDIsPresent(500)) {
if (bdmFindPartition(path, CONFIG_OPL_FILENAME, 1)) {
configSetMove(path);
return configWriteMulti(types);
Expand Down