* [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.