[PATCH] NVMe: Create module parameter for shutdown timeout.

[PATCH] NVMe: Create module parameter for shutdown timeout.

[Dan McLeran]

The current implementation hard-codes shutdown timeout to 2 seconds. Some devices take
longer than this to successfully complete a normal shutdown. This patch creates a module
parameter for shutdown timeout and sets the default to 5 seconds.

Signed-off-by: Dan McLeran <daniel.mcleran at intel.com>
---
 drivers/block/nvme-core.c |    7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/block/nvme-core.c b/drivers/block/nvme-core.c
index 02351e2..a3c0af7 100644
--- a/drivers/block/nvme-core.c
+++ b/drivers/block/nvme-core.c
@@ -48,6 +48,7 @@
 #define SQ_SIZE(depth)		(depth * sizeof(struct nvme_command))
 #define CQ_SIZE(depth)		(depth * sizeof(struct nvme_completion))
 #define ADMIN_TIMEOUT		(admin_timeout * HZ)
+#define SHUTDOWN_TIMEOUT	(nvme_shutdown_timeout * HZ)
 #define IOD_TIMEOUT		(retry_time * HZ)
 
 static unsigned char admin_timeout = 60;
@@ -62,6 +63,10 @@ static unsigned char retry_time = 30;
 module_param(retry_time, byte, 0644);
 MODULE_PARM_DESC(retry_time, "time in seconds to retry failed I/O");
 
+unsigned char nvme_shutdown_timeout = 5;
+module_param_named(shutdown_timeout, nvme_shutdown_timeout, byte, 0644);
+MODULE_PARM_DESC(shutdown_timeout, "timeout in seconds for controller shutdown");
+
 static int nvme_major;
 module_param(nvme_major, int, 0);
 
@@ -1428,7 +1433,7 @@ static int nvme_shutdown_ctrl(struct nvme_dev *dev)
 	cc = (readl(&dev->bar->cc) & ~NVME_CC_SHN_MASK) | NVME_CC_SHN_NORMAL;
 	writel(cc, &dev->bar->cc);
 
-	timeout = 2 * HZ + jiffies;
+	timeout = SHUTDOWN_TIMEOUT + jiffies;
 	while ((readl(&dev->bar->csts) & NVME_CSTS_SHST_MASK) !=
 							NVME_CSTS_SHST_CMPLT) {
 		msleep(100);
-- 
1.7.10.4

[PATCH] NVMe: Create module parameter for shutdown timeout.

[Matthew Wilcox]

On Fri, Jun 20, 2014@03:22:54AM -0600, Dan McLeran wrote:
> The current implementation hard-codes shutdown timeout to 2 seconds. Some devices take
> longer than this to successfully complete a normal shutdown. This patch creates a module
> parameter for shutdown timeout and sets the default to 5 seconds.

Those devices are broken.  From the spec:

"It is recommended that the host wait a minimum of one second for the
shutdown operations to complete."

We're being generous and allowing them two seconds.  Whoever's device
this is should fix it.

[PATCH] NVMe: Create module parameter for shutdown timeout.

[Mayes, Barrett N]

>Those devices are broken.  From the spec:
>"It is recommended that the host wait a minimum of one second for the shutdown operations to complete."
>We're being generous and allowing them two seconds.  Whoever's device this is should fix it.

"Minimum of one second" means "wait at least one second".  It doesn't specify an upper boundary for the maximum time to wait.  I believe there's a proposal in flight to try and clarify this (or there will be soon).

Start stop unit on the SCSI side was more generous with timing, as was standby immediate for ATA.  1 sec. for shutdown, especially to be power fail safe following device acknowledgement is unnecessarily aggressive.

[PATCH] NVMe: Create module parameter for shutdown timeout.

[Mayes, Barrett N]

>"Minimum of one second" means "wait at least one second".  It doesn't specify an upper boundary for the maximum time to wait.
>I believe there's a proposal in flight to try and clarify this (or there will be soon).

TP011 for Runtime D3 support added bytes 91:88 to ID controller for runtime D3 entry latency and associated text for host to wait this amount of time to enter shutdown.

A value of 0 still means wait at least one second without an upper limit imposed by the spec.

[PATCH] NVMe: Create module parameter for shutdown timeout.

[Matthew Wilcox]

On Fri, Jun 20, 2014@03:22:54AM -0600, Dan McLeran wrote:
> @@ -62,6 +63,10 @@ static unsigned char retry_time = 30;
>  module_param(retry_time, byte, 0644);
>  MODULE_PARM_DESC(retry_time, "time in seconds to retry failed I/O");
>  
> +unsigned char nvme_shutdown_timeout = 5;
> +module_param_named(shutdown_timeout, nvme_shutdown_timeout, byte, 0644);
> +MODULE_PARM_DESC(shutdown_timeout, "timeout in seconds for controller shutdown");

You've copied the pattern for nvme_io_timeout instead of retry_time.
The reason io_timeout has the nvme_ prefix is because it's exposed outside
this file (it's used by nvme-scsi.c).  I don't see a reason to expose
shutdown_timeout outside of nvme-core.c, so retry_time is the pattern to
follow here.

>  static int nvme_major;
>  module_param(nvme_major, int, 0);
>  
> @@ -1428,7 +1433,7 @@ static int nvme_shutdown_ctrl(struct nvme_dev *dev)
>  	cc = (readl(&dev->bar->cc) & ~NVME_CC_SHN_MASK) | NVME_CC_SHN_NORMAL;
>  	writel(cc, &dev->bar->cc);
>  
> -	timeout = 2 * HZ + jiffies;
> +	timeout = SHUTDOWN_TIMEOUT + jiffies;
>  	while ((readl(&dev->bar->csts) & NVME_CSTS_SHST_MASK) !=
>  							NVME_CSTS_SHST_CMPLT) {
>  		msleep(100);
> -- 
> 1.7.10.4
> 
> 
> _______________________________________________
> Linux-nvme mailing list
> Linux-nvme at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-nvme