[PATCH] nvme: Honor RTD3 Entry Latency for shutdowns

[PATCH] nvme: Honor RTD3 Entry Latency for shutdowns

[Martin K. Petersen]

If an NVMe controller reports RTD3 Entry Latency bigger than
shutdown_timeout, use that value to set the shutdown timer. Otherwise
fall back to the module parameter which defaults to 5 seconds.

Signed-off-by: Martin K. Petersen <martin.petersen at oracle.com>
---
 drivers/nvme/host/core.c | 18 +++++++++++++++++-
 drivers/nvme/host/nvme.h |  1 +
 2 files changed, 18 insertions(+), 1 deletion(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index b7dfbbeccb47..ed8f286f24bd 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -1465,7 +1465,7 @@ EXPORT_SYMBOL_GPL(nvme_enable_ctrl);
 
 int nvme_shutdown_ctrl(struct nvme_ctrl *ctrl)
 {
-	unsigned long timeout = jiffies + (shutdown_timeout * HZ);
+	unsigned long timeout = jiffies + (ctrl->shutdown_timeout * HZ);
 	u32 csts;
 	int ret;
 
@@ -1833,6 +1833,22 @@ int nvme_init_identify(struct nvme_ctrl *ctrl)
 	ctrl->sgls = le32_to_cpu(id->sgls);
 	ctrl->kas = le16_to_cpu(id->kas);
 
+	if (ctrl->vs >= NVME_VS(1, 2, 0)) {
+		u32 transition_time = le32_to_cpu(id->rtd3e);
+
+		do_div(transition_time, 1000000); /* us -> s */
+
+		if (transition_time > shutdown_timeout) {
+			ctrl->shutdown_timeout = transition_time;
+			dev_warn(ctrl->device,
+				 "Shutdown timeout set to %u seconds\n",
+				 ctrl->shutdown_timeout);
+		}
+	}
+
+	if (!ctrl->shutdown_timeout)
+		ctrl->shutdown_timeout = shutdown_timeout;
+
 	ctrl->npss = id->npss;
 	ctrl->apsta = id->apsta;
 	prev_apst_enabled = ctrl->apst_enabled;
diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
index 2c8a02be46fd..9b959ee18cb6 100644
--- a/drivers/nvme/host/nvme.h
+++ b/drivers/nvme/host/nvme.h
@@ -162,6 +162,7 @@ struct nvme_ctrl {
 	u16 kas;
 	u8 npss;
 	u8 apsta;
+	unsigned int shutdown_timeout;
 	unsigned int kato;
 	bool subsystem;
 	unsigned long quirks;
-- 
2.13.0

[PATCH] nvme: Honor RTD3 Entry Latency for shutdowns

[Scott Bauer]

On Wed, Aug 16, 2017@03:55:52PM -0400, Martin K. Petersen wrote:
> If an NVMe controller reports RTD3 Entry Latency bigger than
> shutdown_timeout, use that value to set the shutdown timer. Otherwise
> fall back to the module parameter which defaults to 5 seconds.

My only concern is if this value extends past the DPM_WATCHDOG timeout
value. If it does we're going to end up panic()ing the kernel during
suspends. If others agree I think we should set it to the minimum value
of DPM_WATCHDOG_TIMEOUT and shutdown_timeout.

[PATCH] nvme: Honor RTD3 Entry Latency for shutdowns

[Martin K. Petersen]

Scott,

> My only concern is if this value extends past the DPM_WATCHDOG timeout
> value. If it does we're going to end up panic()ing the kernel during
> suspends. If others agree I think we should set it to the minimum value
> of DPM_WATCHDOG_TIMEOUT and shutdown_timeout.

I don't have a problem with that. Keith?

-- 
Martin K. Petersen	Oracle Linux Engineering

[PATCH] nvme: Honor RTD3 Entry Latency for shutdowns

[Keith Busch]

On Wed, Aug 16, 2017@04:49:43PM -0400, Martin K. Petersen wrote:
> 
> Scott,
> 
> > My only concern is if this value extends past the DPM_WATCHDOG timeout
> > value. If it does we're going to end up panic()ing the kernel during
> > suspends. If others agree I think we should set it to the minimum value
> > of DPM_WATCHDOG_TIMEOUT and shutdown_timeout.
> 
> I don't have a problem with that. Keith?

Sounds good to me as well.

[PATCH] nvme: Honor RTD3 Entry Latency for shutdowns

[Martin K. Petersen]

Keith,

>> > My only concern is if this value extends past the DPM_WATCHDOG
>> > timeout value. If it does we're going to end up panic()ing the
>> > kernel during suspends. If others agree I think we should set it to
>> > the minimum value of DPM_WATCHDOG_TIMEOUT and shutdown_timeout.
>> 
>> I don't have a problem with that. Keith?
>
> Sounds good to me as well.

Hrm, this gets pretty messy.

DPM_WATCHDOG_TIMEOUT is buried deep in the config options (hidden behind
PM_DEBUG and EXPERT). And as a result doesn't appear to be enabled in
most common kernel configs. It also isn't exported from the PM subsystem
in a generic way.

In addition, I'm not sure what we'd do in case a device demands a 10
second shutdown time but the user has the kernel configured with a 2
second DPM watchdog. Then what? The device is still going to take 10
seconds to complete the shutdown request.

-- 
Martin K. Petersen	Oracle Linux Engineering

[PATCH] nvme: Honor RTD3 Entry Latency for shutdowns

[Scott Bauer]

On Thu, Aug 17, 2017@01:06:32PM -0400, Martin K. Petersen wrote:
> 
> Keith,
> 
> >> > My only concern is if this value extends past the DPM_WATCHDOG
> >> > timeout value. If it does we're going to end up panic()ing the
> >> > kernel during suspends. If others agree I think we should set it to
> >> > the minimum value of DPM_WATCHDOG_TIMEOUT and shutdown_timeout.
> >> 
> >> I don't have a problem with that. Keith?
> >
> > Sounds good to me as well.
> 
> Hrm, this gets pretty messy.
> 
> DPM_WATCHDOG_TIMEOUT is buried deep in the config options (hidden behind
> PM_DEBUG and EXPERT). And as a result doesn't appear to be enabled in
> most common kernel configs. It also isn't exported from the PM subsystem
> in a generic way.
> 
> In addition, I'm not sure what we'd do in case a device demands a 10
> second shutdown time but the user has the kernel configured with a 2
> second DPM watchdog. Then what? The device is still going to take 10
> seconds to complete the shutdown request.

ugh, I thought it would be as easy as if (IS_ENABLED(CONFIG_DPM_WATCHDOG))
but I guess since that's a runtime check GCC cant remove the scope below,
since it's not known at compilation time.

We wont be able to fix this without some gross ifdefs/ifndefs.

I guess we can do a dev_warn() or something if the shutdown time is >
than some threshold instead? That way if people are getting sporatic
panics on suspends we can see that warning and fix it from there?

[PATCH v2] nvme: Honor RTD3 Entry Latency for shutdowns

[Martin K. Petersen]

If an NVMe v1.2+ compliant controller reports RTD3 Entry Latency larger
than shutdown_timeout, up to a maximum of 60 seconds, use that value to
set the shutdown timer. Otherwise fall back to the module parameter
which defaults to 5 seconds.

Signed-off-by: Martin K. Petersen <martin.petersen at oracle.com>
---
 drivers/nvme/host/core.c | 19 ++++++++++++++++++-
 drivers/nvme/host/nvme.h |  1 +
 2 files changed, 19 insertions(+), 1 deletion(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index b7dfbbeccb47..13cc601012b0 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -1465,7 +1465,7 @@ EXPORT_SYMBOL_GPL(nvme_enable_ctrl);
 
 int nvme_shutdown_ctrl(struct nvme_ctrl *ctrl)
 {
-	unsigned long timeout = jiffies + (shutdown_timeout * HZ);
+	unsigned long timeout = jiffies + (ctrl->shutdown_timeout * HZ);
 	u32 csts;
 	int ret;
 
@@ -1833,6 +1833,23 @@ int nvme_init_identify(struct nvme_ctrl *ctrl)
 	ctrl->sgls = le32_to_cpu(id->sgls);
 	ctrl->kas = le16_to_cpu(id->kas);
 
+	if (ctrl->vs >= NVME_VS(1, 2, 0)) {
+		u32 transition_time = le32_to_cpu(id->rtd3e);
+
+		do_div(transition_time, 1000000); /* us -> s */
+		clamp(transition_time, shutdown_timeout, 60); /* max 60s */
+
+		if (transition_time != shutdown_timeout) {
+			ctrl->shutdown_timeout = transition_time;
+			dev_warn(ctrl->device,
+				 "Shutdown timeout set to %u seconds\n",
+				 ctrl->shutdown_timeout);
+		}
+	}
+
+	if (!ctrl->shutdown_timeout)
+		ctrl->shutdown_timeout = shutdown_timeout;
+
 	ctrl->npss = id->npss;
 	ctrl->apsta = id->apsta;
 	prev_apst_enabled = ctrl->apst_enabled;
diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
index 2c8a02be46fd..9b959ee18cb6 100644
--- a/drivers/nvme/host/nvme.h
+++ b/drivers/nvme/host/nvme.h
@@ -162,6 +162,7 @@ struct nvme_ctrl {
 	u16 kas;
 	u8 npss;
 	u8 apsta;
+	unsigned int shutdown_timeout;
 	unsigned int kato;
 	bool subsystem;
 	unsigned long quirks;
-- 
2.14.1

[PATCH v2] nvme: Honor RTD3 Entry Latency for shutdowns

[Christoph Hellwig]

On Wed, Aug 23, 2017@06:32:58PM -0400, Martin K. Petersen wrote:
> If an NVMe v1.2+ compliant controller reports RTD3 Entry Latency larger
> than shutdown_timeout, up to a maximum of 60 seconds, use that value to
> set the shutdown timer. Otherwise fall back to the module parameter
> which defaults to 5 seconds.

This generally looks fine, but NVMe allows TPs and thus new fields
to be implemented even against older spec versions, so we should avoid
version tests wherever possible.

> +	if (ctrl->vs >= NVME_VS(1, 2, 0)) {
> +		u32 transition_time = le32_to_cpu(id->rtd3e);

So instead of the version check here I'd check for a non-zero
RTD3E field instead.

[PATCH v2] nvme: Honor RTD3 Entry Latency for shutdowns

[Martin K. Petersen]

Christoph,

> This generally looks fine, but NVMe allows TPs and thus new fields
> to be implemented even against older spec versions, so we should avoid
> version tests wherever possible.

Fine with me. I was just mirroring the other version conditionals.

-- 
Martin K. Petersen	Oracle Linux Engineering

[PATCH v3] nvme: Honor RTD3 Entry Latency for shutdowns

[Martin K. Petersen]

If an NVMe controller reports RTD3 Entry Latency larger than
shutdown_timeout, up to a maximum of 60 seconds, use that value to set
the shutdown timer. Otherwise fall back to the module parameter which
defaults to 5 seconds.

Signed-off-by: Martin K. Petersen <martin.petersen at oracle.com>
---
 drivers/nvme/host/core.c | 16 +++++++++++++++-
 drivers/nvme/host/nvme.h |  1 +
 2 files changed, 16 insertions(+), 1 deletion(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index b7dfbbeccb47..775b702f9c91 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -1465,7 +1465,7 @@ EXPORT_SYMBOL_GPL(nvme_enable_ctrl);
 
 int nvme_shutdown_ctrl(struct nvme_ctrl *ctrl)
 {
-	unsigned long timeout = jiffies + (shutdown_timeout * HZ);
+	unsigned long timeout = jiffies + (ctrl->shutdown_timeout * HZ);
 	u32 csts;
 	int ret;
 
@@ -1765,6 +1765,7 @@ int nvme_init_identify(struct nvme_ctrl *ctrl)
 	int ret, page_shift;
 	u32 max_hw_sectors;
 	bool prev_apst_enabled;
+	u32 transition_time;
 
 	ret = ctrl->ops->reg_read32(ctrl, NVME_REG_VS, &ctrl->vs);
 	if (ret) {
@@ -1833,6 +1834,19 @@ int nvme_init_identify(struct nvme_ctrl *ctrl)
 	ctrl->sgls = le32_to_cpu(id->sgls);
 	ctrl->kas = le16_to_cpu(id->kas);
 
+	transition_time = le32_to_cpu(id->rtd3e);
+	if (transition_time) {
+		do_div(transition_time, 1000000); /* us -> s */
+		clamp(transition_time, shutdown_timeout, 60); /* max 60s */
+		ctrl->shutdown_timeout = transition_time;
+
+		if (transition_time != shutdown_timeout)
+			dev_warn(ctrl->device,
+				 "Shutdown timeout set to %u seconds\n",
+				 ctrl->shutdown_timeout);
+	} else
+		ctrl->shutdown_timeout = shutdown_timeout;
+
 	ctrl->npss = id->npss;
 	ctrl->apsta = id->apsta;
 	prev_apst_enabled = ctrl->apst_enabled;
diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
index 2c8a02be46fd..9b959ee18cb6 100644
--- a/drivers/nvme/host/nvme.h
+++ b/drivers/nvme/host/nvme.h
@@ -162,6 +162,7 @@ struct nvme_ctrl {
 	u16 kas;
 	u8 npss;
 	u8 apsta;
+	unsigned int shutdown_timeout;
 	unsigned int kato;
 	bool subsystem;
 	unsigned long quirks;
-- 
2.14.1

[PATCH v3] nvme: Honor RTD3 Entry Latency for shutdowns

[Christoph Hellwig]

On Thu, Aug 24, 2017@10:26:41PM -0400, Martin K. Petersen wrote:
> If an NVMe controller reports RTD3 Entry Latency larger than
> shutdown_timeout, up to a maximum of 60 seconds, use that value to set
> the shutdown timer. Otherwise fall back to the module parameter which
> defaults to 5 seconds.
> 
> Signed-off-by: Martin K. Petersen <martin.petersen at oracle.com>

Do you think this is urgent enough for 4.13?

[PATCH v3] nvme: Honor RTD3 Entry Latency for shutdowns

[Martin K. Petersen]

Christoph,

> On Thu, Aug 24, 2017@10:26:41PM -0400, Martin K. Petersen wrote:
>> If an NVMe controller reports RTD3 Entry Latency larger than
>> shutdown_timeout, up to a maximum of 60 seconds, use that value to set
>> the shutdown timer. Otherwise fall back to the module parameter which
>> defaults to 5 seconds.
>> 
>> Signed-off-by: Martin K. Petersen <martin.petersen at oracle.com>
>
> Do you think this is urgent enough for 4.13?

4.14 should be fine.

Thanks!

-- 
Martin K. Petersen	Oracle Linux Engineering

[PATCH v3] nvme: Honor RTD3 Entry Latency for shutdowns

[Keith Busch]

On Thu, Aug 24, 2017@10:26:41PM -0400, Martin K. Petersen wrote:
> +	transition_time = le32_to_cpu(id->rtd3e);
> +	if (transition_time) {
> +		do_div(transition_time, 1000000); /* us -> s */
> +		clamp(transition_time, shutdown_timeout, 60); /* max 60s */
> +		ctrl->shutdown_timeout = transition_time;

My first experience with the kernel's 'clamp', so I had to look up that
macro. The above does not appear to fit the usage since 'clamp' returns
a value rather than directly clamp the parameter; I think it should be:

	transition_time = clamp(transition_time, shutdown_timeout, 60);

[PATCH v4] nvme: Honor RTD3 Entry Latency for shutdowns

[Martin K. Petersen]

If an NVMe controller reports RTD3 Entry Latency larger than
shutdown_timeout, up to a maximum of 60 seconds, use that value to set
the shutdown timer. Otherwise fall back to the module parameter which
defaults to 5 seconds.

Signed-off-by: Martin K. Petersen <martin.petersen at oracle.com>
---
 drivers/nvme/host/core.c | 16 +++++++++++++++-
 drivers/nvme/host/nvme.h |  1 +
 2 files changed, 16 insertions(+), 1 deletion(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index b7dfbbeccb47..ea7bf12b563c 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -1465,7 +1465,7 @@ EXPORT_SYMBOL_GPL(nvme_enable_ctrl);
 
 int nvme_shutdown_ctrl(struct nvme_ctrl *ctrl)
 {
-	unsigned long timeout = jiffies + (shutdown_timeout * HZ);
+	unsigned long timeout = jiffies + (ctrl->shutdown_timeout * HZ);
 	u32 csts;
 	int ret;
 
@@ -1765,6 +1765,7 @@ int nvme_init_identify(struct nvme_ctrl *ctrl)
 	int ret, page_shift;
 	u32 max_hw_sectors;
 	bool prev_apst_enabled;
+	u32 transition_time;
 
 	ret = ctrl->ops->reg_read32(ctrl, NVME_REG_VS, &ctrl->vs);
 	if (ret) {
@@ -1833,6 +1834,19 @@ int nvme_init_identify(struct nvme_ctrl *ctrl)
 	ctrl->sgls = le32_to_cpu(id->sgls);
 	ctrl->kas = le16_to_cpu(id->kas);
 
+	transition_time = le32_to_cpu(id->rtd3e);
+	if (transition_time) {
+		do_div(transition_time, 1000000); /* us -> s */
+		ctrl->shutdown_timeout = clamp_t(unsigned int, transition_time,
+						 shutdown_timeout, 60);
+
+		if (ctrl->shutdown_timeout != shutdown_timeout)
+			dev_warn(ctrl->device,
+				 "Shutdown timeout set to %u seconds\n",
+				 ctrl->shutdown_timeout);
+	} else
+		ctrl->shutdown_timeout = shutdown_timeout;
+
 	ctrl->npss = id->npss;
 	ctrl->apsta = id->apsta;
 	prev_apst_enabled = ctrl->apst_enabled;
diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
index 2c8a02be46fd..9b959ee18cb6 100644
--- a/drivers/nvme/host/nvme.h
+++ b/drivers/nvme/host/nvme.h
@@ -162,6 +162,7 @@ struct nvme_ctrl {
 	u16 kas;
 	u8 npss;
 	u8 apsta;
+	unsigned int shutdown_timeout;
 	unsigned int kato;
 	bool subsystem;
 	unsigned long quirks;
-- 
2.14.1

[PATCH v3] nvme: Honor RTD3 Entry Latency for shutdowns

[Martin K. Petersen]

Keith,

> My first experience with the kernel's 'clamp', so I had to look up
> that macro. The above does not appear to fit the usage since 'clamp'
> returns a value rather than directly clamp the parameter; I think it
> should be:
>
> 	transition_time = clamp(transition_time, shutdown_timeout, 60);

Ah, my brain was seduced by the do_div() above.

Fixed and tested in qemu.

-- 
Martin K. Petersen	Oracle Linux Engineering

[PATCH v4] nvme: Honor RTD3 Entry Latency for shutdowns

[Sagi Grimberg]

Reviewed-by: Sagi Grimberg <sagi at grimberg.me>

[PATCH v4] nvme: Honor RTD3 Entry Latency for shutdowns

[Keith Busch]

On Fri, Aug 25, 2017@07:14:50PM -0400, Martin K. Petersen wrote:
> If an NVMe controller reports RTD3 Entry Latency larger than
> shutdown_timeout, up to a maximum of 60 seconds, use that value to set
> the shutdown timer. Otherwise fall back to the module parameter which
> defaults to 5 seconds.
> 
> Signed-off-by: Martin K. Petersen <martin.petersen at oracle.com>

Looks good,

Reviewed-by: Keith Busch <keith.busch at intel.com>