| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <BYAPR04MB57492DE452441E784E88DBAA866B0@BYAPR04MB5749.namprd04.prod.outlook.com>
Date: Wed, 23 Oct 2019 20:31:18 +0000
From: Chaitanya Kulkarni <Chaitanya.Kulkarni@....com>
To: Logan Gunthorpe <logang@...tatee.com>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"linux-nvme@...ts.infradead.org" <linux-nvme@...ts.infradead.org>
CC: Christoph Hellwig <hch@....de>, Sagi Grimberg <sagi@...mberg.me>,
Max Gurtovoy <maxg@...lanox.com>,
Stephen Bates <sbates@...thlin.com>
Subject: Re: [PATCH 3/7] nvmet: Introduce common execute function for
get_log_page and identify
Thanks for this patch.
Looks good.
Reviewed-by: Chaitanya Kulkarni <chaitanya.kulkarni@....com>
On 10/23/2019 09:36 AM, Logan Gunthorpe wrote:
> Instead of picking the sub-command handler to execute in a nested
> switch statement introduce a landing functions that calls out
> to the appropriate sub-command handler.
>
> This will allow us to have a common place in the handler to check
> the transfer length in a future patch.
>
> Signed-off-by: Christoph Hellwig <hch@....de>
> [logang@...tatee.com: separated out of a larger draft patch from hch]
> Signed-off-by: Logan Gunthorpe <logang@...tatee.com>
> ---
> drivers/nvme/target/admin-cmd.c | 93 ++++++++++++++++++---------------
> 1 file changed, 50 insertions(+), 43 deletions(-)
>
> diff --git a/drivers/nvme/target/admin-cmd.c b/drivers/nvme/target/admin-cmd.c
> index 831a062d27cb..3665b45d6515 100644
> --- a/drivers/nvme/target/admin-cmd.c
> +++ b/drivers/nvme/target/admin-cmd.c
> @@ -282,6 +282,33 @@ static void nvmet_execute_get_log_page_ana(struct nvmet_req *req)
> nvmet_req_complete(req, status);
> }
>
> +static void nvmet_execute_get_log_page(struct nvmet_req *req)
> +{
> + switch (req->cmd->get_log_page.lid) {
> + case NVME_LOG_ERROR:
> + return nvmet_execute_get_log_page_error(req);
> + case NVME_LOG_SMART:
> + return nvmet_execute_get_log_page_smart(req);
> + case NVME_LOG_FW_SLOT:
> + /*
> + * We only support a single firmware slot which always is
> + * active, so we can zero out the whole firmware slot log and
> + * still claim to fully implement this mandatory log page.
> + */
> + return nvmet_execute_get_log_page_noop(req);
> + case NVME_LOG_CHANGED_NS:
> + return nvmet_execute_get_log_changed_ns(req);
> + case NVME_LOG_CMD_EFFECTS:
> + return nvmet_execute_get_log_cmd_effects_ns(req);
> + case NVME_LOG_ANA:
> + return nvmet_execute_get_log_page_ana(req);
> + }
> + pr_err("unhandled lid %d on qid %d\n",
> + req->cmd->get_log_page.lid, req->sq->qid);
> + req->error_loc = offsetof(struct nvme_get_log_page_command, lid);
> + nvmet_req_complete(req, NVME_SC_INVALID_FIELD | NVME_SC_DNR);
> +}
> +
> static void nvmet_execute_identify_ctrl(struct nvmet_req *req)
> {
> struct nvmet_ctrl *ctrl = req->sq->ctrl;
> @@ -565,6 +592,25 @@ static void nvmet_execute_identify_desclist(struct nvmet_req *req)
> nvmet_req_complete(req, status);
> }
>
> +static void nvmet_execute_identify(struct nvmet_req *req)
> +{
> + switch (req->cmd->identify.cns) {
> + case NVME_ID_CNS_NS:
> + return nvmet_execute_identify_ns(req);
> + case NVME_ID_CNS_CTRL:
> + return nvmet_execute_identify_ctrl(req);
> + case NVME_ID_CNS_NS_ACTIVE_LIST:
> + return nvmet_execute_identify_nslist(req);
> + case NVME_ID_CNS_NS_DESC_LIST:
> + return nvmet_execute_identify_desclist(req);
> + }
> +
> + pr_err("unhandled identify cns %d on qid %d\n",
> + req->cmd->identify.cns, req->sq->qid);
> + req->error_loc = offsetof(struct nvme_identify, cns);
> + nvmet_req_complete(req, NVME_SC_INVALID_FIELD | NVME_SC_DNR);
> +}
> +
> /*
> * A "minimum viable" abort implementation: the command is mandatory in the
> * spec, but we are not required to do any useful work. We couldn't really
> @@ -819,52 +865,13 @@ u16 nvmet_parse_admin_cmd(struct nvmet_req *req)
>
> switch (cmd->common.opcode) {
> case nvme_admin_get_log_page:
> + req->execute = nvmet_execute_get_log_page;
> req->data_len = nvmet_get_log_page_len(cmd);
> -
> - switch (cmd->get_log_page.lid) {
> - case NVME_LOG_ERROR:
> - req->execute = nvmet_execute_get_log_page_error;
> - return 0;
> - case NVME_LOG_SMART:
> - req->execute = nvmet_execute_get_log_page_smart;
> - return 0;
> - case NVME_LOG_FW_SLOT:
> - /*
> - * We only support a single firmware slot which always
> - * is active, so we can zero out the whole firmware slot
> - * log and still claim to fully implement this mandatory
> - * log page.
> - */
> - req->execute = nvmet_execute_get_log_page_noop;
> - return 0;
> - case NVME_LOG_CHANGED_NS:
> - req->execute = nvmet_execute_get_log_changed_ns;
> - return 0;
> - case NVME_LOG_CMD_EFFECTS:
> - req->execute = nvmet_execute_get_log_cmd_effects_ns;
> - return 0;
> - case NVME_LOG_ANA:
> - req->execute = nvmet_execute_get_log_page_ana;
> - return 0;
> - }
> - break;
> + return 0;
> case nvme_admin_identify:
> + req->execute = nvmet_execute_identify;
> req->data_len = NVME_IDENTIFY_DATA_SIZE;
> - switch (cmd->identify.cns) {
> - case NVME_ID_CNS_NS:
> - req->execute = nvmet_execute_identify_ns;
> - return 0;
> - case NVME_ID_CNS_CTRL:
> - req->execute = nvmet_execute_identify_ctrl;
> - return 0;
> - case NVME_ID_CNS_NS_ACTIVE_LIST:
> - req->execute = nvmet_execute_identify_nslist;
> - return 0;
> - case NVME_ID_CNS_NS_DESC_LIST:
> - req->execute = nvmet_execute_identify_desclist;
> - return 0;
> - }
> - break;
> + return 0;
> case nvme_admin_abort_cmd:
> req->execute = nvmet_execute_abort;
> req->data_len = 0;
>
Powered by blists - more mailing lists