On 10/23/19 2:55 PM, James Smart wrote:
diff --git a/drivers/scsi/elx/efct/efct_lio.c b/drivers/scsi/elx/efct/efct_lio.c
new file mode 100644
index 000000000000..c2661ab3e9c3
--- /dev/null
+++ b/drivers/scsi/elx/efct/efct_lio.c
@@ -0,0 +1,2643 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) 2019 Broadcom. All Rights Reserved. The term
+ * “Broadcom” refers to Broadcom Inc. and/or its subsidiaries.
+ */
+
+#include "efct_driver.h"
+
+#include <scsi/scsi.h>
+#include <scsi/scsi_host.h>
+#include <scsi/scsi_device.h>
+#include <scsi/scsi_cmnd.h>
+#include <scsi/scsi_tcq.h>
+#include <target/target_core_base.h>
+#include <target/target_core_fabric.h>
Please do not include SCSI initiator header files in a SCSI target
driver. See also commit ba929992522b ("target: Minimize SCSI header
#include directives").
+#define FABRIC_NAME "efct"
+#define FABRIC_NAME_NPIV "efct_npiv"
Some time ago Christoph Hellwig asked not to use the prefix "fabric" but
to use the prefix "target" instead.
> +#define FABRIC_SNPRINTF_LEN 32
"FABRIC_SNPRINTF_LEN" is a bad choice for the name for this constant.
Please change this into a name that refers to what the purpose of this
constant is (wwn string?) instead of how that string is generated.
+#define FABRIC_SNPRINTF(str, len, pre, wwn) snprintf(str, len, \
+ "%s%02x:%02x:%02x:%02x:%02x:%02x:%02x:%02x", pre, \
+ (u8)((wwn >> 56) & 0xff), (u8)((wwn >> 48) & 0xff), \
+ (u8)((wwn >> 40) & 0xff), (u8)((wwn >> 32) & 0xff), \
+ (u8)((wwn >> 24) & 0xff), (u8)((wwn >> 16) & 0xff), \
+ (u8)((wwn >> 8) & 0xff), (u8)((wwn & 0xff)))
Please convert this macro into a function and choose a better name, e.g.
efct_format_wwn().
+#define ARRAY2WWN(w, a) (w = ((((u64)(a)[0]) << 56) | (((u64)(a)[1]) << 48) | \
+ (((u64)(a)[2]) << 40) | (((u64)(a)[3]) << 32) | \
+ (((u64)(a)[4]) << 24) | (((u64)(a)[5]) << 16) | \
+ (((u64)(a)[6]) << 8) | (((u64)(a)[7]))))
Is this perhaps an open-coded version of get_unaligned_be64()?
+/* Per-target data for virtual targets */
+struct efct_lio_vport_data_t {
+ struct list_head list_entry;
+ bool initiator_mode;
+ bool target_mode;
+ u64 phy_wwpn;
+ u64 phy_wwnn;
+ u64 vport_wwpn;
+ u64 vport_wwnn;
+ struct efct_lio_vport *lio_vport;
+};
+
+/* Per-target data for virtual targets */
+struct efct_lio_vport_list_t {
+ struct list_head list_entry;
+ struct efct_lio_vport *lio_vport;
+};
Two times the same comment for two different data structures?
Additionally, what is a "virtual target"?
+/* local prototypes */
+static char *efct_lio_get_npiv_fabric_wwn(struct se_portal_group *);
+static char *efct_lio_get_fabric_wwn(struct se_portal_group *);
+static u16 efct_lio_get_tag(struct se_portal_group *);
+static u16 efct_lio_get_npiv_tag(struct se_portal_group *);
+static int efct_lio_check_demo_mode(struct se_portal_group *);
+static int efct_lio_check_demo_mode_cache(struct se_portal_group *);
+static int efct_lio_check_demo_write_protect(struct se_portal_group *);
+static int efct_lio_check_prod_write_protect(struct se_portal_group *);
+static int efct_lio_npiv_check_demo_write_protect(struct se_portal_group *);
+static int efct_lio_npiv_check_prod_write_protect(struct se_portal_group *);
+static u32 efct_lio_tpg_get_inst_index(struct se_portal_group *);
+static int efct_lio_check_stop_free(struct se_cmd *se_cmd);
+static void efct_lio_aborted_task(struct se_cmd *se_cmd);
+static void efct_lio_release_cmd(struct se_cmd *);
+static void efct_lio_close_session(struct se_session *);
+static u32 efct_lio_sess_get_index(struct se_session *);
+static int efct_lio_write_pending(struct se_cmd *);
+static void efct_lio_set_default_node_attrs(struct se_node_acl *);
+static int efct_lio_get_cmd_state(struct se_cmd *);
+static int efct_lio_queue_data_in(struct se_cmd *);
+static int efct_lio_queue_status(struct se_cmd *);
+static void efct_lio_queue_tm_rsp(struct se_cmd *);
+static struct se_wwn *efct_lio_make_sport(struct target_fabric_configfs *,
+ struct config_group *, const char *);
+static void efct_lio_drop_sport(struct se_wwn *);
+static void efct_lio_npiv_drop_sport(struct se_wwn *);
+static int efct_lio_parse_wwn(const char *, u64 *, u8 npiv);
+static int efct_lio_parse_npiv_wwn(const char *name, size_t size,
+ u64 *wwpn, u64 *wwnn);
+static struct se_portal_group *efct_lio_make_tpg(struct se_wwn *,
+ const char *);
+static struct se_portal_group *efct_lio_npiv_make_tpg(struct se_wwn *,
+ const char *);
+static void efct_lio_drop_tpg(struct se_portal_group *);
+static struct se_wwn *efct_lio_npiv_make_sport(struct target_fabric_configfs *,
+ struct config_group *,
+ const char *);
+static int
+efct_lio_parse_npiv_wwn(const char *name, size_t size, u64 *wwpn, u64 *wwnn);
+static void efct_lio_npiv_drop_tpg(struct se_portal_group *);
+static int efct_lio_async_worker(struct efct_s *efct);
+static void efct_lio_sg_unmap(struct efct_io_s *io);
+static int efct_lio_abort_tgt_cb(struct efct_io_s *io,
+ enum efct_scsi_io_status_e scsi_status,
+ u32 flags, void *arg);
+
+static int efct_lio_init_nodeacl(struct se_node_acl *, const char *);
+
+static int efct_lio_check_demo_mode_login_only(struct se_portal_group *);
+static int efct_lio_npiv_check_demo_mode_login_only(struct se_portal_group *);
Please reorder the code in this file such that most or all of these
function declarations disappear.
+static ssize_t
+efct_lio_wwn_version_show(struct config_item *item, char *page)
+{
+ return sprintf(page, "Emulex EFCT fabric module version %s\n",
+ __stringify(EFCT_LIO_VERSION));
+}
Version numbers are not useful in upstream code. Please remove this
attribute and also the EFCT_LIO_VERSION constant.
+static struct efct_lio_tpg *
+efct_get_vport_tpg(struct efc_node_s *node)
+{
+ struct efct_s *efct;
+ u64 wwpn = node->sport->wwpn;
+ struct efct_lio_vport_list_t *vport, *next;
+ struct efct_lio_vport *lio_vport = NULL;
+ struct efct_lio_tpg *tpg = NULL;
+ unsigned long flags = 0;
+
+ efct = node->efc->base;
+ spin_lock_irqsave(&efct->tgt_efct.efct_lio_lock, flags);
+ list_for_each_entry_safe(vport, next,
+ &efct->tgt_efct.vport_list, list_entry) {
+ lio_vport = vport->lio_vport;
+ if (wwpn && lio_vport &&
+ lio_vport->npiv_wwpn == wwpn) {
+ efc_log_test(efct, "found tpg on vport\n");
+ tpg = lio_vport->tpg;
+ break;
+ }
+ }
+ spin_unlock_irqrestore(&efct->tgt_efct.efct_lio_lock, flags);
+ return tpg;
+}
The indentation in this function is wrong. list_for_each_entry() should
occur at the same level as spin_lock_irqsave().
+/* local static data */
> +/* local static data */
Are these comments useful?
+#define LIO_IOFMT "[%04x][i:%0*x t:%0*x h:%04x][c:%02x]"
+#define LIO_TMFIOFMT "[%04x][i:%0*x t:%0*x h:%04x][f:%02x]"
+#define LIO_IOFMT_ITT_SIZE(efct) 4
+
+#define efct_lio_io_printf(io, fmt, ...) \
+ efc_log_debug(io->efct, "[%s]" LIO_IOFMT " " fmt, \
+ io->node->display_name, io->instance_index, \
+ LIO_IOFMT_ITT_SIZE(io->efct), io->init_task_tag, \
+ LIO_IOFMT_ITT_SIZE(io->efct), io->tgt_task_tag, io->hw_tag,\
+ (io->tgt_io.cdb ? io->tgt_io.cdb[0] : 0xFF), ##__VA_ARGS__)
+#define efct_lio_tmfio_printf(io, fmt, ...) \
+ efc_log_debug(io->efct, "[%s]" LIO_TMFIOFMT " " fmt,\
+ io->node->display_name, io->instance_index, \
+ LIO_IOFMT_ITT_SIZE(io->efct), io->init_task_tag, \
+ LIO_IOFMT_ITT_SIZE(io->efct), io->tgt_task_tag, io->hw_tag,\
+ io->tgt_io.tmf, ##__VA_ARGS__)
Please remove the LIO_IOFMT, LIO_TMFIOFMT and LIO_IOFMT_ITT_SIZE macros
and expand these macros where these are used. I think that will make the
above logging functions much more easy to read.
+#define efct_lio_io_state_trace(io, value) (io->tgt_io.state |= value)
This macro has "trace" in its name but does not trace anything. Please
either remove this macro or choose a better name.
+static int efct_lio_tgt_session_data(struct efct_s *efct, u64 wwpn,
+ char *buf, int size)
+{
+ struct efc_sli_port_s *sport = NULL;
+ struct efc_node_s *node = NULL;
+ struct efc_lport *efc = efct->efcport;
+ u16 loop_id = 0;
+ int off = 0, rc = 0;
+
+ if (!efc->domain) {
+ efc_log_err(efct, "failed to find efct/domain\n");
+ return -1;
+ }
+
+ list_for_each_entry(sport, &efc->domain->sport_list, list_entry) {
+ if (sport->wwpn == wwpn) {
+ list_for_each_entry(node, &sport->node_list,
+ list_entry) {
+ /* Dump sessions only remote NPORT
+ * sessions
+ */
+ if (efct_lio_node_is_initiator(node)) {
+ rc = snprintf(buf + off,
+ size - off,
+ "0x%016llx,0x%08x,0x%04x\n",
+ be64_to_cpup((__force __be64 *)
+ node->wwpn),
+ node->rnode.fc_id,
+ loop_id);
+ if (rc < 0)
+ break;
+ off += rc;
+ }
+ }
+ }
+ }
+
+ buf[size - 1] = '\0';
+ return 0;
+}
Please use get_unaligned_be64() instead of using __force casts.
+static const struct file_operations efct_debugfs_session_fops = {
+ .owner = THIS_MODULE,
+ .open = efct_debugfs_session_open,
+ .release = efct_debugfs_session_close,
+ .read = efct_debugfs_session_read,
+ .write = efct_debugfs_session_write,
+ .llseek = default_llseek,
+};
+
+static const struct file_operations efct_npiv_debugfs_session_fops = {
+ .owner = THIS_MODULE,
+ .open = efct_npiv_debugfs_session_open,
+ .release = efct_debugfs_session_close,
+ .read = efct_debugfs_session_read,
+ .write = efct_debugfs_session_write,
+ .llseek = default_llseek,
+};
Since the information that is exported through debugfs (logged in
initiators) is information that is also useful for other target drivers,
I think this functionality should be implemented in the target core
instead of in this target driver.
+/* command has been aborted, cleanup here */
+static void efct_lio_aborted_task(struct se_cmd *se_cmd)
+{
+ int rc;
+ struct efct_scsi_tgt_io_s *ocp = container_of(se_cmd,
+ struct efct_scsi_tgt_io_s,
+ cmd);
+ struct efct_io_s *io = container_of(ocp, struct efct_io_s, tgt_io);
+
+ efct_lio_io_trace(io, "%s\n", __func__);
+ efct_lio_io_state_trace(io, EFCT_LIO_STATE_TFO_ABORTED_TASK);
+
+ if (!(se_cmd->transport_state & CMD_T_ABORTED) || ocp->rsp_sent)
+ return;
+
+ /*
+ * if io is non-null, take a reference out on it so it isn't
+ * freed until the abort operation is complete.
+ */
+ if (kref_get_unless_zero(&io->ref) == 0) {
+ /* command no longer active */
+ struct efct_s *efct = io->efct;
+
+ efc_log_test(efct,
+ "success: command no longer active (exists=%d)\n",
+ (io != NULL));
+ return;
+ }
+
+ efct_lio_io_printf(io, "CMD_T_ABORTED set, aborting=%d\n",
+ ocp->aborting);
+ ocp->aborting = true;
+ /* set to non-success so data moves won't continue */
+ ocp->err = EFCT_SCSI_STATUS_ABORTED;
+
+ /* wait until abort is complete; once we return, LIO will call
+ * queue_tm_rsp() which will send response to TMF
+ */
+ init_completion(&io->tgt_io.done);
+
+ rc = efct_scsi_tgt_abort_io(io, efct_lio_abort_tgt_cb, NULL);
+ if (rc == 0) {
+ /* wait for abort to complete before returning */
+ rc = wait_for_completion_timeout(&io->tgt_io.done,
+ usecs_to_jiffies(10000000));
+
+ /* done with reference on aborted IO */
+ kref_put(&io->ref, io->release);
+
+ if (rc) {
+ efct_lio_io_printf(io,
+ "abort completed successfully\n");
+ /* check if TASK_ABORTED status should be sent
+ * for this IO
+ */
+ } else {
+ efct_lio_io_printf(io,
+ "timeout waiting for abort completed\n");
+ }
+ } else {
+ efct_lio_io_printf(io, "Failed to abort\n");
+ }
+}
The .aborted_task() callback function must not wait until the aborted
command has finished but instead must free the resources owned by the
aborted command.
The comment "check if TASK_ABORTED status should be sent for this IO" is
wrong. .aborted_task() is only called if no response will be sent to the
initiator.
+/**
+ * @brief Housekeeping for LIO SG mapping.
+ *
+ * @param io Pointer to IO context.
+ *
+ * @return count Count returned by pci_map_sg.
+ */
The above comment follows the Doxygen syntax. Kernel function headers
must use the kernel-doc syntax. See also
Documentation/process/kernel-docs.rst.
+static struct se_wwn *
+efct_lio_make_sport(struct target_fabric_configfs *tf,
+ struct config_group *group, const char *name)
+{
+ struct efct_lio_sport *lio_sport;
+ struct efct_s *efct;
+ int efctidx, ret;
+ u64 wwpn;
+ char *sessions_name;
+
+ ret = efct_lio_parse_wwn(name, &wwpn, 0);
+ if (ret)
+ return ERR_PTR(ret);
+
+ /* Now search for the HBA that has this WWPN */
+ for (efctidx = 0; efctidx < MAX_EFCT_DEVICES; efctidx++) {
+ u64 pwwn;
+ u8 pn[8];
+
+ efct = efct_devices[efctidx];
+ if (!efct)
+ continue;
+ memcpy(pn, efct_hw_get_ptr(&efct->hw, EFCT_HW_WWN_PORT),
+ sizeof(pn));
+ ARRAY2WWN(pwwn, pn);
+ if (pwwn == wwpn)
+ break;
+ }
+ if (efctidx == MAX_EFCT_DEVICES) {
+ pr_err("cannot find EFCT for wwpn %s\n", name);
+ return ERR_PTR(-ENXIO);
+ }
+ efct = efct_devices[efctidx];
+ lio_sport = kzalloc(sizeof(*lio_sport), GFP_KERNEL);
+ if (!lio_sport)
+ return ERR_PTR(-ENOMEM);
+ lio_sport->efct = efct;
+ lio_sport->wwpn = wwpn;
+ FABRIC_SNPRINTF(lio_sport->wwpn_str, sizeof(lio_sport->wwpn_str),
+ "naa.", wwpn);
+ efct->tgt_efct.lio_sport = lio_sport;
+
+ sessions_name = kasprintf(GFP_KERNEL, "efct-sessions-%d",
+ efct->instance_index);
+ if (sessions_name && efct->sess_debugfs_dir)
+ lio_sport->sessions = debugfs_create_file(sessions_name,
+ 0644,
+ efct->sess_debugfs_dir,
+ lio_sport,
+ &efct_debugfs_session_fops);
+ kfree(sessions_name);
+
+ return &lio_sport->sport_wwn;
+}
+
+static struct se_wwn *
+efct_lio_npiv_make_sport(struct target_fabric_configfs *tf,
+ struct config_group *group, const char *name)
+{
+ struct efct_lio_vport *lio_vport;
+ struct efct_s *efct;
+ int efctidx, ret = -1;
+ u64 p_wwpn, npiv_wwpn, npiv_wwnn;
+ char *p, tmp[128];
+ struct efct_lio_vport_list_t *vport_list;
+ char *sessions_name;
+ struct fc_vport *new_fc_vport;
+ struct fc_vport_identifiers vport_id;
+ unsigned long flags = 0;
+
+ snprintf(tmp, 128, "%s", name);
+
+ p = strchr(tmp, '@');
+
+ if (!p) {
+ pr_err("Unable to find separator operator(@)\n");
+ return ERR_PTR(ret);
+ }
+ *p++ = '\0';
+
+ ret = efct_lio_parse_wwn(tmp, &p_wwpn, 0);
+ if (ret)
+ return ERR_PTR(ret);
+
+ ret = efct_lio_parse_npiv_wwn(p, strlen(p) + 1, &npiv_wwpn, &npiv_wwnn);
+ if (ret)
+ return ERR_PTR(ret);
+
+ /* Now search for the HBA that has this WWPN */
+ for (efctidx = 0; efctidx < MAX_EFCT_DEVICES; efctidx++) {
+ u64 pwwn;
+ u8 pn[8];
+
+ efct = efct_devices[efctidx];
+ if (!efct)
+ continue;
+ if (!efct->xport->req_wwpn) {
+ memcpy(pn, efct_hw_get_ptr(&efct->hw,
+ EFCT_HW_WWN_PORT), sizeof(pn));
+ ARRAY2WWN(pwwn, pn);
+ } else {
+ pwwn = efct->xport->req_wwpn;
+ }
+ if (pwwn == p_wwpn)
+ break;
+ }
+ if (efctidx == MAX_EFCT_DEVICES) {
+ pr_err("cannot find EFCT for base wwpn %s\n", name);
+ return ERR_PTR(-ENXIO);
+ }
+ efct = efct_devices[efctidx];
+ lio_vport = kzalloc(sizeof(*lio_vport), GFP_KERNEL);
+ if (!lio_vport)
+ return ERR_PTR(-ENOMEM);
+
+ lio_vport->efct = efct;
+ lio_vport->wwpn = p_wwpn;
+ lio_vport->npiv_wwpn = npiv_wwpn;
+ lio_vport->npiv_wwnn = npiv_wwnn;
+
+ FABRIC_SNPRINTF(lio_vport->wwpn_str, sizeof(lio_vport->wwpn_str),
+ "naa.", npiv_wwpn);
+
+ vport_list = kmalloc(sizeof(*vport_list), GFP_KERNEL);
+ if (!vport_list) {
+ kfree(lio_vport);
+ return ERR_PTR(-ENOMEM);
+ }
+
+ memset(vport_list, 0, sizeof(struct efct_lio_vport_list_t));
+ vport_list->lio_vport = lio_vport;
+ spin_lock_irqsave(&efct->tgt_efct.efct_lio_lock, flags);
+ INIT_LIST_HEAD(&vport_list->list_entry);
+ list_add_tail(&vport_list->list_entry, &efct->tgt_efct.vport_list);
+ spin_unlock_irqrestore(&efct->tgt_efct.efct_lio_lock, flags);
+
+ sessions_name = kasprintf(GFP_KERNEL, "sessions-npiv-%d",
+ efct->instance_index);
+ if (sessions_name && efct->sess_debugfs_dir)
+ lio_vport->sessions = debugfs_create_file(sessions_name,
+ 0644,
+ efct->sess_debugfs_dir,
+ lio_vport,
+ &efct_npiv_debugfs_session_fops);
+ kfree(sessions_name);
+ memset(&vport_id, 0, sizeof(vport_id));
+ vport_id.port_name = npiv_wwpn;
+ vport_id.node_name = npiv_wwnn;
+ vport_id.roles = FC_PORT_ROLE_FCP_INITIATOR;
+ vport_id.vport_type = FC_PORTTYPE_NPIV;
+ vport_id.disable = false;
+
+ new_fc_vport = fc_vport_create(efct->shost, 0, &vport_id);
+ if (!new_fc_vport) {
+ efc_log_err(efct, "fc_vport_create failed\n");
+ kfree(lio_vport);
+ kfree(vport_list);
+ return ERR_PTR(-ENOMEM);
+ }
+
+ lio_vport->fc_vport = new_fc_vport;
+
+ return &lio_vport->vport_wwn;
+}
Please rework efct_lio_make_sport() and efct_lio_npiv_make_sport() such
that the amount of duplicate code is reduced significantly.
+
+ /* Create kernel worker thread to service async requests
+ * (new/delete initiator, new cmd/tmf). Previously, a worker thread
+ * was needed to make upcalls into LIO because the HW completion
+ * context ran in an interrupt context (tasklet).
+ * This is no longer necessary now that HW completions run in a
+ * kernel thread context. However, performance is much better when
+ * these types of reqs have their own thread.
+ *
+ * Note: We've seen better performance when IO completion (non-async)
+ * upcalls into LIO are not given an additional kernel thread.
+ * Thus,make such upcalls directly from the HW completion kernel thread
+ */
+
+ worker = &efct->tgt_efct.async_worker;
+ efct_mqueue_init(efct, &worker->wq);
+
+ worker->thread = kthread_create((int(*)(void *)) efct_lio_async_worker,
+ efct, "efct_lio_async_worker");
+
+ if (IS_ERR(worker->thread)) {
+ efc_log_err(efct, "kthread_create failed: %ld\n",
+ PTR_ERR(worker->thread));
+ worker->thread = NULL;
+ return -1;
+ }
+
+ wake_up_process(worker->thread);
Please use the kernel workqueue infrastructure instead of duplicating it.
+/**
+ * @brief Worker thread for LIO commands.
+ *
+ * @par Description
+ * This thread is used to make LIO upcalls associated with
+ * asynchronous requests (i.e. new commands received, register
+ * sessions, unregister sessions).
+ *
+ * @param mythread Pointer to the thread object.
+ *
+ * @return Always returns 0.
+ */
+static int efct_lio_async_worker(struct efct_s *efct)
+{
+ struct efct_lio_wq_data_s *wq_data;
+ struct efc_node_s *node;
+ struct se_session *se_sess;
+ int done = 0;
+ bool free_data = true;
+ struct efct_scsi_tgt_io_s *ocp;
+ int dir, rc = 0;
+ struct efct_io_s *io;
+ struct efct_io_s *tmfio;
+ struct efct_scsi_tgt_node_s *tgt_node = NULL;
+
+ while (!done) {
+ /* Poll with a timeout, to keep the kernel from complaining
+ * of not periodically running
+ */
+ wq_data = efct_mqueue_get(&efct->tgt_efct.async_worker.wq,
+ 10000000);
+ if (kthread_should_stop())
+ break;
+
+ if (!wq_data)
+ continue;
+
[ ... ]
+ }
+ if (free_data)
+ kfree(wq_data);
+ }
+
+ complete(&efct->tgt_efct.async_worker.done);
+
+ return 0;
+}
Same comment here: please use the kernel workqueue infrastructure
instead of duplicating it.
+#define scsi_pack_result(key, code, qualifier) (((key & 0xff) << 16) | \
+ ((code && 0xff) << 8) | (qualifier & 0xff))
Where is this macro used? I haven't found any uses of this macro in this
patch.
+#define FABRIC_SNPRINTF_LEN 32
Please choose a better name for this constant. Or even better, leave out
this define entirely and use sizeof().
+static inline int
+efct_mqueue_init(struct efct_s *efct, struct efct_mqueue_s *q)
+{
+ memset(q, 0, sizeof(*q));
+ q->efct = efct;
+ spin_lock_init(&q->lock);
+ init_completion(&q->prod);
+ INIT_LIST_HEAD(&q->queue);
+ return 0;
+}
Functions that are not in the hot path should be defined in a .c file
instead of in a header file.
Thanks,
Bart.
