Skip to contentSkip to conversation

NVMe: Create module parameter for shutdown timeout.

PATCH New Dan McLeran 3 participants 5 messages Jun 20, 2014 – Jun 28, 2014

Subsystems (2)
Block layer NVMe
lore
Share & tools
lore

Dan McLeran

author
#0 lore raw

PATCH 1/1 NVMe: Create module parameter for shutdown timeout.

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(-)
drivers/block/nvme-core.c +6 -1
index 02351e2..a3c0af7 100644
@@ -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);
Signature · 1 line
1.7.10.4
#1 lore raw

PATCH 1/1 NVMe: Create module parameter for shutdown timeout.

Quoted 3 lines · 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.

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.

#2 lore raw

PATCH 1/1 NVMe: Create module parameter for shutdown timeout.

Quoted 3 lines

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.

#3 lore raw

PATCH 1/1 NVMe: Create module parameter for shutdown timeout.

In reply to Matthew Wilcox
Quoted 2 lines

"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.

#4 lore raw

PATCH 1/1 NVMe: Create module parameter for shutdown timeout.

In reply to Dan McLeran

drivers/block/nvme-core.c static unsigned char retry_time = 30; · line 68

Quoted 7 lines · Dan McLeran
drivers/block/nvme-core.c +3 -0
@@ -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.

Quoted 20 lines
drivers/block/nvme-core.c +1 -1
 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);
Signature · 7 lines
1.7.10.4


_______________________________________________
Linux-nvme mailing list
Linux-nvme at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme