From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751595AbdFHHoR (ORCPT ); Thu, 8 Jun 2017 03:44:17 -0400 Received: from verein.lst.de ([213.95.11.211]:57436 "EHLO newverein.lst.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750752AbdFHHoQ (ORCPT ); Thu, 8 Jun 2017 03:44:16 -0400 Date: Thu, 8 Jun 2017 09:44:14 +0200 From: Christoph Hellwig To: Johannes Thumshirn Cc: Christoph Hellwig , Sagi Grimberg , Keith Busch , Hannes Reinecke , Max Gurtovoy , Linux NVMe Mailinglist , Linux Kernel Mailinglist Subject: Re: [PATCH v6 05/10] nvmet: implement namespace identify descriptor list Message-ID: <20170608074414.GD13953@lst.de> References: <20170607094536.32419-1-jthumshirn@suse.de> <20170607094536.32419-6-jthumshirn@suse.de> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20170607094536.32419-6-jthumshirn@suse.de> 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 On Wed, Jun 07, 2017 at 11:45:32AM +0200, Johannes Thumshirn wrote: > A NVMe Identify NS command with a CNS value of '3' is expecting a list > of Namespace Identification Descriptor structures to be returned to > the host for the namespace requested in the namespace identify > command. > > This Namespace Identification Descriptor structure consists of the > type of the namespace identifier, the length of the identifier and the > actual identifier. > > Valid types are NGUID and UUID which we have saved in our nvme_ns > structure if they have been configured via configfs. If no value has > been assigened to one of these we return an "invalid opcode" back to > the host to maintain backward compatibiliy with older implementations > without Namespace Identify Descriptor list support. > > Also as the Namespace Identify Descriptor list is the only mandatory > feature change between 1.2.1 and 1.3 we can bump the advertised > version as well. > > Signed-off-by: Johannes Thumshirn > Reviewed-by: Hannes Reinecke > Reviewed-by: Max Gurtovoy > --- > drivers/nvme/target/admin-cmd.c | 61 +++++++++++++++++++++++++++++++++++++++++ > drivers/nvme/target/core.c | 3 +- > drivers/nvme/target/nvmet.h | 1 + > 3 files changed, 64 insertions(+), 1 deletion(-) > > diff --git a/drivers/nvme/target/admin-cmd.c b/drivers/nvme/target/admin-cmd.c > index 96c144325443..6f9f0881aa2b 100644 > --- a/drivers/nvme/target/admin-cmd.c > +++ b/drivers/nvme/target/admin-cmd.c > @@ -367,6 +367,64 @@ static void nvmet_execute_identify_nslist(struct nvmet_req *req) > nvmet_req_complete(req, status); > } > > +static u16 nvmet_copy_ns_identifier(struct nvmet_req *req, u8 type, u8 len, > + void *id, off_t *off) > +{ > + struct nvme_ns_id_desc desc = { > + .nidt = type, > + .nidl = len, > + }; > + u16 status; > + > + status = nvmet_copy_to_sgl(req, *off, &desc, sizeof(desc)); > + if (status) > + return status; > + *off += sizeof(desc); > + > + status = nvmet_copy_to_sgl(req, *off, id, len); > + if (status) > + return status; > + *off += len; > + > + return 0; > +} > + > +static void nvmet_execute_identify_desclist(struct nvmet_req *req) > +{ > + struct nvmet_ns *ns; > + u16 status = 0; > + off_t off = 0; > + > + ns = nvmet_find_namespace(req->sq->ctrl, req->cmd->identify.nsid); > + if (!ns) { > + status = NVME_SC_INVALID_NS | NVME_SC_DNR; > + goto out; > + } > + > + if (memchr_inv(&ns->uuid, 0, sizeof(ns->uuid))) { > + status = nvmet_copy_ns_identifier(req, NVME_NIDT_UUID, > + NVME_NIDT_UUID_LEN, > + &ns->uuid, &off); > + if (status) > + goto out_put_ns; > + } > + if (memchr_inv(ns->nguid, 0, sizeof(ns->nguid))) { > + status = nvmet_copy_ns_identifier(req, NVME_NIDT_NGUID, > + NVME_NIDT_NGUID_LEN, > + &ns->nguid, &off); > + if (status) > + goto out_put_ns; > + } > + > + if (sg_zeroout_area(req->sg, req->sg_cnt, NVME_IDENTIFY_DATA_SIZE, off) Shouldn;t the third argument be NVME_IDENTIFY_DATA_SIZE - off in theory? It's probably fine as is as the S/G helpers deal with overflows gracefully, but still.. Otherwise looks fine: Reviewed-by: Christoph Hellwig