I suspect what the code was trying to do here was follow the CC.EN guidance in the spec (section 3.1.5 of the 1.3 spec) that transitioning CC.EN from 0 to 1 when CSTS.RDY is 1 or transitioning CC.EN from 1 to 0 when CSTS.RDY is 0 "has undefined results".

If that is the case, there should be code similar to:

if (csts.bits.rdy == desired) {
    nvme_printf(ctrlr,
        "%s: cannot transition in undefined state "
        "(cc.en -> %u when csts.rdy = %u)\n",
        __func__, desired, csts.bits.rdy);
    return (ENXIO);
}

in nvme_ctrlr_disable() and nvme_ctrlr_enable(), but not here.

Vendors specify the maximum time for CSTS.RDY and CSTS.SHST transitions in CAP.TO, and by in large, test that their products meet this value. While the other changes seem good, this change forces the driver to wait more than the vendor specifies and longer than the spec mandates.

The original code here is what I would do and have done.

The old code didn't do that at all... But this is a good suggestion.

I'm not sure I get this comment.

We still use ready_timeout_in_ms which is derived from CAP.TO. What enable_timeout does is for the CC.EN readback after we've set it to make sure that it transitions before we start the timer on the RDY transition (which is still bounded by ready_timeout_in_ms). I couldn't find a spec for how long it will take for EN to transition.

Bah, I misunderstood what the code was trying to do.

Best I can tell, the Linux code doesn't check that the register write to CC completed for either the enable or disable case. My interpretation of the Linux code is that it allows extra time for the quirked device to set CSTS.RDY == 0.

Since NVMe doesn't specify when implementation must update the EN field, there's an argument that says the code which waits for this field to be set is not guaranteed to be correct for compliant implementations. This would argue for removing the check of cc.bits.en altogether. Also, wouldn't PCIe transaction ordering rules prevent the read of CSTS (non-posted) from passing the write to CC (posted)?

If the troublesome device needs additional time to disable, wouldn't it be better to add a quirk for the device and then add an additional DELAY() in nvme_ctrlr_disable() for those devices?

This is now unused, remove it.

Ah, I see what you are saying... The code was trying to enforce that as a precondition.... However, since we just changed things, there's a race here, I think, so I'll leave this check out.

I hate quirks. They are super ugly. Linux always sleeps 2.3s on disable for these devices.

Also, it's hard to know that you need to wait until you've had one FAIL, notice that the failure was because the timeout was insufficient, and add a quirk. And if your OPS group is lame, it may mean you have to wait for several failures to happen before it makes it back to you for FA.

Should the device ID be 0xa821144d?

Should the device ID be 0xa822144d?

yup

yup.