nvme: fix Apple S3X controller panic

[seuros]

Summary

Serialize S3X I/O and cap dtransfers while keeping namespace handling.
Select 64/128-byte submission queue entries explicitly and set
CC.IOSQES from the same value used for the software queue stride.

When fatal status is set, wait for pending PCIe transactions and then
force FLR so a wedged controller doesn't panic or timeout.

MFC: 1 week
PR: 296946
Fixes: 5e0ba47aa00e

Test Plan

Should boot into T1/T2 Apple hardware and other brands without hiccup.

[ngie]

I made a few edits to the description to help polish up the description.
Will confirm that this no longer results in panics on my MacBookPro 13,2 later on today.
Do you have any references or links to specs you can provide to help confirm that this fix is correct?

[seuros]

Do you have any references or links to specs you can provide to help confirm that this fix is correct?

No spec. Linux boot this machine, and we used to have it working.

So this was more a bisect work to understand and find a way to keep T1/T2 and non Apple NVME working. (I bought 3 T1 laptops to try, including the 13.2)

T1 nvme is using half of its capability , and T2 drive are operating in degraded throughput. (this is the same behaviorr in linux).

But i believe if we implement the MQS (Multi Queue Support) , we could gain speed.

[imp]

The num namespaces changes should be separate

sys/dev/nvme/nvme_ctrlr.c

462

I'm not sure this should be a quirk. It should be part of our normal escelation...

465

This needs a devclt event

469

This needs an event

1177

This should be under bootverbose

sys/dev/nvme/nvme_private.h

344

I thinkthe oigina comment is better.

[seuros]

address comment

[seuros]

sys/dev/nvme/nvme_ctrlr.c

462

This is a stopgap to unbreak T1 NVMe right now without it, people are stuck either reverting 5e0ba47aa00e or manually cherry-picking this diff out-of-tree.

The quirk gate isn't just caution for its own sake making part of the default escalation for all NVMe controllers without more testing can cause chaos.

As an example: this same S3X disk works fine with MQS enabled, but crashed at 78% during install when a timeout triggered under heavy load.

I don't want to change fatal-status handling for every controller in the tree on that basis.

Once this lands I'll follow up with either MQS support or the full escalation flow you're describing.

sys/dev/nvme/nvme_private.h

344

the old comment described how sqe_shift used to be derived. This diff removes that derivation.

[imp]

sys/dev/nvme/nvme_ctrlr.c

462

So this isn't a 'recovery from error' but a 'let's program the card' on start when it's needed?

[imp]

sys/dev/nvme/nvme_private.h

344

OK. Fair enough. I'm good with it.

[seuros]

sys/dev/nvme/nvme_ctrlr.c

462

It a hook, not a start-time provisioning step. the FLR-on-fatal branch only triggers when CSTS.CFS is already set.

On a normal, healthy start csts != NVME_GONE is true but CFS is 0, so this code path is skipped.

It's recovery from a fatal condition i got, gated by the quirk so it doesn't change reset behavior for every other controller.

I can remove it now, or i will with the MQS.

[imp]

sys/dev/nvme/nvme_ctrlr.c

462

So did you see this when you started up once (or occassionally) and this fixed it? Or did we drive the card into timeout, and during timeout process, when we reset the card, this fixed it? It would be good to include that, though I'm on the fence if that's a 'inline comment' detail or 'commit log' detail, but I'm leaning to the former.

I'd add these details to this, gated by quirk for now is fine, but include a line about 'in the future, this should be part of normal recovery' or similar so we can integrated it into the currently overly simplistic recovery. It's fine to have it here. I'd leave the printfs below not bootverbose, at least for the moment, but in the fullness of time we may want to print them once and keep a sysctl counter. For now, don't go ahead with that, since we should do that as part of integrating. Since the if is gated by the quirk, we're not going to spam a lot of people either.

[imp]

sys/dev/nvme/nvme_ctrlr.c

462

Ah, one other thing, I'd make the body of the if a function, returning and errno, and put the bulk of the comment there. That would let us integrate it other places as needed or move where we use it around better.

[seuros]

sys/dev/nvme/nvme_ctrlr.c

462

Driven into timeout under load, not startup.
I hit CSTS.CFS set after the controller wedged mid-transfer (heavy write load), not on a cold boot.

I modified my kernel to allow this panic! in mainline & at release mode, this code is a no-op.

[ngie]

This helps my MacBookPro13,2 boot again.
@imp : do you agree with the proposed change?

[ngie]

sys/dev/nvme/nvme_qpair.c

539

I would consider adding a KASSERT here checking that the value here is greater than 0 for the non-admin qpair.

[seuros]

sys/dev/nvme/nvme_qpair.c

539

the KASSERT above already covers this, io_sqes is pinned to 64/128 so the subtraction can't underflow

[adrian]

is there anything else needed for this?

[ngie]

In D58821#1353680, @adrian wrote:

is there anything else needed for this?

Not from me at least. @imp : second ping?

[imp]

In D58821#1355441, @ngie wrote:

In D58821#1353680, @adrian wrote:

is there anything else needed for this?

Not from me at least. @imp : second ping?

I asked for the FLR function to be separated into its own function. that's all I see, I think, still.

[seuros]

I asked for the FLR function to be separated into its own function. that's all I see, I think, still.

Damn, i should have missed the notification. I will update it soon.

[seuros]

extract function