| Message ID | 20220731063131.38271-1-marcan@marcan.st |
|---|---|
| State | Accepted |
| Commit | e9ac3a939d4f11e9cdcdcb2bad090f4cb1929a85 |
| Delegated to: | Tom Rini |
| Headers | show |
| Series | nvme: Do a clean NVMe shutdown | expand |
> From: Hector Martin <marcan@marcan.st> > Date: Sun, 31 Jul 2022 15:31:31 +0900 > > The brute-force controller disable method can end up racing controller > initilization and causing a crash when we shut down Apple ANS2 NVMe > controllers. Do a proper controlled shutdown, which does block until > things are quiesced properly. This is nicer in general for all > controllers. > > Signed-off-by: Hector Martin <marcan@marcan.st> Tested-by: Mark Kettenis <kettenis@openbsd.org> [firefly-rk3399] > --- > drivers/nvme/nvme.c | 25 ++++++++++++++++++++----- > 1 file changed, 20 insertions(+), 5 deletions(-) > > diff --git a/drivers/nvme/nvme.c b/drivers/nvme/nvme.c > index a305305885ec..5fd2fb9ed6a6 100644 > --- a/drivers/nvme/nvme.c > +++ b/drivers/nvme/nvme.c > @@ -27,9 +27,8 @@ > #define IO_TIMEOUT 30 > #define MAX_PRP_POOL 512 > > -static int nvme_wait_ready(struct nvme_dev *dev, bool enabled) > +static int nvme_wait_csts(struct nvme_dev *dev, u32 mask, u32 val) > { > - u32 bit = enabled ? NVME_CSTS_RDY : 0; > int timeout; > ulong start; > > @@ -38,7 +37,7 @@ static int nvme_wait_ready(struct nvme_dev *dev, bool enabled) > > start = get_timer(0); > while (get_timer(start) < timeout) { > - if ((readl(&dev->bar->csts) & NVME_CSTS_RDY) == bit) > + if ((readl(&dev->bar->csts) & mask) == val) > return 0; > } > > @@ -295,7 +294,7 @@ static int nvme_enable_ctrl(struct nvme_dev *dev) > dev->ctrl_config |= NVME_CC_ENABLE; > writel(dev->ctrl_config, &dev->bar->cc); > > - return nvme_wait_ready(dev, true); > + return nvme_wait_csts(dev, NVME_CSTS_RDY, NVME_CSTS_RDY); > } > > static int nvme_disable_ctrl(struct nvme_dev *dev) > @@ -304,7 +303,16 @@ static int nvme_disable_ctrl(struct nvme_dev *dev) > dev->ctrl_config &= ~NVME_CC_ENABLE; > writel(dev->ctrl_config, &dev->bar->cc); > > - return nvme_wait_ready(dev, false); > + return nvme_wait_csts(dev, NVME_CSTS_RDY, 0); > +} > + > +static int nvme_shutdown_ctrl(struct nvme_dev *dev) > +{ > + dev->ctrl_config &= ~NVME_CC_SHN_MASK; > + dev->ctrl_config |= NVME_CC_SHN_NORMAL; > + writel(dev->ctrl_config, &dev->bar->cc); > + > + return nvme_wait_csts(dev, NVME_CSTS_SHST_MASK, NVME_CSTS_SHST_CMPLT); > } > > static void nvme_free_queue(struct nvme_queue *nvmeq) > @@ -904,6 +912,13 @@ free_nvme: > int nvme_shutdown(struct udevice *udev) > { > struct nvme_dev *ndev = dev_get_priv(udev); > + int ret; > + > + ret = nvme_shutdown_ctrl(ndev); > + if (ret < 0) { > + printf("Error: %s: Shutdown timed out!\n", udev->name); > + return ret; > + } > > return nvme_disable_ctrl(ndev); > } > -- > 2.35.1 > >
On Sun, Jul 31, 2022 at 03:31:31PM +0900, Hector Martin wrote: > The brute-force controller disable method can end up racing controller > initilization and causing a crash when we shut down Apple ANS2 NVMe > controllers. Do a proper controlled shutdown, which does block until > things are quiesced properly. This is nicer in general for all > controllers. > > Signed-off-by: Hector Martin <marcan@marcan.st> > Tested-by: Mark Kettenis <kettenis@openbsd.org> [firefly-rk3399] Applied to u-boot/master, thanks!
diff --git a/drivers/nvme/nvme.c b/drivers/nvme/nvme.c index a305305885ec..5fd2fb9ed6a6 100644 --- a/drivers/nvme/nvme.c +++ b/drivers/nvme/nvme.c @@ -27,9 +27,8 @@ #define IO_TIMEOUT 30 #define MAX_PRP_POOL 512 -static int nvme_wait_ready(struct nvme_dev *dev, bool enabled) +static int nvme_wait_csts(struct nvme_dev *dev, u32 mask, u32 val) { - u32 bit = enabled ? NVME_CSTS_RDY : 0; int timeout; ulong start; @@ -38,7 +37,7 @@ static int nvme_wait_ready(struct nvme_dev *dev, bool enabled) start = get_timer(0); while (get_timer(start) < timeout) { - if ((readl(&dev->bar->csts) & NVME_CSTS_RDY) == bit) + if ((readl(&dev->bar->csts) & mask) == val) return 0; } @@ -295,7 +294,7 @@ static int nvme_enable_ctrl(struct nvme_dev *dev) dev->ctrl_config |= NVME_CC_ENABLE; writel(dev->ctrl_config, &dev->bar->cc); - return nvme_wait_ready(dev, true); + return nvme_wait_csts(dev, NVME_CSTS_RDY, NVME_CSTS_RDY); } static int nvme_disable_ctrl(struct nvme_dev *dev) @@ -304,7 +303,16 @@ static int nvme_disable_ctrl(struct nvme_dev *dev) dev->ctrl_config &= ~NVME_CC_ENABLE; writel(dev->ctrl_config, &dev->bar->cc); - return nvme_wait_ready(dev, false); + return nvme_wait_csts(dev, NVME_CSTS_RDY, 0); +} + +static int nvme_shutdown_ctrl(struct nvme_dev *dev) +{ + dev->ctrl_config &= ~NVME_CC_SHN_MASK; + dev->ctrl_config |= NVME_CC_SHN_NORMAL; + writel(dev->ctrl_config, &dev->bar->cc); + + return nvme_wait_csts(dev, NVME_CSTS_SHST_MASK, NVME_CSTS_SHST_CMPLT); } static void nvme_free_queue(struct nvme_queue *nvmeq) @@ -904,6 +912,13 @@ free_nvme: int nvme_shutdown(struct udevice *udev) { struct nvme_dev *ndev = dev_get_priv(udev); + int ret; + + ret = nvme_shutdown_ctrl(ndev); + if (ret < 0) { + printf("Error: %s: Shutdown timed out!\n", udev->name); + return ret; + } return nvme_disable_ctrl(ndev); }
The brute-force controller disable method can end up racing controller initilization and causing a crash when we shut down Apple ANS2 NVMe controllers. Do a proper controlled shutdown, which does block until things are quiesced properly. This is nicer in general for all controllers. Signed-off-by: Hector Martin <marcan@marcan.st> --- drivers/nvme/nvme.c | 25 ++++++++++++++++++++----- 1 file changed, 20 insertions(+), 5 deletions(-)