RE: [PATCH v1 0/1] nvme: Fix problem when booting from NVMe drive was leading to a hang.

[Michael Kropaczek <Michael.Kropaczek@solidigm.com>]

> -----Original Message-----
> From: Keith Busch <kbusch@kernel.org> 
> Sent: Thursday, February 22, 2024 8:28 AM
> To: Michael Kropaczek <Michael.Kropaczek@solidigm.com>
> Cc: linux-nvme@lists.infradead.org; Jens Axboe <axboe@fb.com>; Christoph Hellwig <hch@lst.de>; Sagi Grimberg <sagi@grimberg.me>
> Subject: Re: [PATCH v1 0/1] nvme: Fix problem when booting from NVMe drive was leading to a hang.
> 
> 
> On Wed, Feb 21, 2024 at 12:43:40AM -0800, Michael Kropaczek wrote:
> > @@ -2593,6 +2593,16 @@ static void nvme_dev_disable(struct nvme_dev *dev, bool shutdown)
> >                       nvme_wait_freeze_timeout(&dev->ctrl, NVME_IO_TIMEOUT);
> >       }
> >
> > +     /*
> > +      * On certain host architectures/HW, DRAM was keeping memory contents over reboot-cycles.
> > +      * It was observed that certain controllers were accessing host memory after
> > +      * resetting which led to undefined state preventing proper initialization.
> > +      */
> > +     if (dev->hmb)
> > +             nvme_set_host_mem(dev, 0);
> > +
> > +     nvme_free_host_mem(dev);
> > +
> 
> Shouldn't this go in nvme_shutdown() instead? I don't think we want to tear down the controller's host memory on every controller reset.

Thank you, Keith, this is a valid point.
The function nvme_dev_disable() changed here is called from the nvme_shutdown() subsequently through nvme_disable_prepare_reset() with the flag shutdown being set as true.

Considering the re-usability of the code, would it be correct to wrap up the change into if(shutdown) statement?

The reason for asking is that disabling of host memory should follow other calls as well and placing it within the body of nvme_shutdown() would require more adjustments.

The call to nvme_disable_prepare_reset() with the flag shutdown set as true is made whenever the NVMe controller should be shut down anyway.