Does this work instead? I'm not a huge fan of "wild pointer" arithmetic; the array notation makes alignment, sizing, and indexing clearer to me, if that strategy could be used instead.

Is there a well-defined constant that can be used in place of 2?

Does this work instead?

Does it make sense to think about how one might do this as a subclass instead of a series of particularly specific quirks?

Yes, but i'm thinking we could land it without too much mutation and early optimisations, then refactor it.

PS: A bunch of the code/logic is reused in the Apple Sillicon implementation. (I have both running). And the Diff will make no sense without the full picture.

We can add it.

So this is a big ugly hammer for early, buggy NVMe drives. Maybe you can help me understand the symptom this cures? And if the really long time we delay before we check for ready is needed?

How does trying something we know won't work future proof the code? Is there other NVMe drives that apple makes with this quirk coming soon?

I know that Linux went the subclass route, and I think that these operations may be such that we might want to do that too.
Plus I have access to a crypto card that uses the NVMe interface that has other special needs which a properly designed subclass system will fit into.

Having said that, I'm cool with the hacks for the moment until we have something working and solid. Then we can refactor... I know this is a bit of a risk because once it's working, people move on...

Maybe make it a macro instead of inlining it, since we'll need to do this in multiple places.

Yea. I'd be tempted to just make this a field (maybe ctrlr->max_id_cns) and compare against it always for OPC_IDENTIFY commands?

And is this the right place to put it? I'd think we'd want to put it lower in the stack since not all identify commands come through here.

See my comment about a macro instead.

There are few other NVME with other models that i could not test on. This code was defensive, i will remove it.

I'll note for future work that we generally have shifted to if ((flag & FLAG) != 0) elsewhere in the kernel. However, this is correct for this driver because it's older and uses the older if (flag & FLAG) style.

As a design thing, I think that this might be better as it's own quirk, though that might be slicing things too thinly.

If you normally set sqe_size to 1, and bumped that to '2' for apple, then this macro could actually be the much simpler &(qpair)->cmd[idx] (maybe with an extra set of parens).

This is where you'd do '1' or '2'.

and this would need an additional * sizeof(struct nvme_command) at the end.

same here.

how does

so if sqes_max is 6 or 7, then we cna simplify sqe_size to be seq_max - 6 always and rename to sqe_shift. But that would also need to change the * seq_size to a << seq_shift.

This is odd to create a goto/label which only jumps a few lines like this. Is there a reason why the previous logic was insufficient? I've proposed an alternative diff which avoids the goto/label.

So it occurs to me that

if (nvme_ctrlr_identify(ctrlr) == 0 &&
    nvme_ctrlr_set_num_qpairs(ctrlr) == 0 &&
    nvme_ctrlr_construct_io_qpairs(ctrlr) == 0)
        nvme_ctrlr_start(ctrlr, false);
else
        nvme_ctrlr_fail(ctrlr, false);

might be even better. If all three things succeed, start the controller, otherwise fail it.

I like @imp's suggestion: it's a lot cleaner than mine.

This is no doubt where things went awry with my '16 MBP: they (Apple) probably used the same NVMe controller in the MBP, but the behavior of the controller differs in some key ways. I'm going to boot Linux and see what details I can get from my '16 (pre-T2) MBP and my '19 (T2) MBP.

Here's what Linux has down for these controllers:

{ PCI_DEVICE(PCI_VENDOR_ID_APPLE, 0x2001),
        /*
         * Fix for the Apple controller found in the MacBook8,1 and
         * some MacBook7,1 to avoid controller resets and data loss.
         */
        .driver_data = NVME_QUIRK_SINGLE_VECTOR |
                        NVME_QUIRK_QDEPTH_ONE },
{ PCI_DEVICE(PCI_VENDOR_ID_APPLE, 0x2003) },
{ PCI_DEVICE(PCI_VENDOR_ID_APPLE, 0x2005),
        .driver_data = NVME_QUIRK_SINGLE_VECTOR |
                        NVME_QUIRK_128_BYTES_SQES |
                        NVME_QUIRK_SHARED_TAGS |
                        NVME_QUIRK_SKIP_CID_GEN |
                        NVME_QUIRK_IDENTIFY_CNS },

This block is what caused the panic on my MBP: hardcoding the value to 0 fixed the panic.

Can you add a printf for ctrlr->cdata.sqes (as hex) please?

Can you add a printf for ctrlr->cdata.sqes (as hex) please?

I got 2 printouts (even though there's only 1 controller in the laptop):

• 0
• 0x76

Can you add a printf for ctrlr->cdata.sqes (as hex) please?

I got 2 printouts (even though there's only 1 controller in the laptop):

• 0
• 0x76

The function is called via nvme_ctrlr_construct_admin_qpair and nvme_ctrlr_construct_io_qpair; it's probably barfing on one of the qpair calls and not the other. Let's figure out which one.

Can you add a printf for ctrlr->cdata.sqes (as hex) please?

I got 2 printouts (even though there's only 1 controller in the laptop):

• 0
• 0x76

The function is called via nvme_ctrlr_construct_admin_qpair and nvme_ctrlr_construct_io_qpair; it's probably barfing on one of the qpair calls and not the other. Let's figure out which one.

The I/O qpair call is the one that's returning 0x76; the qpair ID is 1.

To my naive self, it seems kind of odd that cid_base is being adjusted based on adminq.num_trackers if it's the I/O qpair (just based on the name I would expect the ADMIN qpair call to adjust that value, not the I/O one..).

Can you add a printf for ctrlr->cdata.sqes (as hex) please?

I got 2 printouts (even though there's only 1 controller in the laptop):

• 0
• 0x76

The function is called via nvme_ctrlr_construct_admin_qpair and nvme_ctrlr_construct_io_qpair; it's probably barfing on one of the qpair calls and not the other. Let's figure out which one.

The I/O qpair call is the one that's returning 0x76; the qpair ID is 1.

To my naive self, it seems kind of odd that cid_base is being adjusted based on adminq.num_trackers if it's the I/O qpair (just based on the name I would expect the ADMIN qpair call to adjust that value, not the I/O one..).

I started skimming through the NVMe spec a bit [1]. I don't quite understand why this change is needed for shifting the cid_base yet, but I'm onboarding myself a bit with NVMe terminology/concepts.

If you could help me understand @guest-seuros , maybe I can programmer's duck a solution to the bug out with you.

1. https://nvmexpress.org/wp-content/uploads/NVM-Express-Base-Specification-Revision-2.3-2025.08.01-Ratified.pdf

The stopgap for the issue you reported is in: D58821.

The driver was deriving the I/O submission-queue-entry size at runtime by probing cdata.sqes from IDENTIFY CONTROLLER and that probe was giving the wrong value for S3X, because we still don't have MQs.

It worked before because we had defaults that match the T1 Drive.

Your drive could get double throughput once we implement the Multi Queue Support.