Re: [PATCH 1/2] nvme-apple: Do not try to shut down the controller twice

[Christoph Hellwig]

On Wed, Jan 11, 2023 at 02:10:42PM +0900, Hector Martin wrote:
> OK, so the first question is who is responsible for resetting the
> controller after a shutdown? The spec requires a reset in order to bring
> it back up from that state. Indeed the PCIe code does an explicit
> disable right now (though, judging by the comment, it probably wasn't
> done with the intent of resetting after a shutdown, it just happens to
> work for that too :))

We need to do the reset before banging the registers to make sure
the controller is in a sane state before starting to set it up.

> Right now, apple_nvme_reset_work() tries to check for the condition of
> an enabled controller (under the assumption that it's coming from a live
> controller, not considering shutdown/sleep) and issue an
> apple_nvme_disable(). However, this doesn't work when resuming because
> at that point the firmware coprocessor is shut down, so the device isn't
> usable (can't even get a disable command to complete properly). Perhaps
> a better conditional here would be to check for
> apple_rtkit_is_running(), since apple_nvme_disable() can't work otherwise.

So on a resume the controller should have previously been shutdown
properly, and this shouldn't be an issue.  Does the apple implementation
leave some weird state after a shut down?  In that case the resume
callback might want to do an explicit controller disable before doing
the reset.

> Alternatively, we could just revert to the prior behavior of always
> issuing a disable after a shutdown. We need to disable at some point to
> come back anyway, so it might as well be done early (before we shut down
> firmware, so it works).

So the disable before shutdown doesn't really make sense from the
NVMe POV - the shutdown is to cleanly bring the device into a state
where it can quickly recover.  While a disable is an abprupt shutdown,
which can have effects on recover time, and might also use way more
P/E cycles than nessecary.  So if you always want to do a disable,
it should be after shutdown, and in doubt in the resume / setup path
instead of the remove / suspend one.