All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] NVMe: init nvme queue before enabling irq
@ 2016-01-20  4:57 ` Wenbo Wang
  0 siblings, 0 replies; 13+ messages in thread
From: Wenbo Wang @ 2016-01-20  4:57 UTC (permalink / raw)
  To: keith.busch, axboe
  Cc: linux-nvme, linux-kernel, wenwei.tao, Wenbo Wang, Wenbo Wang

During reset process, the nvme_dev->bar (ioremapped) may change,
so nvmeq->q_db shall be also updated by nvme_init_queue().

Currently nvmeq irq is enabled before queue init, so a spurious
interrupt triggered nvme_process_cq may access nvmeq->q_db just
before it is updated, this could cause kernel panic.

Signed-off-by: Wenbo Wang <wenbo.wang@memblaze.com>
Reviewed-by: Wenwei Tao <wenwei.tao@memblaze.com>
---
 drivers/nvme/host/pci.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index f5c0e26..df55f28 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -1590,11 +1590,17 @@ static int nvme_create_queue(struct nvme_queue *nvmeq, int qid)
 	if (result < 0)
 		goto release_cq;
 
+	/*
+	 * Init queue door bell ioremap address before enabling irq, if not,
+	 * a spurious interrupt triggered nvme_process_cq may access invalid
+	 * address
+	 */
+	nvme_init_queue(nvmeq, qid);
+
 	result = queue_request_irq(dev, nvmeq, nvmeq->irqname);
 	if (result < 0)
 		goto release_sq;
 
-	nvme_init_queue(nvmeq, qid);
 	return result;
 
  release_sq:
@@ -1789,6 +1795,8 @@ static int nvme_configure_admin_queue(struct nvme_dev *dev)
 	if (result)
 		goto free_nvmeq;
 
+	nvme_init_queue(nvmeq, 0);
+
 	nvmeq->cq_vector = 0;
 	result = queue_request_irq(dev, nvmeq, nvmeq->irqname);
 	if (result) {
@@ -3164,7 +3172,6 @@ static void nvme_probe_work(struct work_struct *work)
 		goto disable;
 	}
 
-	nvme_init_queue(dev->queues[0], 0);
 	result = nvme_alloc_admin_tags(dev);
 	if (result)
 		goto disable;
-- 
1.8.3.1

^ permalink raw reply related	[flat|nested] 13+ messages in thread
* [PATCH] NVMe: init nvme queue before enabling irq
@ 2016-01-20  4:57 ` Wenbo Wang
  0 siblings, 0 replies; 13+ messages in thread
From: Wenbo Wang @ 2016-01-20  4:57 UTC (permalink / raw)


During reset process, the nvme_dev->bar (ioremapped) may change,
so nvmeq->q_db shall be also updated by nvme_init_queue().

Currently nvmeq irq is enabled before queue init, so a spurious
interrupt triggered nvme_process_cq may access nvmeq->q_db just
before it is updated, this could cause kernel panic.

Signed-off-by: Wenbo Wang <wenbo.wang at memblaze.com>
Reviewed-by: Wenwei Tao <wenwei.tao at memblaze.com>
---
 drivers/nvme/host/pci.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index f5c0e26..df55f28 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -1590,11 +1590,17 @@ static int nvme_create_queue(struct nvme_queue *nvmeq, int qid)
 	if (result < 0)
 		goto release_cq;
 
+	/*
+	 * Init queue door bell ioremap address before enabling irq, if not,
+	 * a spurious interrupt triggered nvme_process_cq may access invalid
+	 * address
+	 */
+	nvme_init_queue(nvmeq, qid);
+
 	result = queue_request_irq(dev, nvmeq, nvmeq->irqname);
 	if (result < 0)
 		goto release_sq;
 
-	nvme_init_queue(nvmeq, qid);
 	return result;
 
  release_sq:
@@ -1789,6 +1795,8 @@ static int nvme_configure_admin_queue(struct nvme_dev *dev)
 	if (result)
 		goto free_nvmeq;
 
+	nvme_init_queue(nvmeq, 0);
+
 	nvmeq->cq_vector = 0;
 	result = queue_request_irq(dev, nvmeq, nvmeq->irqname);
 	if (result) {
@@ -3164,7 +3172,6 @@ static void nvme_probe_work(struct work_struct *work)
 		goto disable;
 	}
 
-	nvme_init_queue(dev->queues[0], 0);
 	result = nvme_alloc_admin_tags(dev);
 	if (result)
 		goto disable;
-- 
1.8.3.1

^ permalink raw reply related	[flat|nested] 13+ messages in thread
* Re: [PATCH] NVMe: init nvme queue before enabling irq
  2016-01-20  4:57 ` Wenbo Wang
@ 2016-01-20  9:06   ` Sagi Grimberg
  -1 siblings, 0 replies; 13+ messages in thread
From: Sagi Grimberg @ 2016-01-20  9:06 UTC (permalink / raw)
  To: Wenbo Wang, keith.busch, axboe
  Cc: Wenbo Wang, wenwei.tao, linux-kernel, linux-nvme


> During reset process, the nvme_dev->bar (ioremapped) may change,
> so nvmeq->q_db shall be also updated by nvme_init_queue().
>
> Currently nvmeq irq is enabled before queue init, so a spurious
> interrupt triggered nvme_process_cq may access nvmeq->q_db just
> before it is updated, this could cause kernel panic.
>
> Signed-off-by: Wenbo Wang <wenbo.wang@memblaze.com>
> Reviewed-by: Wenwei Tao <wenwei.tao@memblaze.com>

This patch makes sense to me.

> ---
>   drivers/nvme/host/pci.c | 11 +++++++++--
>   1 file changed, 9 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
> index f5c0e26..df55f28 100644
> --- a/drivers/nvme/host/pci.c
> +++ b/drivers/nvme/host/pci.c
> @@ -1590,11 +1590,17 @@ static int nvme_create_queue(struct nvme_queue *nvmeq, int qid)
>   	if (result < 0)
>   		goto release_cq;
>
> +	/*
> +	 * Init queue door bell ioremap address before enabling irq, if not,
> +	 * a spurious interrupt triggered nvme_process_cq may access invalid
> +	 * address
> +	 */

Not sure we need an explicit comment on the fix here. But I don't
mind keeping it.

> +	nvme_init_queue(nvmeq, qid);
> +

I think that this makes the duplication in nvme_alloc_queue redundant.
Can you remove it?

Other then that, looks good to me,

Reviewed-by: Sagi Grimberg <sagig@mellanox.com>

^ permalink raw reply	[flat|nested] 13+ messages in thread
* [PATCH] NVMe: init nvme queue before enabling irq
@ 2016-01-20  9:06   ` Sagi Grimberg
  0 siblings, 0 replies; 13+ messages in thread
From: Sagi Grimberg @ 2016-01-20  9:06 UTC (permalink / raw)



> During reset process, the nvme_dev->bar (ioremapped) may change,
> so nvmeq->q_db shall be also updated by nvme_init_queue().
>
> Currently nvmeq irq is enabled before queue init, so a spurious
> interrupt triggered nvme_process_cq may access nvmeq->q_db just
> before it is updated, this could cause kernel panic.
>
> Signed-off-by: Wenbo Wang <wenbo.wang at memblaze.com>
> Reviewed-by: Wenwei Tao <wenwei.tao at memblaze.com>

This patch makes sense to me.

> ---
>   drivers/nvme/host/pci.c | 11 +++++++++--
>   1 file changed, 9 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
> index f5c0e26..df55f28 100644
> --- a/drivers/nvme/host/pci.c
> +++ b/drivers/nvme/host/pci.c
> @@ -1590,11 +1590,17 @@ static int nvme_create_queue(struct nvme_queue *nvmeq, int qid)
>   	if (result < 0)
>   		goto release_cq;
>
> +	/*
> +	 * Init queue door bell ioremap address before enabling irq, if not,
> +	 * a spurious interrupt triggered nvme_process_cq may access invalid
> +	 * address
> +	 */

Not sure we need an explicit comment on the fix here. But I don't
mind keeping it.

> +	nvme_init_queue(nvmeq, qid);
> +

I think that this makes the duplication in nvme_alloc_queue redundant.
Can you remove it?

Other then that, looks good to me,

Reviewed-by: Sagi Grimberg <sagig at mellanox.com>

^ permalink raw reply	[flat|nested] 13+ messages in thread
* Re: [PATCH] NVMe: init nvme queue before enabling irq
  2016-01-20  4:57 ` Wenbo Wang
@ 2016-01-20  9:09   ` Johannes Thumshirn
  -1 siblings, 0 replies; 13+ messages in thread
From: Johannes Thumshirn @ 2016-01-20  9:09 UTC (permalink / raw)
  To: Wenbo Wang
  Cc: keith.busch, axboe, Wenbo Wang, wenwei.tao, linux-kernel, linux-nvme

On Tue, Jan 19, 2016 at 11:57:40PM -0500, Wenbo Wang wrote:
> During reset process, the nvme_dev->bar (ioremapped) may change,
> so nvmeq->q_db shall be also updated by nvme_init_queue().
> 
> Currently nvmeq irq is enabled before queue init, so a spurious
> interrupt triggered nvme_process_cq may access nvmeq->q_db just
> before it is updated, this could cause kernel panic.

If it can cause a kernel panic shouldn't it go through stable then as well?

> 
> Signed-off-by: Wenbo Wang <wenbo.wang@memblaze.com>
> Reviewed-by: Wenwei Tao <wenwei.tao@memblaze.com>
> ---
>  drivers/nvme/host/pci.c | 11 +++++++++--
>  1 file changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
> index f5c0e26..df55f28 100644
> --- a/drivers/nvme/host/pci.c
> +++ b/drivers/nvme/host/pci.c
> @@ -1590,11 +1590,17 @@ static int nvme_create_queue(struct nvme_queue *nvmeq, int qid)
>  	if (result < 0)
>  		goto release_cq;
>  
> +	/*
> +	 * Init queue door bell ioremap address before enabling irq, if not,
> +	 * a spurious interrupt triggered nvme_process_cq may access invalid
> +	 * address
> +	 */
> +	nvme_init_queue(nvmeq, qid);
> +
>  	result = queue_request_irq(dev, nvmeq, nvmeq->irqname);
>  	if (result < 0)
>  		goto release_sq;
>  
> -	nvme_init_queue(nvmeq, qid);
>  	return result;
>  
>   release_sq:
> @@ -1789,6 +1795,8 @@ static int nvme_configure_admin_queue(struct nvme_dev *dev)
>  	if (result)
>  		goto free_nvmeq;
>  
> +	nvme_init_queue(nvmeq, 0);
> +
>  	nvmeq->cq_vector = 0;
>  	result = queue_request_irq(dev, nvmeq, nvmeq->irqname);
>  	if (result) {
> @@ -3164,7 +3172,6 @@ static void nvme_probe_work(struct work_struct *work)
>  		goto disable;
>  	}
>  
> -	nvme_init_queue(dev->queues[0], 0);
>  	result = nvme_alloc_admin_tags(dev);
>  	if (result)
>  		goto disable;
> -- 
> 1.8.3.1
> 
> 
> 
> _______________________________________________
> Linux-nvme mailing list
> Linux-nvme@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-nvme

Other than that

Reviewed-by: Johannes Thumshirn <jthumshirn@suse.de>

-- 
Johannes Thumshirn                                          Storage
jthumshirn@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] 13+ messages in thread
* [PATCH] NVMe: init nvme queue before enabling irq
@ 2016-01-20  9:09   ` Johannes Thumshirn
  0 siblings, 0 replies; 13+ messages in thread
From: Johannes Thumshirn @ 2016-01-20  9:09 UTC (permalink / raw)


On Tue, Jan 19, 2016@11:57:40PM -0500, Wenbo Wang wrote:
> During reset process, the nvme_dev->bar (ioremapped) may change,
> so nvmeq->q_db shall be also updated by nvme_init_queue().
> 
> Currently nvmeq irq is enabled before queue init, so a spurious
> interrupt triggered nvme_process_cq may access nvmeq->q_db just
> before it is updated, this could cause kernel panic.

If it can cause a kernel panic shouldn't it go through stable then as well?

> 
> Signed-off-by: Wenbo Wang <wenbo.wang at memblaze.com>
> Reviewed-by: Wenwei Tao <wenwei.tao at memblaze.com>
> ---
>  drivers/nvme/host/pci.c | 11 +++++++++--
>  1 file changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
> index f5c0e26..df55f28 100644
> --- a/drivers/nvme/host/pci.c
> +++ b/drivers/nvme/host/pci.c
> @@ -1590,11 +1590,17 @@ static int nvme_create_queue(struct nvme_queue *nvmeq, int qid)
>  	if (result < 0)
>  		goto release_cq;
>  
> +	/*
> +	 * Init queue door bell ioremap address before enabling irq, if not,
> +	 * a spurious interrupt triggered nvme_process_cq may access invalid
> +	 * address
> +	 */
> +	nvme_init_queue(nvmeq, qid);
> +
>  	result = queue_request_irq(dev, nvmeq, nvmeq->irqname);
>  	if (result < 0)
>  		goto release_sq;
>  
> -	nvme_init_queue(nvmeq, qid);
>  	return result;
>  
>   release_sq:
> @@ -1789,6 +1795,8 @@ static int nvme_configure_admin_queue(struct nvme_dev *dev)
>  	if (result)
>  		goto free_nvmeq;
>  
> +	nvme_init_queue(nvmeq, 0);
> +
>  	nvmeq->cq_vector = 0;
>  	result = queue_request_irq(dev, nvmeq, nvmeq->irqname);
>  	if (result) {
> @@ -3164,7 +3172,6 @@ static void nvme_probe_work(struct work_struct *work)
>  		goto disable;
>  	}
>  
> -	nvme_init_queue(dev->queues[0], 0);
>  	result = nvme_alloc_admin_tags(dev);
>  	if (result)
>  		goto disable;
> -- 
> 1.8.3.1
> 
> 
> 
> _______________________________________________
> Linux-nvme mailing list
> Linux-nvme at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-nvme

Other than that

Reviewed-by: Johannes Thumshirn <jthumshirn at suse.de>

-- 
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] 13+ messages in thread
* [PATCH] NVMe: init nvme queue before enabling irq
  2016-01-20  9:09   ` Johannes Thumshirn
  (?)
@ 2016-01-20  9:45   ` Wenbo Wang
  2016-01-20  9:56       ` Sagi Grimberg
  -1 siblings, 1 reply; 13+ messages in thread
From: Wenbo Wang @ 2016-01-20  9:45 UTC (permalink / raw)


> If it can cause a kernel panic shouldn't it go through stable then as well?

Sorry, not quite understand this comment.
The "reset process" is the nvme device reset process (performed by nvme_reset_work()) triggered by device fail condition.
During normal boot up, nvmeq door bell is initialized in nvme_alloc_queue() which happens before enabling irq, so there is no error.
During nvme device reset process, nvme_alloc_queue() is skipped and the race condition exists.

-----Original Message-----
From: linux-kernel-owner@vger.kernel.org [mailto:linux-kernel-owner@vger.kernel.org] On Behalf Of Johannes Thumshirn
Sent: Wednesday, January 20, 2016 5:10 PM
To: Wenbo Wang
Cc: keith.busch at intel.com; axboe at fb.com; Wenbo Wang; Wenwei.Tao; linux-kernel at vger.kernel.org; linux-nvme at lists.infradead.org
Subject: Re: [PATCH] NVMe: init nvme queue before enabling irq

On Tue, Jan 19, 2016@11:57:40PM -0500, Wenbo Wang wrote:
> During reset process, the nvme_dev->bar (ioremapped) may change, so 
> nvmeq->q_db shall be also updated by nvme_init_queue().
> 
> Currently nvmeq irq is enabled before queue init, so a spurious 
> interrupt triggered nvme_process_cq may access nvmeq->q_db just before 
> it is updated, this could cause kernel panic.

If it can cause a kernel panic shouldn't it go through stable then as well?

> 
> Signed-off-by: Wenbo Wang <wenbo.wang at memblaze.com>
> Reviewed-by: Wenwei Tao <wenwei.tao at memblaze.com>
> ---
>  drivers/nvme/host/pci.c | 11 +++++++++--
>  1 file changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c index 
> f5c0e26..df55f28 100644
> --- a/drivers/nvme/host/pci.c
> +++ b/drivers/nvme/host/pci.c
> @@ -1590,11 +1590,17 @@ static int nvme_create_queue(struct nvme_queue *nvmeq, int qid)
>  	if (result < 0)
>  		goto release_cq;
>  
> +	/*
> +	 * Init queue door bell ioremap address before enabling irq, if not,
> +	 * a spurious interrupt triggered nvme_process_cq may access invalid
> +	 * address
> +	 */
> +	nvme_init_queue(nvmeq, qid);
> +
>  	result = queue_request_irq(dev, nvmeq, nvmeq->irqname);
>  	if (result < 0)
>  		goto release_sq;
>  
> -	nvme_init_queue(nvmeq, qid);
>  	return result;
>  
>   release_sq:
> @@ -1789,6 +1795,8 @@ static int nvme_configure_admin_queue(struct nvme_dev *dev)
>  	if (result)
>  		goto free_nvmeq;
>  
> +	nvme_init_queue(nvmeq, 0);
> +
>  	nvmeq->cq_vector = 0;
>  	result = queue_request_irq(dev, nvmeq, nvmeq->irqname);
>  	if (result) {
> @@ -3164,7 +3172,6 @@ static void nvme_probe_work(struct work_struct *work)
>  		goto disable;
>  	}
>  
> -	nvme_init_queue(dev->queues[0], 0);
>  	result = nvme_alloc_admin_tags(dev);
>  	if (result)
>  		goto disable;
> --
> 1.8.3.1
> 
> 
> 
> _______________________________________________
> Linux-nvme mailing list
> Linux-nvme at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-nvme

Other than that

Reviewed-by: Johannes Thumshirn <jthumshirn at suse.de>

-- 
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] 13+ messages in thread
* Re: [PATCH] NVMe: init nvme queue before enabling irq
  2016-01-20  9:45   ` Wenbo Wang
@ 2016-01-20  9:56       ` Sagi Grimberg
  0 siblings, 0 replies; 13+ messages in thread
From: Sagi Grimberg @ 2016-01-20  9:56 UTC (permalink / raw)
  To: Wenbo Wang, Johannes Thumshirn, Wenbo Wang
  Cc: keith.busch, axboe, Wenwei.Tao, linux-kernel, linux-nvme


>> If it can cause a kernel panic shouldn't it go through stable then as well?
>
> Sorry, not quite understand this comment.
> The "reset process" is the nvme device reset process (performed by nvme_reset_work()) triggered by device fail condition.
> During normal boot up, nvmeq door bell is initialized in nvme_alloc_queue() which happens before enabling irq, so there is no error.
> During nvme device reset process, nvme_alloc_queue() is skipped and the race condition exists.

I think what Johannes meant was that this patch should include a
"CC: stable@vger.kernel.org" tag.

^ permalink raw reply	[flat|nested] 13+ messages in thread
* [PATCH] NVMe: init nvme queue before enabling irq
@ 2016-01-20  9:56       ` Sagi Grimberg
  0 siblings, 0 replies; 13+ messages in thread
From: Sagi Grimberg @ 2016-01-20  9:56 UTC (permalink / raw)



>> If it can cause a kernel panic shouldn't it go through stable then as well?
>
> Sorry, not quite understand this comment.
> The "reset process" is the nvme device reset process (performed by nvme_reset_work()) triggered by device fail condition.
> During normal boot up, nvmeq door bell is initialized in nvme_alloc_queue() which happens before enabling irq, so there is no error.
> During nvme device reset process, nvme_alloc_queue() is skipped and the race condition exists.

I think what Johannes meant was that this patch should include a
"CC: stable at vger.kernel.org" tag.

^ permalink raw reply	[flat|nested] 13+ messages in thread
* Re: [PATCH] NVMe: init nvme queue before enabling irq
  2016-01-20  9:56       ` Sagi Grimberg
@ 2016-01-20 10:22         ` Johannes Thumshirn
  -1 siblings, 0 replies; 13+ messages in thread
From: Johannes Thumshirn @ 2016-01-20 10:22 UTC (permalink / raw)
  To: Sagi Grimberg
  Cc: Wenbo Wang, Wenbo Wang, keith.busch, axboe, Wenwei.Tao,
	linux-kernel, linux-nvme

On Wed, Jan 20, 2016 at 11:56:01AM +0200, Sagi Grimberg wrote:
> 
> >>If it can cause a kernel panic shouldn't it go through stable then as well?
> >
> >Sorry, not quite understand this comment.
> >The "reset process" is the nvme device reset process (performed by nvme_reset_work()) triggered by device fail condition.
> >During normal boot up, nvmeq door bell is initialized in nvme_alloc_queue() which happens before enabling irq, so there is no error.
> >During nvme device reset process, nvme_alloc_queue() is skipped and the race condition exists.
> 
> I think what Johannes meant was that this patch should include a
> "CC: stable@vger.kernel.org" tag.
> 

Exactly. This makes work for us distribution people a lot easier (i.e. we do
not have to manually scan all commits and decide if we need to backport a
patch or not)

Thanks

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

-- 
Johannes Thumshirn                                          Storage
jthumshirn@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] 13+ messages in thread
* [PATCH] NVMe: init nvme queue before enabling irq
@ 2016-01-20 10:22         ` Johannes Thumshirn
  0 siblings, 0 replies; 13+ messages in thread
From: Johannes Thumshirn @ 2016-01-20 10:22 UTC (permalink / raw)


On Wed, Jan 20, 2016@11:56:01AM +0200, Sagi Grimberg wrote:
> 
> >>If it can cause a kernel panic shouldn't it go through stable then as well?
> >
> >Sorry, not quite understand this comment.
> >The "reset process" is the nvme device reset process (performed by nvme_reset_work()) triggered by device fail condition.
> >During normal boot up, nvmeq door bell is initialized in nvme_alloc_queue() which happens before enabling irq, so there is no error.
> >During nvme device reset process, nvme_alloc_queue() is skipped and the race condition exists.
> 
> I think what Johannes meant was that this patch should include a
> "CC: stable at vger.kernel.org" tag.
> 

Exactly. This makes work for us distribution people a lot easier (i.e. we do
not have to manually scan all commits and decide if we need to backport a
patch or not)

Thanks

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

-- 
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] 13+ messages in thread
* [PATCH] NVMe: init nvme queue before enabling irq
  2016-01-20 10:22         ` Johannes Thumshirn
  (?)
@ 2016-01-20 10:41         ` Wenbo Wang
  -1 siblings, 0 replies; 13+ messages in thread
From: Wenbo Wang @ 2016-01-20 10:41 UTC (permalink / raw)


Thanks for this information. I'll add this.

-----Original Message-----
From: linux-kernel-owner@vger.kernel.org [mailto:linux-kernel-owner@vger.kernel.org] On Behalf Of Johannes Thumshirn
Sent: Wednesday, January 20, 2016 6:22 PM
To: Sagi Grimberg
Cc: Wenbo Wang; Wenbo Wang; keith.busch at intel.com; axboe at fb.com; Wenwei.Tao; linux-kernel at vger.kernel.org; linux-nvme at lists.infradead.org
Subject: Re: [PATCH] NVMe: init nvme queue before enabling irq

On Wed, Jan 20, 2016@11:56:01AM +0200, Sagi Grimberg wrote:
> 
> >>If it can cause a kernel panic shouldn't it go through stable then as well?
> >
> >Sorry, not quite understand this comment.
> >The "reset process" is the nvme device reset process (performed by nvme_reset_work()) triggered by device fail condition.
> >During normal boot up, nvmeq door bell is initialized in nvme_alloc_queue() which happens before enabling irq, so there is no error.
> >During nvme device reset process, nvme_alloc_queue() is skipped and the race condition exists.
> 
> I think what Johannes meant was that this patch should include a
> "CC: stable at vger.kernel.org" tag.
> 

Exactly. This makes work for us distribution people a lot easier (i.e. we do not have to manually scan all commits and decide if we need to backport a patch or not)

Thanks

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

-- 
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] 13+ messages in thread
* [PATCH] NVMe: init nvme queue before enabling irq
  2016-01-20  9:06   ` Sagi Grimberg
  (?)
@ 2016-01-20 10:52   ` Wenbo Wang
  -1 siblings, 0 replies; 13+ messages in thread
From: Wenbo Wang @ 2016-01-20 10:52 UTC (permalink / raw)


> I think that this makes the duplication in nvme_alloc_queue redundant.
> Can you remove it?

Thanks. Patch v2 has been sent out.

-----Original Message-----
From: Sagi Grimberg [mailto:sagig@dev.mellanox.co.il] 
Sent: Wednesday, January 20, 2016 5:06 PM
To: Wenbo Wang; keith.busch at intel.com; axboe at fb.com
Cc: Wenbo Wang; Wenwei.Tao; linux-kernel at vger.kernel.org; linux-nvme at lists.infradead.org
Subject: Re: [PATCH] NVMe: init nvme queue before enabling irq


> During reset process, the nvme_dev->bar (ioremapped) may change, so 
> nvmeq->q_db shall be also updated by nvme_init_queue().
>
> Currently nvmeq irq is enabled before queue init, so a spurious 
> interrupt triggered nvme_process_cq may access nvmeq->q_db just before 
> it is updated, this could cause kernel panic.
>
> Signed-off-by: Wenbo Wang <wenbo.wang at memblaze.com>
> Reviewed-by: Wenwei Tao <wenwei.tao at memblaze.com>

This patch makes sense to me.

> ---
>   drivers/nvme/host/pci.c | 11 +++++++++--
>   1 file changed, 9 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c index 
> f5c0e26..df55f28 100644
> --- a/drivers/nvme/host/pci.c
> +++ b/drivers/nvme/host/pci.c
> @@ -1590,11 +1590,17 @@ static int nvme_create_queue(struct nvme_queue *nvmeq, int qid)
>   	if (result < 0)
>   		goto release_cq;
>
> +	/*
> +	 * Init queue door bell ioremap address before enabling irq, if not,
> +	 * a spurious interrupt triggered nvme_process_cq may access invalid
> +	 * address
> +	 */

Not sure we need an explicit comment on the fix here. But I don't mind keeping it.

> +	nvme_init_queue(nvmeq, qid);
> +

I think that this makes the duplication in nvme_alloc_queue redundant.
Can you remove it?

Other then that, looks good to me,

Reviewed-by: Sagi Grimberg <sagig at mellanox.com>

^ permalink raw reply	[flat|nested] 13+ messages in thread
end of thread, other threads:[~2016-01-20 10:52 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-01-20  4:57 [PATCH] NVMe: init nvme queue before enabling irq Wenbo Wang
2016-01-20  4:57 ` Wenbo Wang
2016-01-20  9:06 ` Sagi Grimberg
2016-01-20  9:06   ` Sagi Grimberg
2016-01-20 10:52   ` Wenbo Wang
2016-01-20  9:09 ` Johannes Thumshirn
2016-01-20  9:09   ` Johannes Thumshirn
2016-01-20  9:45   ` Wenbo Wang
2016-01-20  9:56     ` Sagi Grimberg
2016-01-20  9:56       ` Sagi Grimberg
2016-01-20 10:22       ` Johannes Thumshirn
2016-01-20 10:22         ` Johannes Thumshirn
2016-01-20 10:41         ` Wenbo Wang

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.