From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S934212AbcATJH6 (ORCPT ); Wed, 20 Jan 2016 04:07:58 -0500 Received: from mail-wm0-f50.google.com ([74.125.82.50]:34770 "EHLO mail-wm0-f50.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S933979AbcATJGI (ORCPT ); Wed, 20 Jan 2016 04:06:08 -0500 Subject: Re: [PATCH] NVMe: init nvme queue before enabling irq To: Wenbo Wang , keith.busch@intel.com, axboe@fb.com References: <1453265860-31080-1-git-send-email-mail_weber_wang@163.com> Cc: Wenbo Wang , wenwei.tao@memblaze.com, linux-kernel@vger.kernel.org, linux-nvme@lists.infradead.org From: Sagi Grimberg Message-ID: <569F4DFA.2090504@dev.mellanox.co.il> Date: Wed, 20 Jan 2016 11:06:02 +0200 User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:38.0) Gecko/20100101 Thunderbird/38.5.1 MIME-Version: 1.0 In-Reply-To: <1453265860-31080-1-git-send-email-mail_weber_wang@163.com> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org > 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 > Reviewed-by: Wenwei Tao 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 From mboxrd@z Thu Jan 1 00:00:00 1970 From: sagig@dev.mellanox.co.il (Sagi Grimberg) Date: Wed, 20 Jan 2016 11:06:02 +0200 Subject: [PATCH] NVMe: init nvme queue before enabling irq In-Reply-To: <1453265860-31080-1-git-send-email-mail_weber_wang@163.com> References: <1453265860-31080-1-git-send-email-mail_weber_wang@163.com> Message-ID: <569F4DFA.2090504@dev.mellanox.co.il> > 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 > Reviewed-by: Wenwei Tao 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