Re: [PATCH v2 24/32] elx: efct: LIO backend interface routines

[Bart Van Assche <bvanassche@acm.org>]

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.