----- On 22 Aug, 2019, at 02:06, Christoph Hellwig [email protected] wrote: > On Fri, Aug 16, 2019 at 11:47:21AM +0200, Marta Rybczynska wrote: >> It is not possible to get 64-bit results from the passthru commands, >> what prevents from getting for the Capabilities (CAP) property value. >> >> As a result, it is not possible to implement IOL's NVMe Conformance >> test 4.3 Case 1 for Fabrics targets [1] (page 123). >> >> This issue has been already discussed [2], but without a solution. >> >> This patch solves the problem by adding new ioctls with a new >> passthru structure, including 64-bit results. The older ioctls stay >> unchanged. > > Ok, with my idea not being suitable I think I'm fine with this approach, a > little nitpick below: > >> +static bool is_admin_cmd(unsigned int cmd) >> +{ >> + if ((cmd == NVME_IOCTL_ADMIN_CMD) || (cmd == NVME_IOCTL_ADMIN64_CMD)) >> + return true; >> + return false; >> +} > > No need for the inner braces. But I'm actually not sure the current > code structure is very suitable for extending it. > >> + >> static int nvme_ioctl(struct block_device *bdev, fmode_t mode, >> unsigned int cmd, unsigned long arg) >> { >> @@ -1418,13 +1473,13 @@ static int nvme_ioctl(struct block_device *bdev, >> fmode_t >> mode, >> * seperately and drop the ns SRCU reference early. This avoids a >> * deadlock when deleting namespaces using the passthrough interface. >> */ >> - if (cmd == NVME_IOCTL_ADMIN_CMD || is_sed_ioctl(cmd)) { >> + if (is_admin_cmd(cmd) || is_sed_ioctl(cmd)) { > > So maybe for this check we should have a is_ctrl_iocl() helper instead > that includes the is_sed_ioctl check. > >> struct nvme_ctrl *ctrl = ns->ctrl; >> >> nvme_get_ctrl(ns->ctrl); >> nvme_put_ns_from_disk(head, srcu_idx); >> >> - if (cmd == NVME_IOCTL_ADMIN_CMD) >> + if (is_admin_cmd(cmd)) >> ret = nvme_user_cmd(ctrl, NULL, argp); >> else >> ret = sed_ioctl(ctrl->opal_dev, cmd, argp); > > And then we can move this whole branch into a helper function, > which then switches on the ioctl cmd, with sed_ioctl as the fallback. Do you mean something like this? +static bool is_ctrl_ioctl(unsigned int cmd) +{ + if (cmd == NVME_IOCTL_ADMIN_CMD || cmd == NVME_IOCTL_ADMIN64_CMD) + return true; + if (is_sed_ioctl(cmd)) + return true; + return false; +} + +static int nvme_handle_ctrl_ioctl(struct nvme_ns *ns, unsigned int cmd, + void __user *argp, + struct nvme_ns_head *head, + int srcu_idx) +{ + struct nvme_ctrl *ctrl = ns->ctrl; + int ret; + + nvme_get_ctrl(ns->ctrl); + nvme_put_ns_from_disk(head, srcu_idx); + + switch (cmd) { + case NVME_IOCTL_ADMIN_CMD: + ret = nvme_user_cmd(ctrl, NULL, argp); + break; + case NVME_IOCTL_ADMIN64_CMD: + ret = nvme_user_cmd64(ctrl, NULL, argp); + break; + default: + ret = sed_ioctl(ctrl->opal_dev, cmd, argp); + break; + } + nvme_put_ctrl(ctrl); + return ret; +} + static int nvme_ioctl(struct block_device *bdev, fmode_t mode, unsigned int cmd, unsigned long arg) { @@ -1418,20 +1501,8 @@ static int nvme_ioctl(struct block_device *bdev, fmode_t mode, * seperately and drop the ns SRCU reference early. This avoids a * deadlock when deleting namespaces using the passthrough interface. */ - if (cmd == NVME_IOCTL_ADMIN_CMD || is_sed_ioctl(cmd)) { - struct nvme_ctrl *ctrl = ns->ctrl; - - nvme_get_ctrl(ns->ctrl); - nvme_put_ns_from_disk(head, srcu_idx); - - if (cmd == NVME_IOCTL_ADMIN_CMD) - ret = nvme_user_cmd(ctrl, NULL, argp); - else - ret = sed_ioctl(ctrl->opal_dev, cmd, argp); - - nvme_put_ctrl(ctrl); - return ret; - } + if (is_ctrl_ioctl(cmd)) + return nvme_handle_ctrl_ioctl(ns, cmd, argp, head, srcu_idx); switch (cmd) { case NVME_IOCTL_ID: Regards, Marta
