Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
IronPort-SDR: Z4Aq/DODSAANbNrx0m80egQCjvWPLa+CFmZtc1FrShdZzgORDtIS27CAzeZnMnd/566KqO49J/
 k30U3heRjcA91DItXlMfiFQbmKnIVhgTZwhvEUKuP4pffD/yktNc+VnN5VmXpYjgmV0IPNxf6n
 jmEcp5i+xF7bD7JuAQa2+pYGXz6GKVg7/wqTOem3GyXhh0W69qqK8XmDXKSem2o3e0TQXlUdmF
 mBM+BCn4Ri5MNXO/HFF6D2UgnvB6Ugx+6+5nWMk3AB8+TtUaoDlFr8liPXuq5vrPcVV+/CLT7I
 ZHc=
From:   Chaitanya Kulkarni <Chaitanya.Kulkarni@wdc.com>
To:     Logan Gunthorpe <logang@deltatee.com>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "linux-nvme@lists.infradead.org" <linux-nvme@lists.infradead.org>
CC:     Christoph Hellwig <hch@lst.de>, Sagi Grimberg <sagi@grimberg.me>,
        Max Gurtovoy <maxg@mellanox.com>,
        Stephen Bates <sbates@raithlin.com>
Subject: Re: [PATCH 3/7] nvmet: Introduce common execute function for
 get_log_page and identify
Thread-Topic: [PATCH 3/7] nvmet: Introduce common execute function for
 get_log_page and identify
Thread-Index: AQHVib/9UOiReJn9AkSMsa7C5dVEEw==
Date:   Wed, 23 Oct 2019 20:31:18 +0000
Message-ID: <BYAPR04MB57492DE452441E784E88DBAA866B0@BYAPR04MB5749.namprd04.prod.outlook.com>
References: <20191023163545.4193-1-logang@deltatee.com>
 <20191023163545.4193-4-logang@deltatee.com>
Accept-Language: en-US
Content-Language: en-US
wdcipoutbound: EOP-TRUE
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-Network-Message-Id: 740b0541-c65a-465c-4e3d-08d757f7f5d1
X-MS-Exchange-CrossTenant-originalarrivaltime: 23 Oct 2019 20:31:18.7818
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: b61c8803-16f3-4c35-9b17-6f65f441df86
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: xqQEWABrYBYXGrdLJVmL8tZjAhp8T5NngZfHYrWwjDFYWuavR3JpixUt0HWFf7byi8jkMc513/yGaNJozfBxcO+RKMzDday8fBQBkSeCAqU=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BYAPR04MB4742
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

Thanks for this patch.=0A=
=0A=
Looks good.=0A=
=0A=
Reviewed-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>=0A=
=0A=
On 10/23/2019 09:36 AM, Logan Gunthorpe wrote:=0A=
> Instead of picking the sub-command handler to execute in a nested=0A=
> switch statement introduce a landing functions that calls out=0A=
> to the appropriate sub-command handler.=0A=
>=0A=
> This will allow us to have a common place in the handler to check=0A=
> the transfer length in a future patch.=0A=
>=0A=
> Signed-off-by: Christoph Hellwig <hch@lst.de>=0A=
> [logang@deltatee.com: separated out of a larger draft patch from hch]=0A=
> Signed-off-by: Logan Gunthorpe <logang@deltatee.com>=0A=
> ---=0A=
>   drivers/nvme/target/admin-cmd.c | 93 ++++++++++++++++++---------------=
=0A=
>   1 file changed, 50 insertions(+), 43 deletions(-)=0A=
>=0A=
> diff --git a/drivers/nvme/target/admin-cmd.c b/drivers/nvme/target/admin-=
cmd.c=0A=
> index 831a062d27cb..3665b45d6515 100644=0A=
> --- a/drivers/nvme/target/admin-cmd.c=0A=
> +++ b/drivers/nvme/target/admin-cmd.c=0A=
> @@ -282,6 +282,33 @@ static void nvmet_execute_get_log_page_ana(struct nv=
met_req *req)=0A=
>   	nvmet_req_complete(req, status);=0A=
>   }=0A=
>=0A=
> +static void nvmet_execute_get_log_page(struct nvmet_req *req)=0A=
> +{=0A=
> +	switch (req->cmd->get_log_page.lid) {=0A=
> +	case NVME_LOG_ERROR:=0A=
> +		return nvmet_execute_get_log_page_error(req);=0A=
> +	case NVME_LOG_SMART:=0A=
> +		return nvmet_execute_get_log_page_smart(req);=0A=
> +	case NVME_LOG_FW_SLOT:=0A=
> +		/*=0A=
> +		 * We only support a single firmware slot which always is=0A=
> +		 * active, so we can zero out the whole firmware slot log and=0A=
> +		 * still claim to fully implement this mandatory log page.=0A=
> +		 */=0A=
> +		return nvmet_execute_get_log_page_noop(req);=0A=
> +	case NVME_LOG_CHANGED_NS:=0A=
> +		return nvmet_execute_get_log_changed_ns(req);=0A=
> +	case NVME_LOG_CMD_EFFECTS:=0A=
> +		return nvmet_execute_get_log_cmd_effects_ns(req);=0A=
> +	case NVME_LOG_ANA:=0A=
> +		return nvmet_execute_get_log_page_ana(req);=0A=
> +	}=0A=
> +	pr_err("unhandled lid %d on qid %d\n",=0A=
> +	       req->cmd->get_log_page.lid, req->sq->qid);=0A=
> +	req->error_loc =3D offsetof(struct nvme_get_log_page_command, lid);=0A=
> +	nvmet_req_complete(req, NVME_SC_INVALID_FIELD | NVME_SC_DNR);=0A=
> +}=0A=
> +=0A=
>   static void nvmet_execute_identify_ctrl(struct nvmet_req *req)=0A=
>   {=0A=
>   	struct nvmet_ctrl *ctrl =3D req->sq->ctrl;=0A=
> @@ -565,6 +592,25 @@ static void nvmet_execute_identify_desclist(struct n=
vmet_req *req)=0A=
>   	nvmet_req_complete(req, status);=0A=
>   }=0A=
>=0A=
> +static void nvmet_execute_identify(struct nvmet_req *req)=0A=
> +{=0A=
> +	switch (req->cmd->identify.cns) {=0A=
> +	case NVME_ID_CNS_NS:=0A=
> +		return nvmet_execute_identify_ns(req);=0A=
> +	case NVME_ID_CNS_CTRL:=0A=
> +		return nvmet_execute_identify_ctrl(req);=0A=
> +	case NVME_ID_CNS_NS_ACTIVE_LIST:=0A=
> +		return nvmet_execute_identify_nslist(req);=0A=
> +	case NVME_ID_CNS_NS_DESC_LIST:=0A=
> +		return nvmet_execute_identify_desclist(req);=0A=
> +	}=0A=
> +=0A=
> +	pr_err("unhandled identify cns %d on qid %d\n",=0A=
> +	       req->cmd->identify.cns, req->sq->qid);=0A=
> +	req->error_loc =3D offsetof(struct nvme_identify, cns);=0A=
> +	nvmet_req_complete(req, NVME_SC_INVALID_FIELD | NVME_SC_DNR);=0A=
> +}=0A=
> +=0A=
>   /*=0A=
>    * A "minimum viable" abort implementation: the command is mandatory in=
 the=0A=
>    * spec, but we are not required to do any useful work.  We couldn't re=
ally=0A=
> @@ -819,52 +865,13 @@ u16 nvmet_parse_admin_cmd(struct nvmet_req *req)=0A=
>=0A=
>   	switch (cmd->common.opcode) {=0A=
>   	case nvme_admin_get_log_page:=0A=
> +		req->execute =3D nvmet_execute_get_log_page;=0A=
>   		req->data_len =3D nvmet_get_log_page_len(cmd);=0A=
> -=0A=
> -		switch (cmd->get_log_page.lid) {=0A=
> -		case NVME_LOG_ERROR:=0A=
> -			req->execute =3D nvmet_execute_get_log_page_error;=0A=
> -			return 0;=0A=
> -		case NVME_LOG_SMART:=0A=
> -			req->execute =3D nvmet_execute_get_log_page_smart;=0A=
> -			return 0;=0A=
> -		case NVME_LOG_FW_SLOT:=0A=
> -			/*=0A=
> -			 * We only support a single firmware slot which always=0A=
> -			 * is active, so we can zero out the whole firmware slot=0A=
> -			 * log and still claim to fully implement this mandatory=0A=
> -			 * log page.=0A=
> -			 */=0A=
> -			req->execute =3D nvmet_execute_get_log_page_noop;=0A=
> -			return 0;=0A=
> -		case NVME_LOG_CHANGED_NS:=0A=
> -			req->execute =3D nvmet_execute_get_log_changed_ns;=0A=
> -			return 0;=0A=
> -		case NVME_LOG_CMD_EFFECTS:=0A=
> -			req->execute =3D nvmet_execute_get_log_cmd_effects_ns;=0A=
> -			return 0;=0A=
> -		case NVME_LOG_ANA:=0A=
> -			req->execute =3D nvmet_execute_get_log_page_ana;=0A=
> -			return 0;=0A=
> -		}=0A=
> -		break;=0A=
> +		return 0;=0A=
>   	case nvme_admin_identify:=0A=
> +		req->execute =3D nvmet_execute_identify;=0A=
>   		req->data_len =3D NVME_IDENTIFY_DATA_SIZE;=0A=
> -		switch (cmd->identify.cns) {=0A=
> -		case NVME_ID_CNS_NS:=0A=
> -			req->execute =3D nvmet_execute_identify_ns;=0A=
> -			return 0;=0A=
> -		case NVME_ID_CNS_CTRL:=0A=
> -			req->execute =3D nvmet_execute_identify_ctrl;=0A=
> -			return 0;=0A=
> -		case NVME_ID_CNS_NS_ACTIVE_LIST:=0A=
> -			req->execute =3D nvmet_execute_identify_nslist;=0A=
> -			return 0;=0A=
> -		case NVME_ID_CNS_NS_DESC_LIST:=0A=
> -			req->execute =3D nvmet_execute_identify_desclist;=0A=
> -			return 0;=0A=
> -		}=0A=
> -		break;=0A=
> +		return 0;=0A=
>   	case nvme_admin_abort_cmd:=0A=
>   		req->execute =3D nvmet_execute_abort;=0A=
>   		req->data_len =3D 0;=0A=
>=0A=
=0A=