From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932998AbcL0WUN (ORCPT ); Tue, 27 Dec 2016 17:20:13 -0500 Received: from mga14.intel.com ([192.55.52.115]:9302 "EHLO mga14.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753801AbcL0WUE (ORCPT ); Tue, 27 Dec 2016 17:20:04 -0500 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.33,418,1477983600"; d="scan'208";a="207297570" Date: Tue, 27 Dec 2016 15:12:22 -0700 From: Scott Bauer To: Jethro Beekman Cc: linux-nvme@lists.infradead.org, Rafael.Antognolli@intel.com, axboe@fb.com, keith.busch@intel.com, jonathan.derrick@intel.com, viro@zeniv.linux.org.uk, hch@infradead.org, linux-kernel@vger.kernel.org, sagi@grimberg.me Subject: Re: [PATCH v3 4/5] nvme: Implement resume_from_suspend and SED Allocation code. Message-ID: <20161227221221.GA2902@sbauer-Z170X-UD5> References: <1482176149-2257-1-git-send-email-scott.bauer@intel.com> <1482176149-2257-5-git-send-email-scott.bauer@intel.com> <3d8b8516-aac6-c11c-dbbb-47fc01308f8c@jbeekman.nl> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <3d8b8516-aac6-c11c-dbbb-47fc01308f8c@jbeekman.nl> User-Agent: Mutt/1.5.24 (2015-08-30) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Sun, Dec 25, 2016 at 03:15:52PM +0100, Jethro Beekman wrote: > On 19-12-16 20:35, Scott Bauer wrote: > > @@ -1796,6 +1797,13 @@ static void nvme_reset_work(struct work_struct *work) > > if (result) > > goto out; > > > > + result = nvme_opal_initialize(&dev->ctrl); > > + if (result) > > + goto out; > > It seems you always try to intialize OPAL even if the drive doesn't support it. > I think you should check if the device supports security commands and then see > if it supports OPAL before calling this. See e.g. > https://lkml.org/lkml/2016/6/19/139 . Ideally, this code would check all > supported protocols and initialize the appropriate security device based on that. The nvme_opal_initalize should probably be changed to nvme_opal_allocate or something. It's not really initalizing anything other than allocting the necessary structures for OPAL. In order to determine if the controller supports opal we need to allocate the previously mentioned structures anyway. I want to stay away from making payloads (specifically discovery0 ) payload in the nvme driver and allow the opal core to do a all the grunt work. In the future we'll probably have to refactor the core a bit to do just packet generation. It looks like at least for NVMe we're going to have to do a discovery to figure out whether we've got mutiple locking ranges per NS or just one global lr during initialization.