From mboxrd@z Thu Jan 1 00:00:00 1970 Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752593AbeADKUg (ORCPT + 1 other); Thu, 4 Jan 2018 05:20:36 -0500 Received: from verein.lst.de ([213.95.11.211]:48490 "EHLO newverein.lst.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752441AbeADKUf (ORCPT ); Thu, 4 Jan 2018 05:20:35 -0500 Date: Thu, 4 Jan 2018 11:20:33 +0100 From: Christoph Hellwig To: Jianchao Wang Cc: keith.busch@intel.com, axboe@fb.com, hch@lst.de, sagi@grimberg.me, linux-nvme@lists.infradead.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH V2] nvme-pci: fix NULL pointer reference in nvme_alloc_ns Message-ID: <20180104102033.GA4834@lst.de> References: <1515112799-1678-1-git-send-email-jianchao.w.wang@oracle.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1515112799-1678-1-git-send-email-jianchao.w.wang@oracle.com> User-Agent: Mutt/1.5.17 (2007-11-01) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Return-Path: This looks generally fine to me, ut a few nitpicks below: > - Based on Sagi's suggestion, add new state NVME_CTRL_ADMIN_LIVE. Maybe call this NVME_CTRL_ADMIN_ONLY ? > - if (ctrl->state != NVME_CTRL_LIVE) > + if ((ctrl->state != NVME_CTRL_LIVE) && > + (ctrl->state != NVME_CTRL_ADMIN_LIVE)) No need for the inner braces, and odd indentation. Also in general I'm tempted to just use switch statements for things like this, e.g. switch (ctrl->state) { case NVME_CTRL_ADMIN_LIVE: case NVME_CTRL_LIVE: break; default: return -EWOULDBLOCK; } > @@ -3074,6 +3087,8 @@ static void nvme_scan_work(struct work_struct *work) > if (ctrl->state != NVME_CTRL_LIVE) > return; > > + BUG_ON(!ctrl->tagset); WARN_ON_ONCE() please. > + bool only_adminq = false; How about a new_state variable instead that holds the new state value?