All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 1/2] nvme: Fix nvme reset command timeout handling
@ 2017-07-27 16:58 Keith Busch
  2017-07-27 16:58 ` [PATCH 2/2] nvme: Provide option to disable ASPT feature Keith Busch
  2017-07-28  7:58 ` [PATCH 1/2] nvme: Fix nvme reset command timeout handling Johannes Thumshirn
  0 siblings, 2 replies; 10+ messages in thread
From: Keith Busch @ 2017-07-27 16:58 UTC (permalink / raw)


We need to return an error if a timeout occurs on any NVMe command during
initialization. Without this, the nvme reset work will be stuck. A timeout
will have a negative error code, meaning we need to stop initializing
the controller. All postitive returns mean the controller is still usable.

bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=196325

Signed-off-by: Keith Busch <keith.busch at intel.com>
Cc: Martin Peres <martin.peres at intel.com>
---
 drivers/nvme/host/core.c | 23 ++++++++++++++++-------
 1 file changed, 16 insertions(+), 7 deletions(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 0c27f1a..b150702 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -1514,7 +1514,7 @@ static void nvme_set_queue_limits(struct nvme_ctrl *ctrl,
 	blk_queue_write_cache(q, vwc, vwc);
 }
 
-static void nvme_configure_apst(struct nvme_ctrl *ctrl)
+static int nvme_configure_apst(struct nvme_ctrl *ctrl)
 {
 	/*
 	 * APST (Autonomous Power State Transition) lets us program a
@@ -1543,16 +1543,16 @@ static void nvme_configure_apst(struct nvme_ctrl *ctrl)
 	 * then don't do anything.
 	 */
 	if (!ctrl->apsta)
-		return;
+		return 0;
 
 	if (ctrl->npss > 31) {
 		dev_warn(ctrl->device, "NPSS is invalid; not using APST\n");
-		return;
+		return 0;
 	}
 
 	table = kzalloc(sizeof(*table), GFP_KERNEL);
 	if (!table)
-		return;
+		return 0;
 
 	if (!ctrl->apst_enabled || ctrl->ps_max_latency_us == 0) {
 		/* Turn off APST. */
@@ -1634,6 +1634,7 @@ static void nvme_configure_apst(struct nvme_ctrl *ctrl)
 		dev_err(ctrl->device, "failed to set APST feature (%d)\n", ret);
 
 	kfree(table);
+	return ret;
 }
 
 static void nvme_set_latency_tolerance(struct device *dev, s32 val)
@@ -1856,17 +1857,25 @@ int nvme_init_identify(struct nvme_ctrl *ctrl)
 
 	kfree(id);
 
+	if (ret < 0)
+		return ret;
+
 	if (ctrl->apst_enabled && !prev_apst_enabled)
 		dev_pm_qos_expose_latency_tolerance(ctrl->device);
 	else if (!ctrl->apst_enabled && prev_apst_enabled)
 		dev_pm_qos_hide_latency_tolerance(ctrl->device);
 
-	nvme_configure_apst(ctrl);
-	nvme_configure_directives(ctrl);
+	ret = nvme_configure_apst(ctrl);
+	if (ret < 0)
+		return ret;
+
+	ret = nvme_configure_directives(ctrl);
+	if (ret < 0)
+		return ret;
 
 	ctrl->identified = true;
 
-	return ret;
+	return 0;
 }
 EXPORT_SYMBOL_GPL(nvme_init_identify);
 
-- 
2.5.5

^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH 2/2] nvme: Provide option to disable ASPT feature
  2017-07-27 16:58 [PATCH 1/2] nvme: Fix nvme reset command timeout handling Keith Busch
@ 2017-07-27 16:58 ` Keith Busch
  2017-07-27 17:28   ` Andy Lutomirski
  2017-07-28  7:58 ` [PATCH 1/2] nvme: Fix nvme reset command timeout handling Johannes Thumshirn
  1 sibling, 1 reply; 10+ messages in thread
From: Keith Busch @ 2017-07-27 16:58 UTC (permalink / raw)


Similar to PCIe's APSM, the NVMe driver needs the ability to not use APST
feature so that users have a way to prevent this feature from making their
machines unusable. This patch makes that possible via module parameter
"apst_off".

Signed-off-by: Keith Busch <keith.busch at intel.com>
Cc: Andy Lutomirski <luto at amacapital.net>
---
 drivers/nvme/host/core.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index b150702..834913b 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -64,6 +64,10 @@ static bool force_apst;
 module_param(force_apst, bool, 0644);
 MODULE_PARM_DESC(force_apst, "allow APST for newly enumerated devices even if quirked off");
 
+static bool apst_off;
+module_param(apst_off, bool, 0644);
+MODULE_PARM_DESC(apst_off, "Disable APST driver support");
+
 static bool streams;
 module_param(streams, bool, 0644);
 MODULE_PARM_DESC(streams, "turn on support for Streams write directives");
@@ -1542,7 +1546,7 @@ static int nvme_configure_apst(struct nvme_ctrl *ctrl)
 	 * If APST isn't supported or if we haven't been initialized yet,
 	 * then don't do anything.
 	 */
-	if (!ctrl->apsta)
+	if (!ctrl->apsta || apst_off)
 		return 0;
 
 	if (ctrl->npss > 31) {
-- 
2.5.5

^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH 2/2] nvme: Provide option to disable ASPT feature
  2017-07-27 16:58 ` [PATCH 2/2] nvme: Provide option to disable ASPT feature Keith Busch
@ 2017-07-27 17:28   ` Andy Lutomirski
  2017-07-27 18:09     ` Keith Busch
  0 siblings, 1 reply; 10+ messages in thread
From: Andy Lutomirski @ 2017-07-27 17:28 UTC (permalink / raw)




> On Jul 27, 2017,@12:58 PM, Keith Busch <keith.busch@intel.com> wrote:
> 
> Similar to PCIe's APSM, the NVMe driver needs the ability to not use APST
> feature so that users have a way to prevent this feature from making their
> machines unusable. This patch makes that possible via module parameter
> "apst_off".

Isn't this redundant with default_ps_max_latency=0?

> 
> Signed-off-by: Keith Busch <keith.busch at intel.com>
> Cc: Andy Lutomirski <luto at amacapital.net>
> ---
> drivers/nvme/host/core.c | 6 +++++-
> 1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> index b150702..834913b 100644
> --- a/drivers/nvme/host/core.c
> +++ b/drivers/nvme/host/core.c
> @@ -64,6 +64,10 @@ static bool force_apst;
> module_param(force_apst, bool, 0644);
> MODULE_PARM_DESC(force_apst, "allow APST for newly enumerated devices even if quirked off");
> 
> +static bool apst_off;
> +module_param(apst_off, bool, 0644);
> +MODULE_PARM_DESC(apst_off, "Disable APST driver support");
> +
> static bool streams;
> module_param(streams, bool, 0644);
> MODULE_PARM_DESC(streams, "turn on support for Streams write directives");
> @@ -1542,7 +1546,7 @@ static int nvme_configure_apst(struct nvme_ctrl *ctrl)
>     * If APST isn't supported or if we haven't been initialized yet,
>     * then don't do anything.
>     */
> -    if (!ctrl->apsta)
> +    if (!ctrl->apsta || apst_off)
>        return 0;
> 
>    if (ctrl->npss > 31) {
> -- 
> 2.5.5
> 

^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH 2/2] nvme: Provide option to disable ASPT feature
  2017-07-27 17:28   ` Andy Lutomirski
@ 2017-07-27 18:09     ` Keith Busch
  0 siblings, 0 replies; 10+ messages in thread
From: Keith Busch @ 2017-07-27 18:09 UTC (permalink / raw)


On Thu, Jul 27, 2017@01:28:20PM -0400, Andy Lutomirski wrote:
> 
> 
> > On Jul 27, 2017,@12:58 PM, Keith Busch <keith.busch@intel.com> wrote:
> > 
> > Similar to PCIe's APSM, the NVMe driver needs the ability to not use APST
> > feature so that users have a way to prevent this feature from making their
> > machines unusable. This patch makes that possible via module parameter
> > "apst_off".
> 
> Isn't this redundant with default_ps_max_latency=0?

Ah, thanks, so it is. I thought this wasn't considered through
nvme_init_identify, but ctrl->ps_max_latency_us would be 0.

This patch isn't needed, but we still need patch 1 in this series.

^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH 1/2] nvme: Fix nvme reset command timeout handling
  2017-07-27 16:58 [PATCH 1/2] nvme: Fix nvme reset command timeout handling Keith Busch
  2017-07-27 16:58 ` [PATCH 2/2] nvme: Provide option to disable ASPT feature Keith Busch
@ 2017-07-28  7:58 ` Johannes Thumshirn
  2017-08-10  8:38   ` Christoph Hellwig
  1 sibling, 1 reply; 10+ messages in thread
From: Johannes Thumshirn @ 2017-07-28  7:58 UTC (permalink / raw)


On Thu, Jul 27, 2017@12:58:40PM -0400, Keith Busch wrote:
>  static void nvme_set_latency_tolerance(struct device *dev, s32 val)
> @@ -1856,17 +1857,25 @@ int nvme_init_identify(struct nvme_ctrl *ctrl)
>  
>  	kfree(id);
>  
> +	if (ret < 0)
> +		return ret;
> +

This is a bit confusing IMHO, how about this?

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 3b77cfe5aa1e..71a97db3ce91 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -1835,13 +1835,16 @@ int nvme_init_identify(struct nvme_ctrl *ctrl)
 		 * In fabrics we need to verify the cntlid matches the
 		 * admin connect
 		 */
-		if (ctrl->cntlid != le16_to_cpu(id->cntlid))
+		if (ctrl->cntlid != le16_to_cpu(id->cntlid)) {
 			ret = -EINVAL;
+			goto free_id;
+		}
 
 		if (!ctrl->opts->discovery_nqn && !ctrl->kas) {
 			dev_err(ctrl->device,
 				"keep-alive support is mandatory for fabrics\n");
 			ret = -EINVAL;
+			goto free_id;
 		}
 	} else {
 		ctrl->cntlid = le16_to_cpu(id->cntlid);
@@ -1862,6 +1865,10 @@ int nvme_init_identify(struct nvme_ctrl *ctrl)
 	ctrl->identified = true;
 
 	return ret;
+
+free_id:
+	kfree(id);
+	return ret;
 }
 EXPORT_SYMBOL_GPL(nvme_init_identify);

Or ditch the goto and do a kfree(id); return ret; directly in the if blocks?

Byte,
	Johannes
 
-- 
Johannes Thumshirn                                          Storage
jthumshirn at suse.de                                +49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 N?rnberg
GF: Felix Imend?rffer, Jane Smithard, Graham Norton
HRB 21284 (AG N?rnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850

^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH 1/2] nvme: Fix nvme reset command timeout handling
  2017-07-28  7:58 ` [PATCH 1/2] nvme: Fix nvme reset command timeout handling Johannes Thumshirn
@ 2017-08-10  8:38   ` Christoph Hellwig
  2017-08-10  8:48     ` Johannes Thumshirn
  0 siblings, 1 reply; 10+ messages in thread
From: Christoph Hellwig @ 2017-08-10  8:38 UTC (permalink / raw)


On Fri, Jul 28, 2017@09:58:34AM +0200, Johannes Thumshirn wrote:
> On Thu, Jul 27, 2017@12:58:40PM -0400, Keith Busch wrote:
> >  static void nvme_set_latency_tolerance(struct device *dev, s32 val)
> > @@ -1856,17 +1857,25 @@ int nvme_init_identify(struct nvme_ctrl *ctrl)
> >  
> >  	kfree(id);
> >  
> > +	if (ret < 0)
> > +		return ret;
> > +
> 
> This is a bit confusing IMHO, how about this?

Agreed.

We probably should get this series into 4.13, so other comments would
be welcome.

^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH 1/2] nvme: Fix nvme reset command timeout handling
  2017-08-10  8:38   ` Christoph Hellwig
@ 2017-08-10  8:48     ` Johannes Thumshirn
  2017-08-10  8:57       ` Christoph Hellwig
  0 siblings, 1 reply; 10+ messages in thread
From: Johannes Thumshirn @ 2017-08-10  8:48 UTC (permalink / raw)


On Thu, Aug 10, 2017@10:38:27AM +0200, Christoph Hellwig wrote:
> Agreed.
> 
> We probably should get this series into 4.13, so other comments would
> be welcome.

So with my proposed change I'm fine with it, so 
Reviewed-by: Johannes Thumshirn <jthumshirn at suse.de>
IFF the change is made. 

Patch 2 is unneeded as Luto stated.

Thanks,
	Johannes
-- 
Johannes Thumshirn                                          Storage
jthumshirn at suse.de                                +49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 N?rnberg
GF: Felix Imend?rffer, Jane Smithard, Graham Norton
HRB 21284 (AG N?rnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850

^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH 1/2] nvme: Fix nvme reset command timeout handling
  2017-08-10  8:48     ` Johannes Thumshirn
@ 2017-08-10  8:57       ` Christoph Hellwig
  2017-08-10  9:05         ` Johannes Thumshirn
  2017-08-10 16:38         ` Keith Busch
  0 siblings, 2 replies; 10+ messages in thread
From: Christoph Hellwig @ 2017-08-10  8:57 UTC (permalink / raw)


On Thu, Aug 10, 2017@10:48:18AM +0200, Johannes Thumshirn wrote:
> On Thu, Aug 10, 2017@10:38:27AM +0200, Christoph Hellwig wrote:
> > Agreed.
> > 
> > We probably should get this series into 4.13, so other comments would
> > be welcome.
> 
> So with my proposed change I'm fine with it, so 
> Reviewed-by: Johannes Thumshirn <jthumshirn at suse.de>
> IFF the change is made. 

Given that you're awake now: can you resend a full patch with your
proposed changes included so that it can be tested when the Americans
get up? :)

^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH 1/2] nvme: Fix nvme reset command timeout handling
  2017-08-10  8:57       ` Christoph Hellwig
@ 2017-08-10  9:05         ` Johannes Thumshirn
  2017-08-10 16:38         ` Keith Busch
  1 sibling, 0 replies; 10+ messages in thread
From: Johannes Thumshirn @ 2017-08-10  9:05 UTC (permalink / raw)


On Thu, Aug 10, 2017@10:57:16AM +0200, Christoph Hellwig wrote:
> On Thu, Aug 10, 2017@10:48:18AM +0200, Johannes Thumshirn wrote:
> > On Thu, Aug 10, 2017@10:38:27AM +0200, Christoph Hellwig wrote:
> > > Agreed.
> > > 
> > > We probably should get this series into 4.13, so other comments would
> > > be welcome.
> > 
> > So with my proposed change I'm fine with it, so 
> > Reviewed-by: Johannes Thumshirn <jthumshirn at suse.de>
> > IFF the change is made. 
> 
> Given that you're awake now: can you resend a full patch with your
> proposed changes included so that it can be tested when the Americans
> get up? :)

It was originally Keith's patch, but sure, if you want me to.

-- 
Johannes Thumshirn                                          Storage
jthumshirn at suse.de                                +49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 N?rnberg
GF: Felix Imend?rffer, Jane Smithard, Graham Norton
HRB 21284 (AG N?rnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850

^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH 1/2] nvme: Fix nvme reset command timeout handling
  2017-08-10  8:57       ` Christoph Hellwig
  2017-08-10  9:05         ` Johannes Thumshirn
@ 2017-08-10 16:38         ` Keith Busch
  1 sibling, 0 replies; 10+ messages in thread
From: Keith Busch @ 2017-08-10 16:38 UTC (permalink / raw)


On Thu, Aug 10, 2017@10:57:16AM +0200, Christoph Hellwig wrote:
> On Thu, Aug 10, 2017@10:48:18AM +0200, Johannes Thumshirn wrote:
> > On Thu, Aug 10, 2017@10:38:27AM +0200, Christoph Hellwig wrote:
> > > Agreed.
> > > 
> > > We probably should get this series into 4.13, so other comments would
> > > be welcome.
> > 
> > So with my proposed change I'm fine with it, so 
> > Reviewed-by: Johannes Thumshirn <jthumshirn at suse.de>
> > IFF the change is made. 
> 
> Given that you're awake now: can you resend a full patch with your
> proposed changes included so that it can be tested when the Americans
> get up? :)

I'm back in America now! A typhoon caused some trouble for Pacific air
travel, and I lost 1.5 days as a result. Back to normalcy, and thank
you for resending the fixed up patch. :)

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2017-08-10 16:38 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-07-27 16:58 [PATCH 1/2] nvme: Fix nvme reset command timeout handling Keith Busch
2017-07-27 16:58 ` [PATCH 2/2] nvme: Provide option to disable ASPT feature Keith Busch
2017-07-27 17:28   ` Andy Lutomirski
2017-07-27 18:09     ` Keith Busch
2017-07-28  7:58 ` [PATCH 1/2] nvme: Fix nvme reset command timeout handling Johannes Thumshirn
2017-08-10  8:38   ` Christoph Hellwig
2017-08-10  8:48     ` Johannes Thumshirn
2017-08-10  8:57       ` Christoph Hellwig
2017-08-10  9:05         ` Johannes Thumshirn
2017-08-10 16:38         ` Keith Busch

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.