Page MenuHomeFreeBSD

Wait for EN transition on disable
ClosedPublic

Authored by imp on Dec 6 2017, 1:37 AM.
Tags
None
Referenced Files
Unknown Object (File)
Sun, Mar 17, 6:15 PM
Unknown Object (File)
Thu, Mar 7, 7:42 PM
Unknown Object (File)
Wed, Mar 6, 11:18 PM
Unknown Object (File)
Thu, Feb 29, 8:46 PM
Unknown Object (File)
Feb 4 2024, 6:55 AM
Unknown Object (File)
Feb 2 2024, 8:56 AM
Unknown Object (File)
Jan 17 2024, 10:55 AM
Unknown Object (File)
Jan 14 2024, 5:24 AM
Subscribers

Details

Summary

When we're disabling the nvme device, we need to wait first for the EN bit to go from 1 to 0, and then we need to wait for the RDY bit to go to 0. We have a timeout of 2500ms.

Before, we were just waiting a little bit and assuming life is
good. However, we waited only 5ms. Linux, for quirked devices, waits
2.3s. We shouldn't need to wait so long, since we can expect cards to
follow the standard here (Linux doesn't check, but likely should).

This results in not being able to recover from wedged cards on a busy
system on at least one card in Linux's quirk list.

Finally, fail the reset if the disable fails. This will lead to a failed device, which is what we want.

Sponsored by: Netflix

Test Plan

Note: We go off the rails when we fail, especially with the nda client, but that's for another time to fix.

Diff Detail

Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 13370
Build 13602: arc lint + arc unit

Event Timeline

imp edited the test plan for this revision. (Show Details)
imp added reviewers: jimharris, scottl.
imp edited the test plan for this revision. (Show Details)

Actually fail the reset.

Doh, must commit before arc diff --update it seems.

This revision is now accepted and ready to land.Dec 7 2017, 5:22 PM
chuck added inline comments.
sys/dev/nvme/nvme_ctrlr.c
250–258

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.

1133

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.

sys/dev/nvme/nvme_ctrlr.c
250–258

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

1133

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.

Update, per chuck's feedback.

This revision now requires review to proceed.Dec 7 2017, 8:22 PM
sys/dev/nvme/nvme_ctrlr.c
1133

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?

Remove unused read of cc register.

sys/dev/nvme/nvme_ctrlr.c
247

This is now unused, remove it.

250–258

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.

1133

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.

After talking with an NVME expert, go with the delay + quirk instead
since this is a very odd thing, and doing the EN read dance (a) would
fail on these cards and (b) isn't a standard conforming thing (as
already pointed out in the review).

sys/dev/nvme/nvme.c
107 ↗(On Diff #36369)

Should the device ID be 0xa821144d?

108 ↗(On Diff #36369)

Should the device ID be 0xa822144d?

sys/dev/nvme/nvme.c
107 ↗(On Diff #36369)

yup

108 ↗(On Diff #36369)

yup.

Other than the noted if check, this looks good to me.

sys/dev/nvme/nvme_ctrlr.c
296

If EN and RDY are out of sync, the user probably needs to issue a reset to recover the card. Would it be useful to log this?

311

Should this check CSTS.RDY? Something like

if (csts.bit.rdy == 1) {
        /* EN == 0 already wait for RDY == 0 or fail */
        err = nvme_ctrlr_wait_for_ready(ctrlr, 0);
        if (err != 0)
                return (err);
}
This revision is now accepted and ready to land.Dec 11 2017, 5:17 AM
This revision was automatically updated to reflect the committed changes.