Skip to content

AHCI: Resolve port initialization conflict on boot vs S4 resume#5853

Closed
G0retZ wants to merge 1 commit intogenodelabs:masterfrom
G0retZ:master
Closed

AHCI: Resolve port initialization conflict on boot vs S4 resume#5853
G0retZ wants to merge 1 commit intogenodelabs:masterfrom
G0retZ:master

Conversation

@G0retZ
Copy link
Copy Markdown

@G0retZ G0retZ commented Apr 13, 2026

This PR is vibe-coded with thorough supervision and sanity check.
Though I'm not an expert in C++ nor in drivers development.

Here is the summary of the reasoning behind the changes produced by AI:

On systems with an IOMMU, SATA ports were logged as "off (ATA)" at boot
despite the BIOS having successfully initialized the devices.

Root cause

Port::reinit() unconditionally called reset() (COMRESET via PxSCTL.Det)
before enable() and before setup_memory() had mapped a Genode-owned FIS
receive buffer into PxFB.

At that point PxFB still held the BIOS-allocated physical address and FRE
(FIS Receive Enable) was left enabled by the BIOS. The device responded to
COMRESET by posting a D2H Register FIS. The HBA attempted a DMA write to
the BIOS address, which the IOMMU blocked. As a result FR (FIS Receive
Running) never cleared. The subsequent setup_memory() → wait_for(FR=0)
timed out with Polling_timeout, which propagated out of _scan_ports and
left the port disabled.

Fix

Move the COMRESET out of the unconditional preamble and instead call
enable() first:

  • On boot and S3 resume the SATA link is already established by the BIOS /
    powered controller. enable() succeeds immediately and no COMRESET is
    needed before init().

  • On S4 resume (full power loss) all port registers including PxSSTS reset
    to zero, so enable() returns false. In that case reset() is issued to
    re-establish the link. After S4, Cmd::St and FRE are guaranteed 0 by
    power loss, so the COMRESET cannot trigger an IOMMU-blocked DMA, and
    setup_memory() inside the subsequent init() will find FR=0 immediately.

The is_resume parameter (defaulting to false) distinguishes the two
failure meanings of enable() returning false: an empty port at boot (throw
immediately, DMA registers never touched) versus a link-down condition at
S4 resume (reset and retry).

The resume call site in main.cc is updated to pass is_resume=true.

Tested on my system building Sculpt from the current master branch and now I have access to all of my drives.
20260413_112232
20260413_112216

On systems with an IOMMU, SATA ports were logged as "off (ATA)" at boot
despite the BIOS having successfully initialized the devices.
The boot and resume followed the same logic despite of different
actual state of ports leading to non-working state for cold boot or S3
case.

In order to fix that we need to add a bool parameter into reinit()
call to alter the reinit logic between boot, S3 and S4 resumes.

With this in place SATA disks get initialized properly on boot and S3
resume and should be also initialized properly on S4 resumes.
@G0retZ
Copy link
Copy Markdown
Author

G0retZ commented Apr 13, 2026

This addresses the issue #5088

@ssumpf
Copy link
Copy Markdown
Member

ssumpf commented Apr 13, 2026

@G0retZ: 4609a4c does - as observed by @cproc in #5088 - not call the Port reset during AHCI driver startup. This leaves the question of what will happen in case a BIOS doesn't hand over a half configured controller or in case there is no BIOS (think ARM based SoCs)? I would try to clear Cmd::St (unconditionally) and than clear Cmd::Fre (as @cproc suggested) before the Port reset because having FIS receive running during reset is definitely not good. I will send a sketch commit for testing to #5088 and would appreciate if you could test this. Also I would like to continue the discussion in #5088.

@nfeske
Copy link
Copy Markdown
Member

nfeske commented Apr 14, 2026

Thanks @G0retZ for the continued investigation. Happy to see it getting resolved. Also thanks @ssumpf for responding.

This PR is vibe-coded with thorough supervision and sanity check.

Thank you for being transparent about it.

As a note of caution, Genode is not open for AI-generated code contributions due to the legal uncertainties of such code. In order to incorporate code into genuine Genode components (those that are marked by the license headers as being part of Genode) we must ensure that such code does not violate rights of third parties. On this account, the Genode contributor's agreement states "You certify and warrant that your Contributions to Genode Labs’s Products do not violate the intellectual property rights of third parties [...]". This cannot be assumed for code generated by AI models without legal records of their training material. As far as I know, AI service providers do not provide such evidence.

@G0retZ
Copy link
Copy Markdown
Author

G0retZ commented Apr 14, 2026

Thank you for being transparent about it.

As a note of caution, Genode is not open for AI-generated code contributions due to the legal uncertainties of such code. In order to incorporate code into genuine Genode components (those that are marked by the license headers as being part of Genode) we must ensure that such code does not violate rights of third parties. On this account, the Genode contributor's agreement states "You certify and warrant that your Contributions to Genode Labs’s Products do not violate the intellectual property rights of third parties [...]". This cannot be assumed for code generated by AI models without legal records of their training material. As far as I know, AI service providers do not provide such evidence.

Ok, I definitely need take this into account next time 👌
To be more specific this code was generated by Claude Sonet 4.6 as a service under my thorough supervision, so probably it's not eligible for merging as is just because it's generated by a 3rd party service.
Though I'd like to do more contributions in the near future but I'm not experienced enough in C++ and system programming to pull this off easily...

Since I don't have legal expertise here, could you please clarify in more details what counts as an unacceptable "AI generated code" contribution?

What if I will use an AI agent service for advisory and static analysis only, but will type the actual code by my own hands?
Or if I will use a locally running AI model that is released under Apache 2.0 or other GPL3 compatible license?

I'd like to know my limits here from Genode perspective 🤔

@nfeske
Copy link
Copy Markdown
Member

nfeske commented Apr 14, 2026

Or if I will use a locally running AI model that is released under Apache 2.0 or other GPL3 compatible license?

I'm not a lawyer. But I consider learning from any source (book, teacher, experience, internet search engine, AI model) as fine as long as the information source is legal (think of trade secrets).

Code generation is different matter. Legally, regardless of whether running the model locally or as a service, unless you created the model yourself, you cannot know from which data the model parameters had been derived. So one should assume the worst. Common sense. The obvious way to stay safe is to craft code by hand, driven by human intent.

At the end of the day, someone has to assert responsibility, for both technical and legal risks. A maintainer becomes responsible for the code whenever accepting a contribution. Accepting the risks of AI-generated artifacts would be irresponsible for Genode Labs. Hence, we have to keep our code untainted.

@G0retZ
Copy link
Copy Markdown
Author

G0retZ commented Apr 14, 2026

I'm not a lawyer. But I consider learning from any source (book, teacher, experience, internet search engine, AI model) as fine as long as the information source is legal (think of trade secrets).

Code generation is different matter. Legally, regardless of whether running the model locally or as a service, unless you created the model yourself, you cannot know from which data the model parameters had been derived. So one should assume the worst. Common sense. The obvious way to stay safe is to craft code by hand, driven by human intent.

At the end of the day, someone has to assert responsibility, for both technical and legal risks. A maintainer becomes responsible for the code whenever accepting a contribution. Accepting the risks of AI-generated artifacts would be irresponsible for Genode Labs. Hence, we have to keep our code untainted.

Thanks for clarification! 🙏
I will change my approach to avoid generating code with AI for contributions from now on. I will use AI as a mere learning and reasoning tool, but will craft code by hand 👍

@ssumpf ssumpf added the fixed label Apr 15, 2026
@ssumpf
Copy link
Copy Markdown
Member

ssumpf commented Apr 15, 2026

Fixed in #5088

@chelmuth chelmuth closed this Apr 15, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants