* [PATCH-4.6 1/3] NVMe: Use simple ida interface
@ 2016-02-18 23:21 Keith Busch
2016-02-18 23:21 ` [PATCH-4.6 2/3] NVMe: Code naming cleanup Keith Busch
` (3 more replies)
0 siblings, 4 replies; 9+ messages in thread
From: Keith Busch @ 2016-02-18 23:21 UTC (permalink / raw)
This is an easier interface to use and removes one of the overloaded
dev_list_lock uses.
Signed-off-by: Keith Busch <keith.busch at intel.com>
---
Feature was originally posted by Lee Duncan last October, but this missed
being applied.
drivers/nvme/host/core.c | 20 ++++----------------
1 file changed, 4 insertions(+), 16 deletions(-)
diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 9b1253f..04443b2 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -1334,29 +1334,17 @@ static DEFINE_IDA(nvme_instance_ida);
static int nvme_set_instance(struct nvme_ctrl *ctrl)
{
- int instance, error;
-
- do {
- if (!ida_pre_get(&nvme_instance_ida, GFP_KERNEL))
- return -ENODEV;
-
- spin_lock(&dev_list_lock);
- error = ida_get_new(&nvme_instance_ida, &instance);
- spin_unlock(&dev_list_lock);
- } while (error == -EAGAIN);
-
- if (error)
- return -ENODEV;
+ int instance = ida_simple_get(&nvme_instance_ida, 0, 0, GFP_KERNEL);
+ if (instance < 0)
+ return instance;
ctrl->instance = instance;
return 0;
}
static void nvme_release_instance(struct nvme_ctrl *ctrl)
{
- spin_lock(&dev_list_lock);
- ida_remove(&nvme_instance_ida, ctrl->instance);
- spin_unlock(&dev_list_lock);
+ ida_simple_remove(&nvme_instance_ida, ctrl->instance);
}
void nvme_uninit_ctrl(struct nvme_ctrl *ctrl)
--
2.6.2.307.g37023ba
^ permalink raw reply related [flat|nested] 9+ messages in thread* [PATCH-4.6 2/3] NVMe: Code naming cleanup 2016-02-18 23:21 [PATCH-4.6 1/3] NVMe: Use simple ida interface Keith Busch @ 2016-02-18 23:21 ` Keith Busch 2016-02-21 16:18 ` Christoph Hellwig 2016-02-22 8:57 ` Sagi Grimberg 2016-02-18 23:21 ` [PATCH-4.6 3/3] NVMe: Don't allow unsupported flags Keith Busch ` (2 subsequent siblings) 3 siblings, 2 replies; 9+ messages in thread From: Keith Busch @ 2016-02-18 23:21 UTC (permalink / raw) Consistency help readability. This renames all instances of struct nvme_ctrl to "ctrl" that weren't already named that. Signed-off-by: Keith Busch <keith.busch at intel.com> --- drivers/nvme/host/core.c | 24 ++++++++++++------------ 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c index 04443b2..33ad10d 100644 --- a/drivers/nvme/host/core.c +++ b/drivers/nvme/host/core.c @@ -248,7 +248,7 @@ int nvme_submit_user_cmd(struct request_queue *q, struct nvme_command *cmd, result, timeout); } -int nvme_identify_ctrl(struct nvme_ctrl *dev, struct nvme_id_ctrl **id) +int nvme_identify_ctrl(struct nvme_ctrl *ctrl, struct nvme_id_ctrl **id) { struct nvme_command c = { }; int error; @@ -261,24 +261,24 @@ int nvme_identify_ctrl(struct nvme_ctrl *dev, struct nvme_id_ctrl **id) if (!*id) return -ENOMEM; - error = nvme_submit_sync_cmd(dev->admin_q, &c, *id, + error = nvme_submit_sync_cmd(ctrl->admin_q, &c, *id, sizeof(struct nvme_id_ctrl)); if (error) kfree(*id); return error; } -static int nvme_identify_ns_list(struct nvme_ctrl *dev, unsigned nsid, __le32 *ns_list) +static int nvme_identify_ns_list(struct nvme_ctrl *ctrl, unsigned nsid, __le32 *ns_list) { struct nvme_command c = { }; c.identify.opcode = nvme_admin_identify; c.identify.cns = cpu_to_le32(2); c.identify.nsid = cpu_to_le32(nsid); - return nvme_submit_sync_cmd(dev->admin_q, &c, ns_list, 0x1000); + return nvme_submit_sync_cmd(ctrl->admin_q, &c, ns_list, 0x1000); } -int nvme_identify_ns(struct nvme_ctrl *dev, unsigned nsid, +int nvme_identify_ns(struct nvme_ctrl *ctrl, unsigned nsid, struct nvme_id_ns **id) { struct nvme_command c = { }; @@ -292,14 +292,14 @@ int nvme_identify_ns(struct nvme_ctrl *dev, unsigned nsid, if (!*id) return -ENOMEM; - error = nvme_submit_sync_cmd(dev->admin_q, &c, *id, + error = nvme_submit_sync_cmd(ctrl->admin_q, &c, *id, sizeof(struct nvme_id_ns)); if (error) kfree(*id); return error; } -int nvme_get_features(struct nvme_ctrl *dev, unsigned fid, unsigned nsid, +int nvme_get_features(struct nvme_ctrl *ctrl, unsigned fid, unsigned nsid, dma_addr_t dma_addr, u32 *result) { struct nvme_command c; @@ -310,10 +310,10 @@ int nvme_get_features(struct nvme_ctrl *dev, unsigned fid, unsigned nsid, c.features.prp1 = cpu_to_le64(dma_addr); c.features.fid = cpu_to_le32(fid); - return __nvme_submit_sync_cmd(dev->admin_q, &c, NULL, 0, result, 0); + return __nvme_submit_sync_cmd(ctrl->admin_q, &c, NULL, 0, result, 0); } -int nvme_set_features(struct nvme_ctrl *dev, unsigned fid, unsigned dword11, +int nvme_set_features(struct nvme_ctrl *ctrl, unsigned fid, unsigned dword11, dma_addr_t dma_addr, u32 *result) { struct nvme_command c; @@ -324,10 +324,10 @@ int nvme_set_features(struct nvme_ctrl *dev, unsigned fid, unsigned dword11, c.features.fid = cpu_to_le32(fid); c.features.dword11 = cpu_to_le32(dword11); - return __nvme_submit_sync_cmd(dev->admin_q, &c, NULL, 0, result, 0); + return __nvme_submit_sync_cmd(ctrl->admin_q, &c, NULL, 0, result, 0); } -int nvme_get_log_page(struct nvme_ctrl *dev, struct nvme_smart_log **log) +int nvme_get_log_page(struct nvme_ctrl *ctrl, struct nvme_smart_log **log) { struct nvme_command c = { }; int error; @@ -342,7 +342,7 @@ int nvme_get_log_page(struct nvme_ctrl *dev, struct nvme_smart_log **log) if (!*log) return -ENOMEM; - error = nvme_submit_sync_cmd(dev->admin_q, &c, *log, + error = nvme_submit_sync_cmd(ctrl->admin_q, &c, *log, sizeof(struct nvme_smart_log)); if (error) kfree(*log); -- 2.6.2.307.g37023ba ^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH-4.6 2/3] NVMe: Code naming cleanup 2016-02-18 23:21 ` [PATCH-4.6 2/3] NVMe: Code naming cleanup Keith Busch @ 2016-02-21 16:18 ` Christoph Hellwig 2016-02-22 8:57 ` Sagi Grimberg 1 sibling, 0 replies; 9+ messages in thread From: Christoph Hellwig @ 2016-02-21 16:18 UTC (permalink / raw) Looks fine, Reviewed-by: Christoph Hellwig <hch at lst.de> ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH-4.6 2/3] NVMe: Code naming cleanup 2016-02-18 23:21 ` [PATCH-4.6 2/3] NVMe: Code naming cleanup Keith Busch 2016-02-21 16:18 ` Christoph Hellwig @ 2016-02-22 8:57 ` Sagi Grimberg 1 sibling, 0 replies; 9+ messages in thread From: Sagi Grimberg @ 2016-02-22 8:57 UTC (permalink / raw) Looks good, Reviewed-by: Sagi Grimberg <sagig at mellanox.com> ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH-4.6 3/3] NVMe: Don't allow unsupported flags 2016-02-18 23:21 [PATCH-4.6 1/3] NVMe: Use simple ida interface Keith Busch 2016-02-18 23:21 ` [PATCH-4.6 2/3] NVMe: Code naming cleanup Keith Busch @ 2016-02-18 23:21 ` Keith Busch 2016-02-19 19:14 ` Derrick, Jonathan 2016-02-21 16:20 ` Christoph Hellwig 2016-02-21 16:18 ` [PATCH-4.6 1/3] NVMe: Use simple ida interface Christoph Hellwig 2016-02-22 8:56 ` Sagi Grimberg 3 siblings, 2 replies; 9+ messages in thread From: Keith Busch @ 2016-02-18 23:21 UTC (permalink / raw) The command flags can change the meaning of other fields in the command that the driver is not prepared to handle. Specifically, the user could passthrough an SGL flag, causing the controller to misinterpret the PRP list the driver created, potentially corrupting memory or data. Signed-off-by: Keith Busch <keith.busch at intel.com> --- Alternatively I have a different patch that builds SGL's if the flags has it set and the device supports SGL. Any interest? I didn't post it since the fast path only gets PRP-able scatter lists, and the additional logic to handle SGL's complicates handling the NVMe IO descriptor. drivers/nvme/host/core.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c index 33ad10d..d8c3a55 100644 --- a/drivers/nvme/host/core.c +++ b/drivers/nvme/host/core.c @@ -398,7 +398,7 @@ static int nvme_submit_io(struct nvme_ns *ns, struct nvme_user_io __user *uio) memset(&c, 0, sizeof(c)); c.rw.opcode = io.opcode; - c.rw.flags = io.flags; + c.rw.flags = 0; c.rw.nsid = cpu_to_le32(ns->ns_id); c.rw.slba = cpu_to_le64(io.slba); c.rw.length = cpu_to_le16(io.nblocks); @@ -428,7 +428,7 @@ static int nvme_user_cmd(struct nvme_ctrl *ctrl, struct nvme_ns *ns, memset(&c, 0, sizeof(c)); c.common.opcode = cmd.opcode; - c.common.flags = cmd.flags; + c.common.flags = 0; c.common.nsid = cpu_to_le32(cmd.nsid); c.common.cdw2[0] = cpu_to_le32(cmd.cdw2); c.common.cdw2[1] = cpu_to_le32(cmd.cdw3); -- 2.6.2.307.g37023ba ^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH-4.6 3/3] NVMe: Don't allow unsupported flags 2016-02-18 23:21 ` [PATCH-4.6 3/3] NVMe: Don't allow unsupported flags Keith Busch @ 2016-02-19 19:14 ` Derrick, Jonathan 2016-02-21 16:20 ` Christoph Hellwig 1 sibling, 0 replies; 9+ messages in thread From: Derrick, Jonathan @ 2016-02-19 19:14 UTC (permalink / raw) I haven't seen a great argument for SGL other than that the devices support it. If anyone can prove it faster, I'd like to see it. Otherwise the set looks good -----Original Message----- From: Linux-nvme [mailto:linux-nvme-bounces@lists.infradead.org] On Behalf Of Keith Busch Sent: Thursday, February 18, 2016 4:21 PM To: linux-nvme at lists.infradead.org; Jens Axboe <axboe at fb.com>; Christoph Hellwig <hch at infradead.org> Cc: Busch, Keith <keith.busch at intel.com> Subject: [PATCH-4.6 3/3] NVMe: Don't allow unsupported flags The command flags can change the meaning of other fields in the command that the driver is not prepared to handle. Specifically, the user could passthrough an SGL flag, causing the controller to misinterpret the PRP list the driver created, potentially corrupting memory or data. Signed-off-by: Keith Busch <keith.busch at intel.com> --- Alternatively I have a different patch that builds SGL's if the flags has it set and the device supports SGL. Any interest? I didn't post it since the fast path only gets PRP-able scatter lists, and the additional logic to handle SGL's complicates handling the NVMe IO descriptor. drivers/nvme/host/core.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c index 33ad10d..d8c3a55 100644 --- a/drivers/nvme/host/core.c +++ b/drivers/nvme/host/core.c @@ -398,7 +398,7 @@ static int nvme_submit_io(struct nvme_ns *ns, struct nvme_user_io __user *uio) memset(&c, 0, sizeof(c)); c.rw.opcode = io.opcode; - c.rw.flags = io.flags; + c.rw.flags = 0; c.rw.nsid = cpu_to_le32(ns->ns_id); c.rw.slba = cpu_to_le64(io.slba); c.rw.length = cpu_to_le16(io.nblocks); @@ -428,7 +428,7 @@ static int nvme_user_cmd(struct nvme_ctrl *ctrl, struct nvme_ns *ns, memset(&c, 0, sizeof(c)); c.common.opcode = cmd.opcode; - c.common.flags = cmd.flags; + c.common.flags = 0; c.common.nsid = cpu_to_le32(cmd.nsid); c.common.cdw2[0] = cpu_to_le32(cmd.cdw2); c.common.cdw2[1] = cpu_to_le32(cmd.cdw3); -- 2.6.2.307.g37023ba _______________________________________________ Linux-nvme mailing list Linux-nvme at lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-nvme ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH-4.6 3/3] NVMe: Don't allow unsupported flags 2016-02-18 23:21 ` [PATCH-4.6 3/3] NVMe: Don't allow unsupported flags Keith Busch 2016-02-19 19:14 ` Derrick, Jonathan @ 2016-02-21 16:20 ` Christoph Hellwig 1 sibling, 0 replies; 9+ messages in thread From: Christoph Hellwig @ 2016-02-21 16:20 UTC (permalink / raw) On Thu, Feb 18, 2016@04:21:17PM -0700, Keith Busch wrote: > Alternatively I have a different patch that builds SGL's if the flags > has it set and the device supports SGL. Any interest? > > I didn't post it since the fast path only gets PRP-able scatter lists, > and the additional logic to handle SGL's complicates handling the NVMe > IO descriptor. I don't think we should allow applications to control the SGL use. There are a couple reasons for that in upcoming specs that I can't talk about, but I think it should be common sense to not expose these sorts of low level details to userspace. > drivers/nvme/host/core.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c > index 33ad10d..d8c3a55 100644 > --- a/drivers/nvme/host/core.c > +++ b/drivers/nvme/host/core.c > @@ -398,7 +398,7 @@ static int nvme_submit_io(struct nvme_ns *ns, struct nvme_user_io __user *uio) > > memset(&c, 0, sizeof(c)); > c.rw.opcode = io.opcode; > - c.rw.flags = io.flags; > + c.rw.flags = 0; Shouldn't we check that flags is zero in the user structure and reject the command if not? Otherwise we can't safely add back support for flags in the future if useful ones appear that we might want to support. ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH-4.6 1/3] NVMe: Use simple ida interface 2016-02-18 23:21 [PATCH-4.6 1/3] NVMe: Use simple ida interface Keith Busch 2016-02-18 23:21 ` [PATCH-4.6 2/3] NVMe: Code naming cleanup Keith Busch 2016-02-18 23:21 ` [PATCH-4.6 3/3] NVMe: Don't allow unsupported flags Keith Busch @ 2016-02-21 16:18 ` Christoph Hellwig 2016-02-22 8:56 ` Sagi Grimberg 3 siblings, 0 replies; 9+ messages in thread From: Christoph Hellwig @ 2016-02-21 16:18 UTC (permalink / raw) On Thu, Feb 18, 2016@04:21:15PM -0700, Keith Busch wrote: > This is an easier interface to use and removes one of the overloaded > dev_list_lock uses. > > Signed-off-by: Keith Busch <keith.busch at intel.com> > --- > Feature was originally posted by Lee Duncan last October, but this missed > being applied. So is Lee's patch or yours? Either way it looks fine to me: Reviewed-by: Christoph Hellwig <hch at lst.de> ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH-4.6 1/3] NVMe: Use simple ida interface 2016-02-18 23:21 [PATCH-4.6 1/3] NVMe: Use simple ida interface Keith Busch ` (2 preceding siblings ...) 2016-02-21 16:18 ` [PATCH-4.6 1/3] NVMe: Use simple ida interface Christoph Hellwig @ 2016-02-22 8:56 ` Sagi Grimberg 3 siblings, 0 replies; 9+ messages in thread From: Sagi Grimberg @ 2016-02-22 8:56 UTC (permalink / raw) Looks good to me, Reviewed-by: Sagi Grimberg <sagig at mellanox.com> ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2016-02-22 8:57 UTC | newest] Thread overview: 9+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2016-02-18 23:21 [PATCH-4.6 1/3] NVMe: Use simple ida interface Keith Busch 2016-02-18 23:21 ` [PATCH-4.6 2/3] NVMe: Code naming cleanup Keith Busch 2016-02-21 16:18 ` Christoph Hellwig 2016-02-22 8:57 ` Sagi Grimberg 2016-02-18 23:21 ` [PATCH-4.6 3/3] NVMe: Don't allow unsupported flags Keith Busch 2016-02-19 19:14 ` Derrick, Jonathan 2016-02-21 16:20 ` Christoph Hellwig 2016-02-21 16:18 ` [PATCH-4.6 1/3] NVMe: Use simple ida interface Christoph Hellwig 2016-02-22 8:56 ` Sagi Grimberg
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).