NVMe: Use simple ida interface
PATCH 1/3 NVMe: Use simple ida interface
This is an easier interface to use and removes one of the overloaded dev_list_lock uses.
---
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(-)
drivers/nvme/host/core.c +4 -16
@@ -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)
Signature · 1 line
2.6.2.307.g37023ba
PATCH 2/3 NVMe: Code naming cleanup
Consistency help readability. This renames all instances of struct nvme_ctrl to "ctrl" that weren't already named that.
--- drivers/nvme/host/core.c | 24 ++++++++++++------------ 1 file changed, 12 insertions(+), 12 deletions(-)
drivers/nvme/host/core.c +12 -12
@@ -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);
Signature · 1 line
2.6.2.307.g37023ba
PATCH 2/3 NVMe: Code naming cleanup
Looks fine,
PATCH 2/3 NVMe: Code naming cleanup
In reply to Keith BuschLooks good,
PATCH 3/3 NVMe: Don't allow unsupported flags
In reply to Keith BuschThe 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.
---
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(-)
drivers/nvme/host/core.c +2 -2
@@ -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);
Signature · 1 line
2.6.2.307.g37023ba
PATCH 3/3 NVMe: Don't allow unsupported flags
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:[email protected]] 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.
---
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(-)
drivers/nvme/host/core.c +1 -1
@@ -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
PATCH 3/3 NVMe: Don't allow unsupported flags
In reply to Keith BuschQuoted 6 lines · Keith Busch
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 static int nvme_submit_io(struct nvme_ns *ns, struct nvme_user_io __user *uio) · line 401
Quoted 13 lines
drivers/nvme/host/core.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)drivers/nvme/host/core.c +1
-1@@ -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.
PATCH 1/3 NVMe: Use simple ida interface
In reply to Keith BuschQuoted 7 lines · Keith Busch
This is an easier interface to use and removes one of the overloaded
dev_list_lockuses.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:
PATCH 1/3 NVMe: Use simple ida interface
In reply to Keith BuschLooks good to me,