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 <[email protected]>
[[email protected]: separated out of a larger draft patch from hch]
Signed-off-by: Logan Gunthorpe <[email protected]>
---
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;
--
2.20.1
Thanks for this patch.
Looks good.
Reviewed-by: Chaitanya Kulkarni <[email protected]>
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 <[email protected]>
> [[email protected]: separated out of a larger draft patch from hch]
> Signed-off-by: Logan Gunthorpe <[email protected]>
> ---
> 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;
>
On Wed, Oct 23, 2019 at 10:35:41AM -0600, 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 <[email protected]>
> [[email protected]: separated out of a larger draft patch from hch]
> Signed-off-by: Logan Gunthorpe <[email protected]>
First signoff needs to be the From line picked up by git. I don't really
care if you keep my attribution or not, but if you do it needs From
line for me as the first theing in the mail body, and if not it
should drop by signoff and just say based on a patch from me.
Otherwise the series looks fine.
On 2019-10-23 7:17 p.m., Christoph Hellwig wrote:
> On Wed, Oct 23, 2019 at 10:35:41AM -0600, 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 <[email protected]>
>> [[email protected]: separated out of a larger draft patch from hch]
>> Signed-off-by: Logan Gunthorpe <[email protected]>
>
> First signoff needs to be the From line picked up by git. I don't really
> care if you keep my attribution or not, but if you do it needs From
> line for me as the first theing in the mail body, and if not it
> should drop by signoff and just say based on a patch from me.
>
> Otherwise the series looks fine.
Ok, understood. Do you want me to fix this up in a v2? Or can you fix it
up when you pick up the patches?
Thanks,
Logan
On Thu, Oct 24, 2019 at 11:18:20AM -0600, Logan Gunthorpe wrote:
> On 2019-10-23 7:17 p.m., Christoph Hellwig wrote:
> >
> > First signoff needs to be the From line picked up by git. I don't really
> > care if you keep my attribution or not, but if you do it needs From
> > line for me as the first theing in the mail body, and if not it
> > should drop by signoff and just say based on a patch from me.
> >
> > Otherwise the series looks fine.
>
> Ok, understood. Do you want me to fix this up in a v2? Or can you fix it
> up when you pick up the patches?
I'll adjust with the commit. Just let me know which way you'd like to go,
whether attribute author to Christoph or use the "Based-on-a-patch-by:"
option.
On 2019-10-24 4:01 p.m., Keith Busch wrote:
> On Thu, Oct 24, 2019 at 11:18:20AM -0600, Logan Gunthorpe wrote:
>> On 2019-10-23 7:17 p.m., Christoph Hellwig wrote:
>>>
>>> First signoff needs to be the From line picked up by git. I don't really
>>> care if you keep my attribution or not, but if you do it needs From
>>> line for me as the first theing in the mail body, and if not it
>>> should drop by signoff and just say based on a patch from me.
>>>
>>> Otherwise the series looks fine.
>>
>> Ok, understood. Do you want me to fix this up in a v2? Or can you fix it
>> up when you pick up the patches?
>
> I'll adjust with the commit. Just let me know which way you'd like to go,
> whether attribute author to Christoph or use the "Based-on-a-patch-by:"
> option.
Attribute the author to Christoph. It was all pretty much verbatim from
the draft patch he sent anyway. I just split it up and tested it.
Thanks,
Logan