On Fri, 2016-07-22 at 17:23 -0700, James Smart wrote:
A couple comments.
> 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.
>
>
> Signed-off-by: James Smart <james.smart@xxxxxxxxxxxx>
>
> ---
snip...
> +int
> +fcloop_fcp_op(struct nvmet_fc_target_port *tgtport,
> + struct nvmefc_tgt_fcp_req *tgt_fcpreq)
> +{
> + struct fcloop_fcpreq *tfcp_req =
> + container_of(tgt_fcpreq, struct fcloop_fcpreq,
> tgt_fcp_req);
> + struct nvmefc_fcp_req *fcpreq = tfcp_req->fcpreq;
> + u32 rsplen = 0, xfrlen = 0;
> + int fcp_err = 0;
> + u8 op = tgt_fcpreq->op;
> +
> + switch (op) {
> + case NVMET_FCOP_WRITEDATA:
> + xfrlen = tgt_fcpreq->transfer_length;
> + fcloop_fcp_copy_data(op, tgt_fcpreq->sg, fcpreq
> ->first_sgl,
> + tgt_fcpreq->offset, xfrlen);
> + fcpreq->transferred_length += xfrlen;
> + break;
> +
> + case NVMET_FCOP_READDATA:
> + case NVMET_FCOP_READDATA_RSP:
> + xfrlen = tgt_fcpreq->transfer_length;
> + fcloop_fcp_copy_data(op, tgt_fcpreq->sg, fcpreq
> ->first_sgl,
> + tgt_fcpreq->offset, xfrlen);
> + fcpreq->transferred_length += xfrlen;
> + if (op == NVMET_FCOP_READDATA)
> + break;
> +
> + /* Fall-Thru to RSP handling */
> +
> + case NVMET_FCOP_RSP:
> + rsplen = ((fcpreq->rsplen < tgt_fcpreq->rsplen) ?
> + fcpreq->rsplen : tgt_fcpreq
> ->rsplen);
> + memcpy(fcpreq->rspaddr, tgt_fcpreq->rspaddr,
> rsplen);
> + if (rsplen < tgt_fcpreq->rsplen)
> + fcp_err = -E2BIG;
> + fcpreq->rcv_rsplen = rsplen;
> + fcpreq->status = 0;
> + tfcp_req->status = 0;
> + break;
> +
> + case NVMET_FCOP_ABORT:
> + tfcp_req->status = NVME_SC_FC_TRANSPORT_ABORTED;
> + break;
> +
> + default:
> + fcp_err = -EINVAL;
> + break;
> + }
> +
> + tgt_fcpreq->transferred_length = xfrlen;
> + tgt_fcpreq->fcp_error = fcp_err;
> + tgt_fcpreq->done(tgt_fcpreq);
> +
> + if ((!fcp_err) && (op == NVMET_FCOP_RSP ||
> + op == NVMET_FCOP_READDATA_RSP ||
> + op == NVMET_FCOP_ABORT))
> + schedule_work(&tfcp_req->work);
> +
> + return 0;
if this function returns an 'int', why would it always return 0 and not
the fcp_err values (if there is an error)?
> +}
> +
> +void
> +fcloop_ls_abort(struct nvme_fc_local_port *localport,
> + struct nvme_fc_remote_port *remoteport,
> + struct nvmefc_ls_req *lsreq)
> +{
> +}
> +
> +void
> +fcloop_fcp_abort(struct nvme_fc_local_port *localport,
> + struct nvme_fc_remote_port *remoteport,
> + void *hw_queue_handle,
> + struct nvmefc_fcp_req *fcpreq)
> +{
> +}
> +
> +
> +struct nvme_fc_port_template fctemplate = {
> + .create_queue = fcloop_create_queue,
> + .delete_queue = fcloop_delete_queue,
> + .ls_req = fcloop_ls_req,
> + .fcp_io = fcloop_fcp_req,
> + .ls_abort = fcloop_ls_abort,
> + .fcp_abort = fcloop_fcp_abort,
> +
> + .max_hw_queues = 1,
> + .max_sgl_segments = 256,
> + .max_dif_sgl_segments = 256,
> + .dma_boundary = 0xFFFFFFFF,
Between here and "struct nvmet_fc_target_template tgttemplate" they are
assigning the same magic values to the same variable names, so why not
have these values as #defines for a tad easier maintainability?
> + /* sizes of additional private data for data structures */
> + .local_priv_sz = sizeof(struct fcloop_lport),
> + .remote_priv_sz = sizeof(struct fcloop_rport),
> + .lsrqst_priv_sz = sizeof(struct fcloop_lsreq),
> + .fcprqst_priv_sz = sizeof(struct fcloop_fcpreq),
> +};
> +
> +struct nvmet_fc_target_template tgttemplate = {
> + .xmt_ls_rsp = fcloop_xmt_ls_rsp,
> + .fcp_op = fcloop_fcp_op,
> +
> + .max_hw_queues = 1,
> + .max_sgl_segments = 256,
> + .max_dif_sgl_segments = 256,
> + .dma_boundary = 0xFFFFFFFF,
> +
see above comment.
> + /* optional features */
> + .target_features = NVMET_FCTGTFEAT_READDATA_RSP,
> +
> + /* sizes of additional private data for data structures */
> + .target_priv_sz = sizeof(struct fcloop_tgtport),
> +};
> +
> +static ssize_t
> +fcloop_create_local_port(struct device *dev, struct device_attribute
> *attr,
> + const char *buf, size_t count)
> +{
> + struct nvme_fc_port_info pinfo;
> + struct fcloop_ctrl_options *opts;
> + struct nvme_fc_local_port *localport;
> + struct fcloop_lport *lport;
> + int ret;
> +
> + opts = kzalloc(sizeof(*opts), GFP_KERNEL);
> + if (!opts)
> + return -ENOMEM;
> +
> + ret = fcloop_parse_options(opts, buf);
> + if (ret)
> + goto out_free_opts;
> +
> + /* everything there ? */
> + if ((opts->mask & LPORT_OPTS) != LPORT_OPTS) {
> + ret = -EINVAL;
> + goto out_free_opts;
> + }
> +
> + pinfo.fabric_name = opts->fabric;
> + pinfo.node_name = opts->wwnn;
> + pinfo.port_name = opts->wwpn;
> + pinfo.port_role = opts->roles;
> + pinfo.port_id = opts->fcaddr;
> +
> + ret = nvme_fc_register_localport(&pinfo, &fctemplate, NULL,
> &localport);
> + if (!ret) {
> + /* success */
> + lport = localport->private;
> + lport->localport = localport;
> + INIT_LIST_HEAD(&lport->list);
> + INIT_LIST_HEAD(&lport->rport_list);
> + list_add_tail(&lport->list, &fcloop_lports);
> +
> + /* mark all of the input buffer consumed */
> + ret = count;
> + }
> +
> +out_free_opts:
> + kfree(opts);
> + return ret;
> +}
> +
> +static int __delete_local_port(struct fcloop_lport *lport)
> +{
> + int ret;
> +
> + if (!list_empty(&lport->rport_list))
> + return -EBUSY;
> +
> + list_del(&lport->list);
> +
Is a mutex or locking mechanism not needed here for this list?
> + ret = nvme_fc_unregister_localport(lport->localport);
> +
> + return ret;
> +}
> +
> +static ssize_t
> +fcloop_delete_local_port(struct device *dev, struct device_attribute
> *attr,
> + const char *buf, size_t count)
> +{
> + struct fcloop_lport *lport, *lnext;
> + u64 fabric, portname;
> + int ret;
> +
> + ret = fcloop_parse_nm_options(dev, &fabric, &portname, buf);
> + if (ret)
> + return ret;
> +
> + list_for_each_entry_safe(lport, lnext, &fcloop_lports, list)
> {
> + if ((lport->localport->fabric_name == fabric) &&
> + (lport->localport->port_name == portname)) {
> + break;
> + }
> + }
> + if (is_end_of_list(lport, &fcloop_lports, list))
> + return -ENOENT;
> +
> + ret = __delete_local_port(lport);
> +
> + if (!ret)
> + return count;
> +
> + return ret;
> +}
> +
> +static ssize_t
> +fcloop_create_remote_port(struct device *dev, struct
> device_attribute *attr,
> + const char *buf, size_t count)
> +{
> + struct fcloop_ctrl_options *opts;
> + struct fcloop_lport *lport, *lnext;
> + struct nvme_fc_remote_port *remoteport;
> + struct fcloop_rport *rport;
> + struct nvme_fc_port_info pinfo;
> + struct nvmet_fc_port_info tinfo;
> + int ret;
> +
> + opts = kzalloc(sizeof(*opts), GFP_KERNEL);
> + if (!opts)
> + return -ENOMEM;
> +
> + ret = fcloop_parse_options(opts, buf);
> + if (ret)
> + goto out_free_opts;
> +
> + /* everything there ? */
> + if ((opts->mask & RPORT_OPTS) != RPORT_OPTS) {
> + ret = -EINVAL;
> + goto out_free_opts;
> + }
> +
> + pinfo.fabric_name = tinfo.fabric_name = opts->fabric;
> + pinfo.node_name = tinfo.node_name = opts->wwnn;
> + pinfo.port_name = tinfo.port_name = opts->wwpn;
> + pinfo.port_role = opts->roles;
> + pinfo.port_id = tinfo.port_id = opts->fcaddr;
> +
> + list_for_each_entry_safe(lport, lnext, &fcloop_lports, list)
> {
> + if (lport->localport->fabric_name == opts->fabric)
> + break;
> + }
> + if (is_end_of_list(lport, &fcloop_lports, list)) {
> + ret = -ENOENT;
> + goto out_free_opts;
> + }
> +
> + ret = nvme_fc_register_remoteport(lport->localport, &pinfo,
> + &remoteport);
> + if (ret)
> + goto out_free_opts;
> +
> + /* success */
> + rport = remoteport->private;
> + rport->remoteport = remoteport;
> + INIT_LIST_HEAD(&rport->list);
> + list_add_tail(&rport->list, &lport->rport_list);
is there not a mutex or locking mechanism needed when manipulating this
list?
> +
> + /* tie into nvme target side */
> + ret = nvmet_fc_register_targetport(&tinfo, &tgttemplate,
> NULL,
> + &rport->targetport);
> + if (ret) {
> + list_del(&rport->list);
> + (void)nvme_fc_unregister_remoteport(remoteport);
> + } else {
> + struct fcloop_tgtport *tport;
> +
> + tport = rport->targetport->private;
> + tport->rport = rport;
> + tport->lport = lport;
> + tport->tgtport = rport->targetport;
> +
> + /* mark all of the input buffer consumed */
> + ret = count;
> + }
> +
> +out_free_opts:
> + kfree(opts);
> + return ret;
> +}
> +
Thanks,
J
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
