nvme-fabrics: Add FC LLDD loopback driver to test FC-NVME
Subsystems (3)
PATCH v3 7/7 nvme-fabrics: Add FC LLDD loopback driver to test FC-NVME
Add FC LLDD loopback driver to test FC host and target transport within nvme-fabrics
To aid in the development and testing of the lower-level api of the FC transport, this loopback driver has been created to act as if it were a FC hba driver supporting both the host interfaces as well as the target interfaces with the nvme FC transport.
--- MAINTAINERS | 1 + drivers/nvme/target/Kconfig | 13 + drivers/nvme/target/Makefile | 2 + drivers/nvme/target/fcloop.c | 1145 ++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 1161 insertions(+) create mode 100644 drivers/nvme/target/fcloop.c
MAINTAINERS +1 -0
@@ -8667,6 +8667,7 @@ F: include/linux/nvme-fc.h F: include/linux/nvme-fc-driver.h F: drivers/nvme/host/fc.c F: drivers/nvme/target/fc.c +F: drivers/nvme/target/fcloop.c NVMEM FRAMEWORK M: Srinivas Kandagatla <srinivas.kandagatla at linaro.org>
drivers/nvme/target/Kconfig +13 -0
@@ -44,3 +44,16 @@ config NVME_TARGET_FC If unsure, say N. +config NVME_TARGET_FCLOOP + tristate "NVMe over Fabrics FC Transport Loopback Test driver" + depends on NVME_TARGET + select NVME_CORE + select NVME_FABRICS + select SG_POOL + select NVME_FC + select NVME_TARGET_FC + help + This enables the NVMe FC loopback test support, which can be useful + to test NVMe-FC transport interfaces. + + If unsure, say N.
drivers/nvme/target/Makefile +2 -0
@@ -3,9 +3,11 @@ obj-$(CONFIG_NVME_TARGET) += nvmet.o obj-$(CONFIG_NVME_TARGET_LOOP) += nvme-loop.o obj-$(CONFIG_NVME_TARGET_RDMA) += nvmet-rdma.o obj-$(CONFIG_NVME_TARGET_FC) += nvmet-fc.o +obj-$(CONFIG_NVME_TARGET_FCLOOP) += nvme-fcloop.o nvmet-y += core.o configfs.o admin-cmd.o io-cmd.o fabrics-cmd.o \ discovery.o nvme-loop-y += loop.o nvmet-rdma-y += rdma.o nvmet-fc-y += fc.o +nvme-fcloop-y += fcloop.o
drivers/nvme/target/fcloop.c new file +1145 -0
1,145 lines not on this page · raw message
Signature · 1 line
2.5.0
PATCH v3 7/7 nvme-fabrics: Add FC LLDD loopback driver to test FC-NVME
Quoted 17 lines
drivers/nvme/target/fcloop.c new file +15
-0+static struct fcloop_nport * +fcloop_alloc_nport (const char *buf,size_t count,bool remoteport) +{ +struct fcloop_nport *newnport, *nport =NULL ; +struct fcloop_lport *tmplport, *lport =NULL ; +struct fcloop_ctrl_options *opts; +unsigned long flags; +u32 opts_mask = (remoteport) ?RPORT_OPTS :TGTPORT_OPTS ; +int ret; + + opts =kzalloc (sizeof (*opts),GFP_KERNEL ); +if (!opts) +return NULL ; + + ret =fcloop_parse_options (opts, buf);
drivers/nvme/target/fcloop.c line 738
Quoted 10 lines
drivers/nvme/target/fcloop.c new file +10
-0+if (ret) + goto out_free_opts; + +/* everything there ? */ +if ((opts->mask & opts_mask) != opts_mask) { + ret = -EINVAL ; + goto out_free_opts; + } + + newnport =kzalloc (sizeof (*nport),GFP_KERNEL );
Nitpick- looks odd to use a pointer variable instead of *newnport for the sizeof parameter, especially when that is the way '*opts' is done a few lines above it.
Quoted 43 lines
drivers/nvme/target/fcloop.c new file +16
-0+if (!newnport) + goto out_free_opts; + +INIT_LIST_HEAD (&newnport->nport_list); + newnport->node_name = opts->wwnn; + newnport->port_name = opts->wwpn; +if (opts->mask &NVMF_OPT_ROLES ) + newnport->port_role = opts->roles; +if (opts->mask &NVMF_OPT_FCADDR ) + newnport->port_id = opts->fcaddr; +kref_init (&newnport->ref); + +spin_lock_irqsave (&fcloop_lock, flags); + +list_for_each_entry (tmplport, &fcloop_lports, lport_list) { + if ((tmplport->localport->node_name == opts->wwnn)&&
drivers/nvme/target/fcloop.c new file +4
-0+ ????(tmplport->localport->port_name == opts->wwpn)) + goto out_invalid_opts; + + if ((tmplport->localport->node_name == opts->lpwwnn)&&
(quoted diff) +1
-0+ ????(tmplport->localport->port_name == opts-Quoted 1 line
lpwwpn))
drivers/nvme/target/fcloop.c new file +19
-0+ lport = tmplport; + } + +if (remoteport) { +if (!lport) + goto out_invalid_opts; + newnport->lport = lport; + } + +list_for_each_entry (nport, &fcloop_nports, nport_list) { +if (nport->node_name == opts->wwnn && + ????nport->port_name == opts->wwpn) { + if ((remoteport && nport->rport) || + ????(!remoteport && nport->tport)) { + nport =NULL ; + goto out_invalid_opts; + } + +fcloop_nport_get (nport);
drivers/nvme/target/fcloop.c line 788
Quoted 10 lines
drivers/nvme/target/fcloop.c new file +10
-0+ +spin_unlock_irqrestore (&fcloop_lock, flags); + +if (remoteport) + nport->lport = lport; +if (opts->mask &NVMF_OPT_ROLES ) + nport->port_role = opts->roles; +if (opts->mask &NVMF_OPT_FCADDR ) + nport->port_id = opts->fcaddr; + goto out_free_opts;
It looks like if we ever get here, we will leak memory as newnport has been kzalloc'ed at this point but this will jump to the "out_free_opts" goto statement.
Quoted 42 lines
drivers/nvme/target/fcloop.c new file +20
-0+ } + } + +list_add_tail (&newnport->nport_list, &fcloop_nports); + +spin_unlock_irqrestore (&fcloop_lock, flags); + +kfree (opts); +return newnport; + +out_invalid_opts: +spin_unlock_irqrestore (&fcloop_lock, flags); +kfree (newnport); +out_free_opts: +kfree (opts); +return nport; +} + +static ssize_t +fcloop_create_remote_port(struct device *dev, struct
device_attribute*attr,drivers/nvme/target/fcloop.c new file +19
-0+ const char *buf, size_t count) +{ +struct nvme_fc_remote_port *remoteport; +struct fcloop_nport *nport; +struct fcloop_rport *rport; +struct nvme_fc_port_info pinfo; +int ret; + + nport =fcloop_alloc_nport (buf, count, true); +if (!nport) +return -EIO ; + + pinfo.node_name = nport->node_name; + pinfo.port_name = nport->port_name; + pinfo.port_role = nport->port_role; + pinfo.port_id = nport->port_id; + + ret = nvme_fc_register_remoteport(nport->lport->localport, + &pinfo,&remoteport);
drivers/nvme/target/fcloop.c line 830
Quoted 3 lines
drivers/nvme/target/fcloop.c new file +3
-0+if (ret || !remoteport) { +fcloop_nport_put (nport); +return ret;
Possible memory leak for nport??
Quoted 19 lines
drivers/nvme/target/fcloop.c new file +6
-0+ } + +/* success */ + rport = remoteport->private; + rport->remoteport = remoteport; + rport->targetport = (nport->tport) ???nport->tport-Quoted 1 line
targetport : NULL;
drivers/nvme/target/fcloop.c new file +12
-0+if (nport->tport) { + nport->tport->remoteport = remoteport; + nport->tport->lport = nport->lport; + } + rport->nport = nport; + rport->lport = nport->lport; + nport->rport = rport; + +return ret ? ret : count; +} + +
snip...
Quoted 22 lines
drivers/nvme/target/fcloop.c new file +3
-0+ +static ssize_t +fcloop_create_target_port(struct device *dev, struct
device_attribute*attr,drivers/nvme/target/fcloop.c new file +17
-0+ const char *buf, size_t count) +{ +struct nvmet_fc_target_port *targetport; +struct fcloop_nport *nport; +struct fcloop_tport *tport; +struct nvmet_fc_port_info tinfo; +int ret; + + nport =fcloop_alloc_nport (buf, count, false); +if (!nport) +return -EIO ; + + tinfo.node_name = nport->node_name; + tinfo.port_name = nport->port_name; + tinfo.port_id = nport->port_id; + + ret = nvmet_fc_register_targetport(&tinfo, &tgttemplate,NULL,
drivers/nvme/target/fcloop.c line 939
Quoted 4 lines
drivers/nvme/target/fcloop.c new file +4
-0+ &targetport); +if (ret) { +fcloop_nport_put (nport); +return ret;
Possible memory leak by not de-allocating 'nport'?
Quoted 16 lines
drivers/nvme/target/fcloop.c new file +6
-0+ } + +/* success */ + tport = targetport->private; + tport->targetport = targetport; + tport->remoteport = (nport->rport) ???nport->rport-Quoted 1 line
remoteport : NULL;
drivers/nvme/target/fcloop.c new file +9
-0+if (nport->rport) + nport->rport->targetport = targetport; + tport->nport = nport; + tport->lport = nport->lport; + nport->tport = tport; + +return ret ? ret : count; +} +
Quoted 1 line
?
Thanks,
J
PATCH v3 7/7 nvme-fabrics: Add FC LLDD loopback driver to test FC-NVME
Quoted 4 lines · J Freyensee
Nitpick- looks odd to use a pointer variable instead of *newnport for the sizeof parameter, especially when that is the way '*opts' is done a few lines above it.
yep - resolved.
Quoted 5 lines
...
Review
Quoted 1 line
(quoted diff) +1
-0+ goto out_free_opts;It looks like if we ever get here, we will leak memory as newnport has been kzalloc'ed at this point but this will jump to the "
out_free_opts" goto statement.
yes - fixed.
Quoted 20 lines
Quoted 20 lines
(quoted diff) +2
-0+static ssize_t +fcloop_create_remote_port(struct device *dev, struct
device_attribute*attr,drivers/nvme/target/fcloop.c new file +16
-0+ const char *buf, size_t count) +{ +struct nvme_fc_remote_port *remoteport; +struct fcloop_nport *nport; +struct fcloop_rport *rport; +struct nvme_fc_port_info pinfo; +int ret; + + nport =fcloop_alloc_nport (buf, count, true); +if (!nport) +return -EIO ; + + pinfo.node_name = nport->node_name; + pinfo.port_name = nport->port_name; + pinfo.port_role = nport->port_role; + pinfo.port_id = nport->port_id;
Quoted 8 lines
drivers/nvme/target/fcloop.c line 830
Quoted 7 lines
drivers/nvme/target/fcloop.c new file +3
-0+ + ret = nvme_fc_register_remoteport(nport->lport->localport, + &pinfo,&remoteport);
drivers/nvme/target/fcloop.c new file +3
-0+if (ret || !remoteport) { +fcloop_nport_put (nport); +return ret;Possible memory leak for nport??
Nope. the alloc_nport() takes a reference (either allocation, or sharing it with a targetport). Thus, the put releases it. If last reference, it's freed.
Quoted 9 lines
drivers/nvme/target/fcloop.c
Quoted 8 lines
drivers/nvme/target/fcloop.c new file +3
-0+ +static ssize_t +fcloop_create_target_port(struct device *dev, struct
device_attribute*attr,drivers/nvme/target/fcloop.c new file +3
-0+ const char *buf, size_t count) +{ +...Possible memory leak by not de-allocating 'nport'?
Nope: same answer as last. The ref counting will manage it.
Thanks
-- james