Re: [PATCH 35/36] usb: gadget: f_tcm: Handle TASK_MANAGEMENT commands

[Thinh Nguyen <Thinh.Nguyen@synopsys.com>]

On 7/8/2022, Dmitry Bogdanov wrote:
> On Wed, Jul 06, 2022 at 04:38:01PM -0700, Thinh Nguyen wrote:
>> Handle target_core_fabric_ops TASK MANAGEMENT functions and their
>> response. If a TASK MANAGEMENT command is received, the driver will
>> interpret the function TMF_*, translate to TMR_*, and fire off a command
>> work executing target_submit_tmr(). On completion, it will handle the
>> TASK MANAGEMENT response through uasp_send_tm_response().
>>
>> Signed-off-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com>
>> ---
>> NOTE: I appologize for this big patch. I feel that this feature needs to be
>> viewed in its entirety to see the whole picture and easier review.
>>
>>
>>   drivers/usb/gadget/function/f_tcm.c | 260 +++++++++++++++++++++++++---
>>   drivers/usb/gadget/function/tcm.h   |   7 +-
>>   2 files changed, 241 insertions(+), 26 deletions(-)
>>
>> diff --git a/drivers/usb/gadget/function/f_tcm.c b/drivers/usb/gadget/function/f_tcm.c
>> index fa09999adda7..a68436f97f91 100644
>> --- a/drivers/usb/gadget/function/f_tcm.c
>> +++ b/drivers/usb/gadget/function/f_tcm.c
>> @@ -12,6 +12,7 @@
>>   #include <linux/string.h>
>>   #include <linux/configfs.h>
>>   #include <linux/ctype.h>
>> +#include <linux/delay.h>
>>   #include <linux/usb/ch9.h>
>>   #include <linux/usb/composite.h>
>>   #include <linux/usb/gadget.h>
>> @@ -462,6 +463,53 @@ static int usbg_bot_setup(struct usb_function *f,
>>   
>>   /* Start uas.c code */
>>   
>> +static int tcm_to_uasp_response(enum tcm_tmrsp_table code)
>> +{
>> +	switch (code) {
>> +	case TMR_FUNCTION_FAILED:
>> +		return RC_TMF_FAILED;
>> +	case TMR_FUNCTION_COMPLETE:
>> +		return RC_TMF_COMPLETE;
>> +	case TMR_FUNCTION_REJECTED:
>> +		return RC_TMF_NOT_SUPPORTED;
>> +	case TMR_LUN_DOES_NOT_EXIST:
>> +		return RC_INCORRECT_LUN;
>> +	case TMR_OVERLAPPED_TAG_ATTEMPTED:
>> +		return RC_OVERLAPPED_TAG;
>> +	case TMR_TASK_DOES_NOT_EXIST:
>> +		return RC_INVALID_INFO_UNIT;
>> +	case TMR_TASK_MGMT_FUNCTION_NOT_SUPPORTED:
>> +	default:
>> +		return RC_TMF_NOT_SUPPORTED;
>> +	}
>> +}
>> +
>> +static unsigned char uasp_to_tcm_func(int code)
>> +{
>> +	switch (code) {
>> +	case TMF_ABORT_TASK:
>> +		return TMR_ABORT_TASK;
>> +	case TMF_ABORT_TASK_SET:
>> +		return TMR_ABORT_TASK_SET;
>> +	case TMF_CLEAR_TASK_SET:
>> +		return TMR_CLEAR_TASK_SET;
>> +	case TMF_LOGICAL_UNIT_RESET:
>> +		return TMR_LUN_RESET;
>> +	case TMF_I_T_NEXUS_RESET:
>> +		return TMR_I_T_NEXUS_RESET;
>> +	case TMF_CLEAR_ACA:
>> +		return TMR_CLEAR_ACA;
>> +	case TMF_QUERY_TASK:
>> +		return TMR_QUERY_TASK;
>> +	case TMF_QUERY_TASK_SET:
>> +		return TMR_QUERY_TASK_SET;
>> +	case TMF_QUERY_ASYNC_EVENT:
>> +		return TMR_QUERY_ASYNC_EVENT;
>> +	default:
>> +		return TMR_UNKNOWN;
>> +	}
>> +}
>> +
>>   static void uasp_cleanup_one_stream(struct f_uas *fu, struct uas_stream *stream)
>>   {
>>   	/* We have either all three allocated or none */
>> @@ -506,6 +554,11 @@ static void uasp_cleanup_old_alt(struct f_uas *fu)
>>   	uasp_free_cmdreq(fu);
>>   }
>>   
>> +static struct uas_stream *uasp_get_stream_by_tag(struct f_uas *fu, u16 tag)
>> +{
>> +	return &fu->stream[tag % USBG_NUM_CMDS];
>> +}
>> +
>>   static void uasp_status_data_cmpl(struct usb_ep *ep, struct usb_request *req);
>>   
>>   static int uasp_prepare_r_request(struct usbg_cmd *cmd)
>> @@ -513,7 +566,7 @@ static int uasp_prepare_r_request(struct usbg_cmd *cmd)
>>   	struct se_cmd *se_cmd = &cmd->se_cmd;
>>   	struct f_uas *fu = cmd->fu;
>>   	struct usb_gadget *gadget = fuas_to_gadget(fu);
>> -	struct uas_stream *stream = cmd->stream;
>> +	struct uas_stream *stream = uasp_get_stream_by_tag(fu, cmd->tag);
>>   
>>   	if (!gadget->sg_supported) {
>>   		cmd->data_buf = kmalloc(se_cmd->data_length, GFP_ATOMIC);
>> @@ -546,7 +599,7 @@ static void uasp_prepare_status(struct usbg_cmd *cmd)
>>   {
>>   	struct se_cmd *se_cmd = &cmd->se_cmd;
>>   	struct sense_iu *iu = &cmd->sense_iu;
>> -	struct uas_stream *stream = cmd->stream;
>> +	struct uas_stream *stream = uasp_get_stream_by_tag(cmd->fu, cmd->tag);
>>   
>>   	cmd->state = UASP_QUEUE_COMMAND;
>>   	iu->iu_id = IU_ID_STATUS;
>> @@ -565,11 +618,36 @@ static void uasp_prepare_status(struct usbg_cmd *cmd)
>>   	stream->req_status->complete = uasp_status_data_cmpl;
>>   }
>>   
>> +static void uasp_prepare_response(struct usbg_cmd *cmd)
>> +{
>> +	struct se_cmd *se_cmd = &cmd->se_cmd;
>> +	struct response_iu *rsp_iu = &cmd->response_iu;
>> +	struct uas_stream *stream = uasp_get_stream_by_tag(cmd->fu, cmd->tag);
>> +
>> +	cmd->state = UASP_QUEUE_COMMAND;
>> +	rsp_iu->iu_id = IU_ID_RESPONSE;
>> +	rsp_iu->tag = cpu_to_be16(cmd->tag);
>> +
>> +	if (cmd->tmr_rsp != TMR_RESPONSE_UNKNOWN)
>> +		rsp_iu->response_code =
>> +			tcm_to_uasp_response(cmd->tmr_rsp);
>> +	else
>> +		rsp_iu->response_code =
>> +			tcm_to_uasp_response(se_cmd->se_tmr_req->response);
>> +
>> +	stream->req_status->is_last = 1;
>> +	stream->req_status->stream_id = cmd->tag;
>> +	stream->req_status->context = cmd;
>> +	stream->req_status->length = sizeof(struct response_iu);
>> +	stream->req_status->buf = rsp_iu;
>> +	stream->req_status->complete = uasp_status_data_cmpl;
>> +}
>> +
>>   static void uasp_status_data_cmpl(struct usb_ep *ep, struct usb_request *req)
>>   {
>>   	struct usbg_cmd *cmd = req->context;
>> -	struct uas_stream *stream = cmd->stream;
>>   	struct f_uas *fu = cmd->fu;
>> +	struct uas_stream *stream = uasp_get_stream_by_tag(fu, cmd->tag);
>>   	struct se_session *se_sess = cmd->se_cmd.se_sess;
>>   	int ret;
>>   
>> @@ -604,6 +682,7 @@ static void uasp_status_data_cmpl(struct usb_ep *ep, struct usb_request *req)
>>   		break;
>>   
>>   	case UASP_QUEUE_COMMAND:
>> +		stream->cmd = NULL;
>>   
>>   		target_free_tag(se_sess, &cmd->se_cmd);
>>   		transport_generic_free_cmd(&cmd->se_cmd, 0);
>> @@ -617,6 +696,7 @@ static void uasp_status_data_cmpl(struct usb_ep *ep, struct usb_request *req)
>>   	return;
>>   
>>   cleanup:
>> +	stream->cmd = NULL;
>>   	target_free_tag(se_sess, &cmd->se_cmd);
>>   	transport_generic_free_cmd(&cmd->se_cmd, 0);
>>   }
>> @@ -624,7 +704,7 @@ static void uasp_status_data_cmpl(struct usb_ep *ep, struct usb_request *req)
>>   static int uasp_send_status_response(struct usbg_cmd *cmd)
>>   {
>>   	struct f_uas *fu = cmd->fu;
>> -	struct uas_stream *stream = cmd->stream;
>> +	struct uas_stream *stream = uasp_get_stream_by_tag(fu, cmd->tag);
>>   	struct sense_iu *iu = &cmd->sense_iu;
>>   
>>   	iu->tag = cpu_to_be16(cmd->tag);
>> @@ -633,10 +713,22 @@ static int uasp_send_status_response(struct usbg_cmd *cmd)
>>   	return usb_ep_queue(fu->ep_status, stream->req_status, GFP_ATOMIC);
>>   }
>>   
>> +static int uasp_send_tm_response(struct usbg_cmd *cmd)
>> +{
>> +	struct f_uas *fu = cmd->fu;
>> +	struct uas_stream *stream = uasp_get_stream_by_tag(fu, cmd->tag);
>> +	struct response_iu *iu = &cmd->response_iu;
>> +
>> +	iu->tag = cpu_to_be16(cmd->tag);
>> +	cmd->fu = fu;
>> +	uasp_prepare_response(cmd);
>> +	return usb_ep_queue(fu->ep_status, stream->req_status, GFP_ATOMIC);
>> +}
>> +
>>   static int uasp_send_read_response(struct usbg_cmd *cmd)
>>   {
>>   	struct f_uas *fu = cmd->fu;
>> -	struct uas_stream *stream = cmd->stream;
>> +	struct uas_stream *stream = uasp_get_stream_by_tag(fu, cmd->tag);
>>   	struct sense_iu *iu = &cmd->sense_iu;
>>   	int ret;
>>   
>> @@ -682,7 +774,7 @@ static int uasp_send_read_response(struct usbg_cmd *cmd)
>>   static int uasp_send_write_request(struct usbg_cmd *cmd)
>>   {
>>   	struct f_uas *fu = cmd->fu;
>> -	struct uas_stream *stream = cmd->stream;
>> +	struct uas_stream *stream = uasp_get_stream_by_tag(fu, cmd->tag);
>>   	struct sense_iu *iu = &cmd->sense_iu;
>>   	int ret;
>>   
>> @@ -943,8 +1035,10 @@ static void usbg_data_write_cmpl(struct usb_ep *ep, struct usb_request *req)
>>   {
>>   	struct usbg_cmd *cmd = req->context;
>>   	struct se_cmd *se_cmd = &cmd->se_cmd;
>> +	struct uas_stream *stream = uasp_get_stream_by_tag(cmd->fu, cmd->tag);
>>   
>>   	if (req->status == -ESHUTDOWN) {
>> +		stream->cmd = NULL;
>>   		target_free_tag(se_cmd->se_sess, se_cmd);
>>   		transport_generic_free_cmd(&cmd->se_cmd, 0);
>>   		return;
>> @@ -962,6 +1056,7 @@ static void usbg_data_write_cmpl(struct usb_ep *ep, struct usb_request *req)
>>   				se_cmd->data_length);
>>   	}
>>   
>> +	cmd->state = UASP_QUEUE_COMMAND;
>>   	target_execute_cmd(se_cmd);
>>   	return;
>>   
>> @@ -1042,9 +1137,66 @@ static int usbg_send_read_response(struct se_cmd *se_cmd)
>>   		return uasp_send_read_response(cmd);
>>   }
>>   
>> -static void usbg_cmd_work(struct work_struct *work)
>> +static void usbg_submit_tmr(struct usbg_cmd *cmd)
>> +{
>> +	struct se_cmd *se_cmd;
>> +	struct tcm_usbg_nexus *tv_nexus;
>> +	struct uas_stream *stream;
>> +	int flags = TARGET_SCF_ACK_KREF;
>> +
>> +	se_cmd = &cmd->se_cmd;
>> +	tv_nexus = cmd->fu->tpg->tpg_nexus;
>> +	stream = uasp_get_stream_by_tag(cmd->fu, cmd->tag);
>> +
>> +	/* Failure detected by f_tcm */
>> +	if (cmd->tmr_rsp != TMR_RESPONSE_UNKNOWN) {
>> +		if (cmd->tmr_rsp == TMR_OVERLAPPED_TAG_ATTEMPTED) {
>> +			/*
>> +			 * There's no guarantee of a matching completion order
>> +			 * between different endpoints. i.e. The device may
>> +			 * receive a new (CDB) command request completion of the
>> +			 * command endpoint before it gets notified of the
>> +			 * previous command status completion from a status
>> +			 * endpoint. The driver still needs to detect
>> +			 * misbehaving host and respond with an overlap command
>> +			 * tag. To prevent false overlapped tag failure, give
>> +			 * the active and matching stream id a short time (1ms)
>> +			 * to complete before respond with overlapped command
>> +			 * failure.
>> +			 */
>> +			msleep(1);
>> +
>> +			/* If the stream is completed, retry the command */
>> +			if (!stream->cmd) {
>> +				usbg_submit_command(cmd->fu, cmd->req);
>> +				return;
>> +			}
>> +
>> +			/* Overlap command tag detected. Abort command. */
>> +			cmd->state = UASP_QUEUE_COMMAND;
>> +			stream->cmd->se_cmd.transport_state |= CMD_T_ABORTED;
>> +			target_get_sess_cmd(&stream->cmd->se_cmd, true);
>> +
>> +			/* This will trigger command abort handler */
>> +			target_execute_cmd(&stream->cmd->se_cmd);
>> +			transport_generic_free_cmd(&stream->cmd->se_cmd, 1);
>> +		}
>> +
>> +
>> +		target_submit_tmr_fail_response(se_cmd, cmd->tmr_rsp,
>> +				tv_nexus->tvn_se_sess, cmd->unpacked_lun,
>> +				GFP_ATOMIC, cmd->tag, flags);
> I think there is no reason to reject TMR via Core, you may use
> your uasp_send_tm_response(cmd) directly like other fabric drivers does.
> That will need some coding to distinguish a completion of the response
> initiated from Core and from fabric driver.

Ok. We can do that.

>> +		return;
>> +	}
>> +
>> +	target_submit_tmr(se_cmd, tv_nexus->tvn_se_sess,
>> +			  cmd->response_iu.add_response_info,
>> +			  cmd->unpacked_lun, NULL, cmd->tmr_func,
>> +			  GFP_ATOMIC, cmd->tag, flags);
>> +}
>> +
>> +static void usbg_submit_cmd(struct usbg_cmd *cmd)
>>   {
>> -	struct usbg_cmd *cmd = container_of(work, struct usbg_cmd, work);
>>   	struct se_cmd *se_cmd;
>>   	struct tcm_usbg_nexus *tv_nexus;
>>   	struct usbg_tpg *tpg;
>> @@ -1073,6 +1225,16 @@ static void usbg_cmd_work(struct work_struct *work)
>>   			TCM_UNSUPPORTED_SCSI_OPCODE, 0);
>>   }
>>   
>> +static void usbg_cmd_work(struct work_struct *work)
>> +{
>> +	struct usbg_cmd *cmd = container_of(work, struct usbg_cmd, work);
>> +
>> +	if (cmd->tmr_func || cmd->tmr_rsp != TMR_RESPONSE_UNKNOWN)
>> +		usbg_submit_tmr(cmd);
> That looks very strange - in response of received SCSI command you will
> send a TMR response???
> I am about cmd->tmr_rsp != TMR_RESPONSE_UNKNOWN case.

Sorry if it's unclear here. The condition here is to submit TMR when we 
have a TASK MANAGEMENT command _or_ we already know that we need to 
submit a tmr_fail_response.

That is, a normal command may need a TMR response such as 
TMR_TASK_DOES_NOT_EXIST or TMR_OVERLAPPED_TAG_ATTEMPTED.

>> +	else
>> +		usbg_submit_cmd(cmd);
>> +}
>> +
>>   static struct usbg_cmd *usbg_get_cmd(struct f_uas *fu,
>>   		struct tcm_usbg_nexus *tv_nexus, u32 scsi_tag)
>>   {
>> @@ -1099,37 +1261,84 @@ static void usbg_release_cmd(struct se_cmd *);
>>   
>>   static int usbg_submit_command(struct f_uas *fu, struct usb_request *req)
>>   {
>> -	struct command_iu *cmd_iu = req->buf;
>> +	struct iu *iu = req->buf;
>>   	struct usbg_cmd *cmd;
>>   	struct usbg_tpg *tpg = fu->tpg;
>>   	struct tcm_usbg_nexus *tv_nexus;
>> +	struct uas_stream *stream;
>> +	struct command_iu *cmd_iu;
>>   	u32 cmd_len;
>>   	u16 scsi_tag;
>>   
>> -	if (cmd_iu->iu_id != IU_ID_COMMAND) {
>> -		pr_err("Unsupported type %d\n", cmd_iu->iu_id);
>> -		return -EINVAL;
>> -	}
>> -
>>   	tv_nexus = tpg->tpg_nexus;
>>   	if (!tv_nexus) {
>>   		pr_err("Missing nexus, ignoring command\n");
>>   		return -EINVAL;
>>   	}
>>   
>> -	cmd_len = (cmd_iu->len & ~0x3) + 16;
>> -	if (cmd_len > USBG_MAX_CMD)
>> -		return -EINVAL;
>> -
>> -	scsi_tag = be16_to_cpup(&cmd_iu->tag);
>> +	scsi_tag = be16_to_cpup(&iu->tag);
>>   	cmd = usbg_get_cmd(fu, tv_nexus, scsi_tag);
>>   	if (IS_ERR(cmd)) {
>>   		pr_err("usbg_get_cmd failed\n");
>>   		return -ENOMEM;
>>   	}
>> -	memcpy(cmd->cmd_buf, cmd_iu->cdb, cmd_len);
>>   
>> -	cmd->stream = &fu->stream[cmd->tag % USBG_NUM_CMDS];
>> +	cmd->req = req;
>> +	cmd->fu = fu;
>> +	cmd->tag = scsi_tag;
>> +	cmd->se_cmd.tag = scsi_tag;
>> +	cmd->tmr_func = 0;
>> +	cmd->tmr_rsp = TMR_RESPONSE_UNKNOWN;
> TMR_* constant are fabric agnostic enum. Better use RC_TMF_* values in
> variables of this driver.

Sure, we can do that.

>> +
>> +	cmd_iu = (struct command_iu *)iu;
>> +
>> +	/* Command and Task Management IUs share the same LUN offset */
>> +	cmd->unpacked_lun = scsilun_to_int(&cmd_iu->lun);
>> +
>> +	if (iu->iu_id != IU_ID_COMMAND && iu->iu_id != IU_ID_TASK_MGMT) {
>> +		cmd->tmr_rsp = TMR_TASK_DOES_NOT_EXIST;
>> +		goto skip;
>> +	}
>> +
>> +	/*
>> +	 * For simplicity, we use mod operation to quickly find an in-progress
>> +	 * matching command tag and respond with overlapped command. The
>> +	 * assumption is that the UASP class driver will limit to using tag id
>> +	 * from 1 to USBG_NUM_CMDS. This is based on observation from the
>> +	 * Windows and Linux UASP storage class driver behavior. If an unusual
>> +	 * UASP class driver uses a tag greater than USBG_NUM_CMDS, then this
>> +	 * method may no longer work due to possible stream id collision. In
>> +	 * that case, we need to use a proper algorithm to fetch the stream (or
>> +	 * simply walk through all active streams to check for overlap).
>> +	 */
>> +	stream = uasp_get_stream_by_tag(fu, scsi_tag);
>> +	if (stream->cmd) {
>> +		WARN_ONCE(stream->cmd->tag != scsi_tag,
> WARN is used to indicate a non fatal bug in the code. May be you want to
> use pr_warn/pr_err here?

Ok. Will update.

Thanks,
Thinh

>> +			  "Command tag %d collided with Stream id %d\n",
>> +			  scsi_tag, stream->cmd->tag);
>> +
>> +		cmd->tmr_rsp = TMR_OVERLAPPED_TAG_ATTEMPTED;
>> +		goto skip;
>> +	}
>> +
>> +	stream->cmd = cmd;
>> +
>> +	if (iu->iu_id == IU_ID_TASK_MGMT) {
>> +		struct task_mgmt_iu *tm_iu;
>> +
>> +		tm_iu = (struct task_mgmt_iu *)iu;
>> +		cmd->tmr_func = uasp_to_tcm_func(tm_iu->function);
>> +		goto skip;
>> +	}
>> +
>> +	cmd_len = (cmd_iu->len & ~0x3) + 16;
>> +	if (cmd_len > USBG_MAX_CMD) {
>> +		pr_err("invalid len %d\n", cmd_len);
>> +		target_free_tag(tv_nexus->tvn_se_sess, &cmd->se_cmd);
>> +		stream->cmd = NULL;
>> +		return -EINVAL;
>> +	}
>> +	memcpy(cmd->cmd_buf, cmd_iu->cdb, cmd_len);
>>   
>>   	switch (cmd_iu->prio_attr & 0x7) {
>>   	case UAS_HEAD_TAG:
>> @@ -1150,9 +1359,7 @@ static int usbg_submit_command(struct f_uas *fu, struct usb_request *req)
>>   		break;
>>   	}
>>   
>> -	cmd->unpacked_lun = scsilun_to_int(&cmd_iu->lun);
>> -	cmd->req = req;
>> -
>> +skip:
>>   	INIT_WORK(&cmd->work, usbg_cmd_work);
>>   	queue_work(tpg->workqueue, &cmd->work);
>>   
>> @@ -1298,13 +1505,16 @@ static int usbg_get_cmd_state(struct se_cmd *se_cmd)
>>   
>>   static void usbg_queue_tm_rsp(struct se_cmd *se_cmd)
>>   {
>> +	struct usbg_cmd *cmd = container_of(se_cmd, struct usbg_cmd, se_cmd);
>> +
>> +	uasp_send_tm_response(cmd);
>>   }
>>   
>>   static void usbg_aborted_task(struct se_cmd *se_cmd)
>>   {
>>   	struct usbg_cmd *cmd = container_of(se_cmd, struct usbg_cmd, se_cmd);
>>   	struct f_uas *fu = cmd->fu;
>> -	struct uas_stream *stream = cmd->stream;
>> +	struct uas_stream *stream = uasp_get_stream_by_tag(fu, cmd->tag);
>>   	int ret = 0;
>>   
>>   	if (stream->req_out->status == -EINPROGRESS)
>> diff --git a/drivers/usb/gadget/function/tcm.h b/drivers/usb/gadget/function/tcm.h
>> index 5157af1b166b..f1cd2399fd69 100644
>> --- a/drivers/usb/gadget/function/tcm.h
>> +++ b/drivers/usb/gadget/function/tcm.h
>> @@ -82,8 +82,11 @@ struct usbg_cmd {
>>   	u16 tag;
>>   	u16 prio_attr;
>>   	struct sense_iu sense_iu;
>> +	struct response_iu response_iu;
>>   	enum uas_state state;
>> -	struct uas_stream *stream;
>> +	int tmr_func;
>> +	int tmr_rsp;
>> +#define	TMR_RESPONSE_UNKNOWN 0xff
>>   
>>   	/* BOT only */
>>   	__le32 bot_tag;
>> @@ -96,6 +99,8 @@ struct uas_stream {
>>   	struct usb_request	*req_in;
>>   	struct usb_request	*req_out;
>>   	struct usb_request	*req_status;
>> +
>> +	struct usbg_cmd		*cmd;
>>   };
>>   
>>   struct usbg_cdb {