Re: [PATCH 18/36] usb: gadget: f_tcm: Don't set static stream_id

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

On 7/8/2022, Dmitry Bogdanov wrote:
> On Wed, Jul 06, 2022 at 04:36:15PM -0700, Thinh Nguyen wrote:
>> Host can assign stream ID value greater than number of streams
>> allocated. The tcm function needs to keep track of which stream is
>> available to assign the stream ID. This patch doesn't track that, but at
>> least it makes sure that there's no Oops if the host send tag with a
>> value greater than the number of supported streams.
>>
>> Signed-off-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com>
>> ---
>>   drivers/usb/gadget/function/f_tcm.c | 32 +++++------------------------
>>   1 file changed, 5 insertions(+), 27 deletions(-)
>>
>> diff --git a/drivers/usb/gadget/function/f_tcm.c b/drivers/usb/gadget/function/f_tcm.c
>> index 270ec631481d..7721216dc9bc 100644
>> --- a/drivers/usb/gadget/function/f_tcm.c
>> +++ b/drivers/usb/gadget/function/f_tcm.c
>> @@ -532,6 +532,7 @@ static int uasp_prepare_r_request(struct usbg_cmd *cmd)
>>   	}
>>   
>>   	stream->req_in->is_last = 1;
>> +	stream->req_in->stream_id = cmd->tag;
>>   	stream->req_in->complete = uasp_status_data_cmpl;
>>   	stream->req_in->length = se_cmd->data_length;
>>   	stream->req_in->context = cmd;
>> @@ -556,6 +557,7 @@ static void uasp_prepare_status(struct usbg_cmd *cmd)
>>   	iu->len = cpu_to_be16(se_cmd->scsi_sense_length);
>>   	iu->status = se_cmd->scsi_status;
>>   	stream->req_status->is_last = 1;
>> +	stream->req_status->stream_id = cmd->tag;
>>   	stream->req_status->context = cmd;
>>   	stream->req_status->length = se_cmd->scsi_sense_length + 16;
>>   	stream->req_status->buf = iu;
>> @@ -786,19 +788,6 @@ static int uasp_alloc_cmd(struct f_uas *fu)
>>   	return -ENOMEM;
>>   }
>>   
>> -static void uasp_setup_stream_res(struct f_uas *fu, int max_streams)
>> -{
>> -	int i;
>> -
>> -	for (i = 0; i < max_streams; i++) {
>> -		struct uas_stream *s = &fu->stream[i];
>> -
>> -		s->req_in->stream_id = i + 1;
>> -		s->req_out->stream_id = i + 1;
>> -		s->req_status->stream_id = i + 1;
>> -	}
>> -}
>> -
>>   static int uasp_prepare_reqs(struct f_uas *fu)
>>   {
>>   	int ret;
>> @@ -819,7 +808,6 @@ static int uasp_prepare_reqs(struct f_uas *fu)
>>   	ret = uasp_alloc_cmd(fu);
>>   	if (ret)
>>   		goto err_free_stream;
>> -	uasp_setup_stream_res(fu, max_streams);
>>   
>>   	ret = usb_ep_queue(fu->ep_cmd, fu->cmd.req, GFP_ATOMIC);
>>   	if (ret)
>> @@ -995,6 +983,7 @@ static int usbg_prepare_w_request(struct usbg_cmd *cmd, struct usb_request *req)
>>   	}
>>   
>>   	req->is_last = 1;
>> +	req->stream_id = cmd->tag;
>>   	req->complete = usbg_data_write_cmpl;
>>   	req->length = se_cmd->data_length;
>>   	req->context = cmd;
>> @@ -1125,16 +1114,8 @@ static int usbg_submit_command(struct f_uas *fu,
>>   	}
>>   	memcpy(cmd->cmd_buf, cmd_iu->cdb, cmd_len);
>>   
>> -	if (fu->flags & USBG_USE_STREAMS) {
>> -		if (cmd->tag > UASP_SS_EP_COMP_NUM_STREAMS)
>> -			goto err;
>> -		if (!cmd->tag)
>> -			cmd->stream = &fu->stream[0];
>> -		else
>> -			cmd->stream = &fu->stream[cmd->tag - 1];
>> -	} else {
>> -		cmd->stream = &fu->stream[0];
>> -	}
>> +	cmd->stream = &fu->stream[cmd->tag %
>> +		UASP_SS_EP_COMP_NUM_STREAMS];
> Use USBG_NUM_CMDS instead of UASP_SS_EP_COMP_NUM_STREAMS like in other
> places.

Sure, I'll do that.

Thanks,
Thinh

>>   
>>   	switch (cmd_iu->prio_attr & 0x7) {
>>   	case UAS_HEAD_TAG:
>> @@ -1161,9 +1142,6 @@ static int usbg_submit_command(struct f_uas *fu,
>>   	queue_work(tpg->workqueue, &cmd->work);
>>   
>>   	return 0;
>> -err:
>> -	usbg_release_cmd(&cmd->se_cmd);
>> -	return -EINVAL;
>>   }
>>   
>>   static void bot_cmd_work(struct work_struct *work)