> From: "Theo de Raadt" <dera...@openbsd.org>
> Date: Tue, 06 Aug 2024 23:04:02 -0600
>
> nvme resume is really crazy, since it does not believe the device is
> stopped, tries to use high-level operations to stop it and then restart
> it, but it ends up reusing queue structures from before.
Well, in sys/dev/ic/nvmereg.h I see this:
#define NVME_NSSR 0x0020 /* NVM Subsystem Reset (Optional) */
so a low-level hardware reset may not be possible...
> dv spent time in here, and I tried to figure it out also. The code
> is highly suspect since it isn't doing the MINIMUM, which is:
>
> at suspend time, ensure the soft state is good, and shut the hw down HARD.
>
> On resume, reset the hardware HARD in the minimum fashion, and then
> reconfigure it to match the soft state.
My understanding of what dv@ was looking at was avoiding placing
nvme(4) in D3 since apparently some drives can't get out of that state
anymore. So that may mean there is now way to completely whack the
device and that we need to keep it alive to send it certain power
management commands.
Anyway, I think you're right in thinking that nvme_intr() needs some
belt and suspenders. In nvme_shutdown() we delete the "normal"
command queue, but nvme_intr() inconditionally looks at both of them.
Now nvme_shutdown() masks the interrupt and nvme_resume() unmasks it
only after it re-creates the "normal" command queue. But I think
there are scenarios where we can get a spurious interrupt and it would
check a queue that isn't there. So I think something like the diff
below would make sense.
Greg, does this fix your crash?
Index: dev/ic/nvme.c
===================================================================
RCS file: /cvs/src/sys/dev/ic/nvme.c,v
diff -u -p -r1.121 nvme.c
--- dev/ic/nvme.c 13 Jul 2024 08:59:41 -0000 1.121
+++ dev/ic/nvme.c 7 Aug 2024 09:31:14 -0000
@@ -418,12 +418,14 @@ nvme_attach(struct nvme_softc *sc)
free_q:
nvme_q_free(sc, sc->sc_q);
+ sc->sc_q = NULL;
disable:
nvme_disable(sc);
free_ccbs:
nvme_ccbs_free(sc, nccbs);
free_admin_q:
nvme_q_free(sc, sc->sc_admin_q);
+ sc->sc_admin_q = NULL;
return (1);
}
@@ -463,6 +465,7 @@ nvme_resume(struct nvme_softc *sc)
free_q:
nvme_q_free(sc, sc->sc_q);
+ sc->sc_q = NULL;
disable:
nvme_disable(sc);
@@ -531,6 +534,7 @@ nvme_shutdown(struct nvme_softc *sc)
printf("%s: unable to delete q, disabling\n", DEVNAME(sc));
goto disable;
}
+ sc->sc_q = NULL;
cc = nvme_read4(sc, NVME_CC);
CLR(cc, NVME_CC_SHN_MASK);
@@ -1552,9 +1556,9 @@ nvme_intr(void *xsc)
struct nvme_softc *sc = xsc;
int rv = 0;
- if (nvme_q_complete(sc, sc->sc_q))
+ if (sc->sc_q && nvme_q_complete(sc, sc->sc_q))
rv = 1;
- if (nvme_q_complete(sc, sc->sc_admin_q))
+ if (sc->sc_admin_q && nvme_q_complete(sc, sc->sc_admin_q))
rv = 1;
return (rv);
