Re: [PATCH] nvme: Do a clean NVMe shutdown

[Tom Rini]

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 
> Tested-by: Mark Kettenis  [firefly-rk3399]

Applied to u-boot/master, thanks!

-- 
Tom


signature.asc
Description: PGP signature

Re: [PATCH] nvme: Do a clean NVMe shutdown

[Mark Kettenis]

> From: Hector Martin 
> 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 

Tested-by: Mark Kettenis  [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(>bar->csts) & NVME_CSTS_RDY) == bit)
> + if ((readl(>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, >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, >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, >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
> 
> 

[PATCH] nvme: Do a clean NVMe shutdown

[Hector Martin]

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 
---
 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(>bar->csts) & NVME_CSTS_RDY) == bit)
+   if ((readl(>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, >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, >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, >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