Fix: Reinitialize Device after Sleep / Subsequent mounts#47
Merged
Conversation
alicankaralar
force-pushed
the
feat/ohci-power-management
branch
from
67d0adb to
4cbb12a
Compare
alicankaralar
changed the title
Owner
|
@copilot resolve the merge conflicts on this branch. |
There was a problem hiding this comment.
Pull request overview
This PR addresses a DriverKit power-management/lifecycle bug where the OHCI controller loses programmed state across sleep/power transitions, preventing devices from re-mounting after wake or subsequent interface power cycles. It introduces a sleep/wake path that quiesces and resets the driver runtime on sleep, then rebuilds it on wake (with a post-wake verification pass).
Changes:
- Added
SetPowerStatehandling to quiesce + reset runtime on sleep and fully rebuild runtime on wake. - Refactored runtime bring-up/teardown into
StartRuntime()andQuiesceRuntime()to share logic across Start/Stop and PM callbacks. - Added a delayed “wake verification” mechanism intended to detect dark-wake cases where interrupts/bus reset completion don’t arrive and retry rebuild.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| ASFWDriver/ASFWDriver.iig | Declares PM entrypoint (SetPowerState) and new runtime lifecycle helpers + ivars for PM state tracking. |
| ASFWDriver/ASFWDriver.cpp | Implements sleep/wake runtime teardown/rebuild and post-wake verification/retry logic; gates RegisterService() to run once. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+487
to
+488
| ivars->context->deps.scheduler->DispatchAsyncAfter( | ||
| kWakeVerifyDelayNs, [this] { VerifyWakeRuntime(1); }); |
Contributor
Author
There was a problem hiding this comment.
addressed in the last commit
Comment on lines
+549
to
+552
| const uint64_t next = attempt + 1; | ||
| ivars->context->deps.scheduler->DispatchAsyncAfter( | ||
| kWakeVerifyDelayNs, [this, next] { VerifyWakeRuntime(next); }); | ||
| } |
Contributor
Author
There was a problem hiding this comment.
addressed in the next commit
Comment on lines
+434
to
+435
| static constexpr uint64_t kWakeVerifyDelayNs = 3'000'000'000ull; | ||
| static constexpr uint64_t kWakeVerifyMaxAttempts = 5; |
Contributor
Author
There was a problem hiding this comment.
addressed in the next commit
The OHCI controller loses its programmed state when the system sleeps (IntMask, LinkControl, PHY link). The dext never handled power events, so after wake it stayed alive but deaf: no bus-reset interrupts, so a device plugged after any sleep never mounted until the adapter was re-plugged or the machine rebooted (field failure 2026-07-05: Venice invisible after an overnight idle; full recovery only via TB re-enumeration). Override IOService::SetPowerState (matched services are always in the PM tree). Sleep quiesces and resets the runtime while the controller still answers MMIO; wake rebuilds it via the same bring-up as Start — full OHCI re-init ending in a forced bus reset, after which normal discovery re-publishes devices. This mirrors Linux firewire-ohci (pci_suspend = software_reset; pci_resume = the same ohci_enable as cold probe, ohci.c:3762) and Apple IOFireWireController::setPowerState (quiesce + gate on sleep, re-init + UpdateROM + resetBus on wake). Start/Stop bodies are factored into StartRuntime/QuiesceRuntime and shared with the power path; RegisterService() stays once-per-instance. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… off SetPowerState(0) fired 2ms after the last audio nub terminated (device switched off): with no child expressing a power demand, idle power management powered the controller domain down, the sleep path tore down the runtime, and no SetPowerState(On) ever followed — nothing demanded power again, so the driver was dead until reboot (field trace 2026-07-05 13:41). A bus controller must stay fully powered with no devices attached: plug detection requires a programmed, interrupting controller. Declare the desire with ChangePowerState(kIOServicePowerCapabilityOn) at StartRuntime; capability 0 then only arrives for real system sleep, and wake restores the declared desire via SetPowerState(On). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ld-driven power-down HW test showed SetPowerState(0) still arrives 1ms after the last audio nub terminates, even with ChangePowerState(On) pinned: the audio driver matched on our nub is a PM-tree child, and our power state was still governed by its demand. Per IOService.iig, SetPowerOverride makes the state governed solely by our own desire — children can come and go, capability 0 then means real system sleep only. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…oin happens after Start returns HW test on b10cc02 showed SetPowerOverride(true) returning kIOReturnError (0xe00002bc) and the child-driven power-down still firing. Root cause from xnu IOUserServer.cpp: serviceStarted -> serviceJoinPMTree runs only AFTER the dext's Start() returns, so PM calls made during Start() hit an uninitialized PM object — powerOverrideOnPriv returns IOPMNotYetInitialized, and ChangePowerState_Impl silently discards the same failure (returns success). Move ChangePowerState(On) + SetPowerOverride(true) into the SetPowerState(On) callback, which the kernel delivers right after the PM join. Same log also confirmed the wake-rebuild path works end to end: the domain repowered at 18:54:59, rebuild forced a bus reset, and the device mounted unattended. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…retries Third HW test: device off/on now survives (SetPowerOverride holds — no power-down on nub termination), but after real system sleep the wake rebuild ran during dark wake (SetPowerState(On) at T+4s, full wake at T+7s) and completed cleanly over MMIO — PHY reads, version register, IBR write all fine — yet the forced bus reset never delivered an interrupt: upstream DMA/MSI state was evidently lost between dark wake and full wake, leaving a deaf controller. Every wake rebuild ends in a forced bus reset, and BusResetCoordinator's resetCount only advances through the full interrupt path, so a completed reset observed shortly after the rebuild proves the controller is alive end to end. VerifyWakeRuntime checks that 3s after wake (plus linkEnable/MMIO readback to classify the failure mode) and rebuilds the runtime again if the check fails, up to 5 attempts. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
alicankaralar
force-pushed
the
feat/ohci-power-management
branch
from
4cbb12a to
5d46c86
Compare
…no queue-blocking sleep Review findings on the wake self-heal path: - Both delayed VerifyWakeRuntime dispatches captured `this` in a lambda; a Stop/free before the callback ran would dereference a freed service. - Scheduler::DispatchAsyncAfter implements delays as IOSleep *on* the bound queue, so each 3 s verify attempt froze the driver work queue (up to 15 s over 5 retries), starving the very interrupt path the check verifies. Replace both call sites with ScheduleWakeVerify(attempt): an IOTimerDispatchSource + OSAction targeting WakeVerifyTimerFired, the same pattern as the async watchdog and SBP-2 session timers. The OSAction retains the service, so a pending callback cannot outlive the driver; Stop disables and releases the timer (disable-then-release, no Cancel — see WatchdogCoordinator::Stop) which also breaks the action→service retain cycle. The timer binds to ctx.workQueue, which is the service default queue, so it stays valid across the sleep/wake runtime rebuilds it supervises and serializes with Start/Stop/ SetPowerState. Also document the blocking behavior on Scheduler::DispatchAsyncAfter for the remaining (short-delay) callers. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Contributor
Author
resolved |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
There is a bug where after a power cycle on the interface, the device would no longer mount on the OS side. Also, after a sleep cycle on Mac, the driver would not mount the interface after turning it on. This PR addresses this issue.