On 2019-12-20 14:37, James Smart wrote:
> +#include <scsi/scsi_tcq.h>
Including the scsi_tcq.h header file is only useful in source files that
implement initiator functionality. This source file implements SCSI
target functionality. Is this include really necessary?
> +static struct workqueue_struct *lio_wq;
> +
> +static int
> +efct_format_wwn(char *str, size_t len, char *pre, u64 wwn)
> +{
> + u8 a[8];
> +
> + put_unaligned_be64(wwn, a);
> + return snprintf(str, len,
> + "%s%02x:%02x:%02x:%02x:%02x:%02x:%02x:%02x",
> + pre, a[0], a[1], a[2], a[3], a[4], a[5], a[6], a[7]);
> +}
Can the type of 'pre' be changed from 'char *' into 'const char *'?
Can %02x:%02x:%02x:%02x:%02x:%02x:%02x:%02x be changed into %8phC?
> +static int
> +efct_lio_parse_wwn(const char *name, u64 *wwp, u8 npiv)
> +{
> + int a[8], num;
> + u8 b[8];
> +
> + if (npiv) {
> + num = sscanf(name, "%02x%02x%02x%02x%02x%02x%02x%02x",
> + &a[0], &a[1], &a[2], &a[3], &a[4],
> + &a[5], &a[6], &a[7]);
> + } else {
> + num = sscanf(name,
> + "%02x:%02x:%02x:%02x:%02x:%02x:%02x:%02x",
> + &a[0], &a[1], &a[2], &a[3], &a[4],
> + &a[5], &a[6], &a[7]);
> + }
> +
> + if (num != 8)
> + return -EINVAL;
> +
> + for (num = 0; num < 8; ++num)
> + b[num] = (u8) a[num];
> +
> + *wwp = get_unaligned_be64(b);
> + return 0;
> +}
If the %02x sscanf specifiers are changed into %02hhx, can the int a[8]
array be left out?
> +static ssize_t
> +efct_lio_npiv_tpg_enable_store(struct config_item *item, const char *page,
> + size_t count)
> +{
> + struct se_portal_group *se_tpg = to_tpg(item);
> + struct efct_lio_tpg *tpg = container_of(se_tpg,
> + struct efct_lio_tpg, tpg);
> + struct efct_lio_vport *lio_vport = tpg->vport;
> + struct efct_lio_vport_data_t *vport_data;
> + struct efct *efct;
> + struct efc *efc;
> + int ret = -1;
> + unsigned long op, flags = 0;
> +
> + if (kstrtoul(page, 0, &op) < 0)
> + return -EINVAL;
> +
> + if (!lio_vport) {
> + pr_err("Unable to find vport\n");
> + return -EINVAL;
> + }
> +
> + efct = lio_vport->efct;
> + efc = efct->efcport;
> +
> + if (op == 1) {
> + atomic_set(&tpg->enabled, 1);
> + efc_log_debug(efct, "enable portal group %d\n", tpg->tpgt);
> +
> + if (efc->domain) {
> + ret = efc_sport_vport_new(efc->domain,
> + lio_vport->npiv_wwpn,
> + lio_vport->npiv_wwnn,
> + U32_MAX, false, true,
> + NULL, NULL, true);
> + if (ret != 0) {
> + efc_log_err(efct, "Failed to create Vport\n");
> + return ret;
> + }
> + return count;
> + }
> +
> + vport_data = kmalloc(sizeof(*vport_data), GFP_KERNEL);
> + if (!vport_data)
> + return ret;
> +
> + memset(vport_data, 0, sizeof(struct efct_lio_vport_data_t));
> + vport_data->phy_wwpn = lio_vport->wwpn;
> + vport_data->vport_wwpn = lio_vport->npiv_wwpn;
> + vport_data->vport_wwnn = lio_vport->npiv_wwnn;
> + vport_data->target_mode = 1;
> + vport_data->initiator_mode = 0;
> + vport_data->lio_vport = lio_vport;
> +
> + /* There is no domain. Add to pending list. When the
> + * domain is created, the driver will create the vport.
> + */
> + efc_log_debug(efct, "link down, move to pending\n");
> + spin_lock_irqsave(&efct->tgt_efct.efct_lio_lock, flags);
> + INIT_LIST_HEAD(&vport_data->list_entry);
> + list_add_tail(&vport_data->list_entry,
> + &efct->tgt_efct.vport_pend_list);
> + spin_unlock_irqrestore(&efct->tgt_efct.efct_lio_lock, flags);
> +
> + } else if (op == 0) {
> + struct efct_lio_vport_data_t *virt_target_data, *next;
> +
> + efc_log_debug(efct, "disable portal group %d\n", tpg->tpgt);
> +
> + atomic_set(&tpg->enabled, 0);
> + /* only physical sport should exist, free lio_sport
> + * allocated in efct_lio_make_sport
> + */
> + if (efc->domain) {
> + efc_sport_vport_del(efct->efcport, efc->domain,
> + lio_vport->npiv_wwpn,
> + lio_vport->npiv_wwnn);
> + return count;
> + }
> + spin_lock_irqsave(&efct->tgt_efct.efct_lio_lock, flags);
> + list_for_each_entry_safe(virt_target_data, next,
> + &efct->tgt_efct.vport_pend_list,
> + list_entry) {
> + if (virt_target_data->lio_vport == lio_vport) {
> + list_del(&virt_target_data->list_entry);
> + kfree(virt_target_data);
> + break;
> + }
> + }
> + spin_unlock_irqrestore(&efct->tgt_efct.efct_lio_lock, flags);
> + } else {
> + return -EINVAL;
> + }
> + return count;
> +}
I think the above function can return -1. Please make sure that this
function returns an appropriate error code if something fails.
> +static bool efct_lio_node_is_initiator(struct efc_node *node)
> +{
> + if (!node)
> + return 0;
> +
> + if (node->rnode.fc_id && node->rnode.fc_id != FC_FID_FLOGI &&
> + node->rnode.fc_id != FC_FID_DIR_SERV &&
> + node->rnode.fc_id != FC_FID_FCTRL) {
> + return 1;
> + }
> +
> + return 0;
> +}
Should return 0 / return 1 perhaps be changed into return false / return
true?
> +static int efct_lio_tgt_session_data(struct efct *efct, u64 wwpn,
> + char *buf, int size)
> +{
> + struct efc_sli_port *sport = NULL;
> + struct efc_node *node = NULL;
> + struct efc *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)
> + continue;
> + list_for_each_entry(node, &sport->node_list,
> + list_entry) {
> + /* Dump only remote NPORT sessions */
> + if (!efct_lio_node_is_initiator(node))
> + continue;
> +
> + rc = snprintf(buf + off, size - off,
> + "0x%016llx,0x%08x,0x%04x\n",
> + get_unaligned_be64(node->wwpn),
> + node->rnode.fc_id, loop_id);
> + if (rc < 0)
> + break;
> + off += rc;
> + }
> + }
> +
> + buf[size - 1] = '\0';
> + return 0;
> +}
Does the caller of this function initialize buf[]? If not, should this
function initialize buf[] before calling snprintf()?
Since snprintf() guarantees '\0' termination I think that the buf[size -
1] = '\0' at the end of this function can be left out.
> +static int
> +efct_lio_datamove_done(struct efct_io *io, enum efct_scsi_io_status scsi_status,
> + u32 flags, void *arg);
Can this forward declaration be avoided by reordering the function
definitions?
> +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 *efct;
> + int ret = -1;
> + u64 p_wwpn, npiv_wwpn, npiv_wwnn;
> + char *p, tmp[128];
> + struct efct_lio_vport_list_t *vport_list;
> + struct fc_vport *new_fc_vport;
> + struct fc_vport_identifiers vport_id;
> + unsigned long flags = 0;
> +
> + snprintf(tmp, 128, "%s", name);
How about using sizeof(tmp) instead of hardcoding the array size?
> + p = strchr(tmp, '@');
> +
> + if (!p) {
> + pr_err("Unable to find separator operator(@)\n");
> + return ERR_PTR(ret);
> + }
> + *p++ = '\0';
Can this be changed into a strsep() call?
> +int efct_scsi_tgt_del_device(struct efct *efct)
> +{
> + int rc = 0;
> +
> + flush_workqueue(lio_wq);
> +
> + return rc;
> +}
Is the 'rc' variable necessary in the above function? Can it be removed?
> +/* Called by the libefc when an initiator goes away. */
> +int efct_scsi_del_initiator(struct efc *efc, struct efc_node *node,
> + int reason)
> +{
> + struct efct *efct = node->efc->base;
> + struct efct_lio_wq_data *wq_data;
> + int watermark;
> + int initiator_count;
> +
> + if (reason == EFCT_SCSI_INITIATOR_MISSING)
> + return EFCT_SCSI_CALL_COMPLETE;
> +
> + wq_data = kmalloc(sizeof(*wq_data), GFP_ATOMIC);
> + if (!wq_data)
> + return EFCT_SCSI_CALL_COMPLETE;
> +
> + memset(wq_data, 0, sizeof(*wq_data));
> + wq_data->ptr = node;
> + wq_data->efct = efct;
> + INIT_WORK(&wq_data->work, efct_lio_remove_session);
> + queue_work(lio_wq, &wq_data->work);
> +
> + /*
> + * update IO watermark: decrement initiator count
> + */
> + initiator_count =
> + atomic_sub_return(1, &efct->tgt_efct.initiator_count);
> + watermark = (efct->tgt_efct.watermark_max -
> + initiator_count * EFCT_IO_WATERMARK_PER_INITIATOR);
> + watermark = (efct->tgt_efct.watermark_min > watermark) ?
> + efct->tgt_efct.watermark_min : watermark;
> + atomic_set(&efct->tgt_efct.io_high_watermark, watermark);
> +
> + return EFCT_SCSI_CALL_ASYNC;
> +}
Is the lio_wq work queue really necessary? Could one of the system
workqueues have been used instead?
> + ret = kstrtoul(page, 0, &val); \
> + if (ret < 0) { \
> + pr_err("kstrtoul() failed with ret: %d\n", ret); \
> + return -EINVAL; \
> + } \
Has it been considered to return 'ret' (the kstrtoul() return value)
instead of -EINVAL?
> + ret = kstrtoul(page, 0, &val); \
> + if (ret < 0) { \
> + pr_err("kstrtoul() failed with ret: %d\n", ret); \
> + return -EINVAL; \
> + } \
Same comment here.
> +#define efct_set_lio_io_state(io, value) (io->tgt_io.state |= value)
Is this macro really useful? Can it be removed?
> +struct efct_scsi_tgt_io {
> + struct se_cmd cmd;
> + unsigned char sense_buffer[TRANSPORT_SENSE_BUFFER];
> + int ddir;
Should 'int' perhaps be changed into 'enum dma_data_direction'?
> + u8 cdb_opcode;
Does this duplicate cmd.t_task_cdb[0]? If so, is it useful to duplicate
that value?
> + u32 cdb_len;
Is this value identical to scsi_command_size(cmd.t_task_cdb)? Is it
essential to have this member in this data structure?
Thanks,
Bart.
