Re: [PATCH v3] cxlflash: Base support for IBM CXL Flash Adapter

[Brian King <brking@linux.vnet.ibm.com>]

On 06/02/2015 06:55 PM, Matthew R. Ochs wrote:
> +/**
> + * send_tmf() - sends a Task Management Function (TMF)
> + * @afu:	AFU to checkout from.
> + * @scp:	SCSI command from stack.
> + * @tmfcmd:	TMF command to send.
> + *
> + * Return:
> + *	0 on success
> + *	SCSI_MLQUEUE_HOST_BUSY when host is busy
> + */
> +static int send_tmf(struct afu *afu, struct scsi_cmnd *scp, u64 tmfcmd)
> +{
> +	struct afu_cmd *cmd;
> +
> +	u32 port_sel = scp->device->channel + 1;
> +	short lflag = 0;
> +	struct Scsi_Host *host = scp->device->host;
> +	struct cxlflash_cfg *cfg = (struct cxlflash_cfg *)host->hostdata;
> +	int rc = 0;
> +
> +	write_lock(&cfg->tmf_lock);

What is this lock protecting? The only thing it seems to be accomplishing is
making sure one thread isn't sending a TMF and another thread is sending
a normal I/O command at the exact same time, yet it looks like you still
allow a TMF to be sent and a normal I/O to be sent immediately after, before
receiving the TMF response.

> +
> +	cmd = cxlflash_cmd_checkout(afu);
> +	if (unlikely(!cmd)) {
> +		pr_err("%s: could not get a free command\n", __func__);
> +		rc = SCSI_MLQUEUE_HOST_BUSY;
> +		goto out;
> +	}
> +
> +	cmd->rcb.ctx_id = afu->ctx_hndl;
> +	cmd->rcb.port_sel = port_sel;
> +	cmd->rcb.lun_id = lun_to_lunid(scp->device->lun);
> +
> +	lflag = SISL_REQ_FLAGS_TMF_CMD;
> +
> +	cmd->rcb.req_flags = (SISL_REQ_FLAGS_PORT_LUN_ID |
> +				SISL_REQ_FLAGS_SUP_UNDERRUN | lflag);
> +
> +	/* Stash the scp in the reserved field, for reuse during interrupt */
> +	cmd->rcb.scp = scp;
> +
> +	/* Copy the CDB from the cmd passed in */
> +	memcpy(cmd->rcb.cdb, &tmfcmd, sizeof(tmfcmd));
> +
> +	/* Send the command */
> +	rc = cxlflash_send_cmd(afu, cmd);
> +out:
> +	write_unlock(&cfg->tmf_lock);
> +	return rc;
> +
> +}
> +

> +/**
> + * cxlflash_send_cmd() - sends an AFU command
> + * @afu:	AFU associated with the host.
> + * @cmd:	AFU command to send.
> + *
> + * Return:
> + *	0 on success
> + *	-1 on failure
> + */
> +int cxlflash_send_cmd(struct afu *afu, struct afu_cmd *cmd)
> +{
> +	int nretry = 0;
> +	int rc = 0;
> +
> +	/* send_cmd is used by critical users such an AFU sync and to
> +	 * send a task management function (TMF). So we do want to retry
> +	 * a bit before returning an error.
> +	 */
> +	do {
> +		afu->room = readq_be(&afu->host_map->cmd_room);

Looks like you now have an MMIO load as part of sending every command,
including commands coming from queuecommand. Won't that be a performance issue?
Is there any way to avoid this? Could you perhaps decrement afu->room
in this function and only re-read it from the AFU when the counter hits zero?

> +		if (afu->room)
> +			break;
> +		udelay(nretry);
> +	} while (nretry++ < MC_ROOM_RETRY_CNT);
> +
> +	/* Write IOARRIN */
> +	if (afu->room)
> +		writeq_be((u64)&cmd->rcb, &afu->host_map->ioarrin);
> +	else {
> +		pr_err("%s: no cmd_room to send 0x%X\n",
> +		       __func__, cmd->rcb.cdb[0]);
> +		rc = SCSI_MLQUEUE_HOST_BUSY;
> +	}
> +
> +	pr_debug("%s: cmd=%p len=%d ea=%p rc=%d\n", __func__, cmd,
> +		 cmd->rcb.data_len, (void *)cmd->rcb.data_ea, rc);
> +
> +	return rc;
> +}
> +



-- 
Brian King
Power Linux I/O
IBM Linux Technology Center