[PATCH v2 00/25] usb: gadget: f_tcm: Enhance UASP driver

[PATCH v2 00/25] usb: gadget: f_tcm: Enhance UASP driver

[Thinh Nguyen]

The Linux UASP gadget driver is incomplete and remained broken for a long time.
It was not implemented for performance either. This series adds some of the
required features for the UASP driver to work. It also makes some changes to
the target core.

This is tested against UASP CV and DWC_usb3x controller. It still needs some
fixes in the target core, which will be separated from this series.

There are still more room for performance improvement and fixes. However, this
series should be sufficient to bring up a working UASP device.


Changes in v2:
 - Remove most target core changes from this series and only keep the must-have
   ones
 - Split the task-management patch to smaller patches
 - Don't send failure Task Management response to target core, reducing
   dependency
 - Add UASP bringup script example in cover page
 - Make various small updates according to feedbacks


Example script
==============
To test UASP, here's an example perl script snippet to bring it up.

Note: the script was cut down and quickly rewritten, so sorry if I make
mistakes. :)

    my $MY_UAS_VID = xxxx;
    my $MY_UAS_PID = yyyy;
    my $SERIAL = "1234";
    my $VENDOR = "VENDOR";
    my $MY_VER = "VER";

    my $vendor_id = "my_vid";
    my $product_id = "my_pid";
    my $revision = "my_rev";

    # Must update:
    my $backing_storage = "/tmp/some_file";
    my $backing_storage_size = 1024*1024*16;
    my $use_ramdisk = 0;

    my $g = "/sys/kernel/config/usb_gadget/g1";

    system("modprobe libcomposite");
    system("modprobe usb_f_tcm");
    system("mkdir -p $g");
    system("mkdir -p $g/configs/c.1");
    system("mkdir -p $g/functions/tcm.0");
    system("mkdir -p $g/strings/0x409");
    system("mkdir -p $g/configs/c.1/strings/0x409");

    my $tp = "/sys/kernel/config/target/usb_gadget/naa.0/tpgt_1";

    my $tf;
    my $ctrl;

    if ($use_ramdisk) {
        $tf = "/sys/kernel/config/target/core/rd_mcp_0/ramdisk";
        $ctrl = 'rd_pages=524288';
    } else {
        $tf = "/sys/kernel/config/target/core/fileio_0/fileio";
        $ctrl = 'fd_dev_name=$backing_storage,fd_dev_size=$backing_storage_size,fd_async_io=1';
    }

    system("mkdir -p /etc/target");

    system("mkdir -p $tp");
    system("mkdir -p $tf");
    system("mkdir -p $tp/lun/lun_0");

    system("echo naa.0         > $tp/nexus");
    system("echo $ctrl         > $tf/control");
    system("echo 1             > $tf/attrib/emulate_ua_intlck_ctrl");
    system("echo 123           > $tf/wwn/vpd_unit_serial");
    system("echo $vendor_id    > $tf/wwn/vendor_id");
    system("echo $product_id   > $tf/wwn/product_id");
    system("echo $revision     > $tf/wwn/revision");
    system("echo 1             > $tf/enable");

    system("ln -s $tf $tp/lun/lun_0/virtual_scsi_port");
    system("echo 1             > $tp/enable");

    system("echo $MY_UAS_PID   > $g/idProduct");

    system("ln -s $g/functions/tcm.0 $g/configs/c.1");

    system("echo $MY_UAS_VID   > $g/idVendor");
    system("echo $SERIAL       > $g/strings/0x409/serialnumber");
    system("echo $VENDOR       > $g/strings/0x409/manufacturer");
    system("echo \"$MY_VER\"   > $g/strings/0x409/product");
    system("echo \"Conf 1\"    > $g/configs/c.1/strings/0x409/configuration");

    system("echo super-speed-plus > $g/max_speed");

    # Make sure the UDC is available
    system("echo $my_udc       > $g/UDC");


Thinh Nguyen (25):
  target: Add overlapped response to tmrsp_table
  target: Add common TMR enum
  usb: gadget: f_tcm: Increase stream count
  usb: gadget: f_tcm: Increase bMaxBurst
  usb: gadget: f_tcm: Don't set static stream_id
  usb: gadget: f_tcm: Allocate matching number of commands to streams
  usb: gadget: f_tcm: Limit number of sessions
  usb: gadget: f_tcm: Handle multiple commands in parallel
  usb: gadget: f_tcm: Use extra number of commands
  usb: gadget: f_tcm: Return ATA cmd direction
  usb: gadget: f_tcm: Execute command on write completion
  usb: gadget: f_tcm: Minor cleanup redundant code
  usb: gadget: f_tcm: Don't free command immediately
  usb: gadget: f_tcm: Translate error to sense
  usb: gadget: f_tcm: Cleanup unused variable
  usb: gadget: f_tcm: Update state on data write
  usb: gadget: f_tcm: Handle abort command
  usb: gadget: f_tcm: Cleanup requests on ep disable
  usb: gadget: f_tcm: Decrement command ref count on cleanup
  usb: gadget: f_tcm: Save CPU ID per command
  usb: gadget: f_tcm: Get stream by tag
  usb: gadget: f_tcm: Send sense on cancelled transfer
  usb: gadget: f_tcm: Handle TASK_MANAGEMENT commands
  usb: gadget: f_tcm: Check overlapped command
  usb: gadget: f_tcm: Comply with UAS Task Management requirement

 drivers/target/target_core_transport.c |  10 +
 drivers/usb/gadget/function/f_tcm.c    | 536 +++++++++++++++++++------
 drivers/usb/gadget/function/tcm.h      |  20 +-
 include/target/target_core_base.h      |   5 +
 4 files changed, 436 insertions(+), 135 deletions(-)


base-commit: 88a15fbb47db483d06b12b1ae69f114b96361a96
-- 
2.28.0

[PATCH v2 01/25] target: Add overlapped response to tmrsp_table

[Thinh Nguyen]

Add TMR_OVERLAPPED_TAG_ATTEMPTED response to tmrsp_table.

Signed-off-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com>
---
 Changes in v2:
 - None

 include/target/target_core_base.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/include/target/target_core_base.h b/include/target/target_core_base.h
index c2b36f7d917d..8e3da143a1ce 100644
--- a/include/target/target_core_base.h
+++ b/include/target/target_core_base.h
@@ -223,6 +223,7 @@ enum tcm_tmrsp_table {
 	TMR_LUN_DOES_NOT_EXIST		= 3,
 	TMR_TASK_MGMT_FUNCTION_NOT_SUPPORTED	= 4,
 	TMR_FUNCTION_REJECTED		= 5,
+	TMR_OVERLAPPED_TAG_ATTEMPTED	= 6,
 };
 
 /*
-- 
2.28.0

[PATCH v2 02/25] target: Add common TMR enum

[Thinh Nguyen]

Add the following common TMR enum:
* TMR_I_T_NEXUS_RESET
* TMR_QUERY_TASK
* TMR_QUERY_TASK_SET
* TMR_QUERY_ASYNC_EVENT

Signed-off-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com>
---
 Changes in v2:
 - Changed enum numbering to continue from 8, 9, 10, ...

 drivers/target/target_core_transport.c | 10 ++++++++++
 include/target/target_core_base.h      |  4 ++++
 2 files changed, 14 insertions(+)

diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
index 7838dc20f713..92cb4a4a9ab9 100644
--- a/drivers/target/target_core_transport.c
+++ b/drivers/target/target_core_transport.c
@@ -3094,6 +3094,10 @@ static const char *target_tmf_name(enum tcm_tmreq_table tmf)
 	case TMR_TARGET_WARM_RESET:	return "TARGET_WARM_RESET";
 	case TMR_TARGET_COLD_RESET:	return "TARGET_COLD_RESET";
 	case TMR_LUN_RESET_PRO:		return "LUN_RESET_PRO";
+	case TMR_I_T_NEXUS_RESET:	return "I_T_NEXUS_RESET";
+	case TMR_QUERY_TASK:		return "QUERY_TASK";
+	case TMR_QUERY_TASK_SET:	return "QUERY_TASK_SET";
+	case TMR_QUERY_ASYNC_EVENT:	return "QUERY_ASYNC_EVENT";
 	case TMR_UNKNOWN:		break;
 	}
 	return "(?)";
@@ -3542,6 +3546,12 @@ static void target_tmr_work(struct work_struct *work)
 	case TMR_TARGET_COLD_RESET:
 		tmr->response = TMR_FUNCTION_REJECTED;
 		break;
+	case TMR_I_T_NEXUS_RESET:
+	case TMR_QUERY_TASK:
+	case TMR_QUERY_TASK_SET:
+	case TMR_QUERY_ASYNC_EVENT:
+		tmr->response = TMR_FUNCTION_REJECTED;
+		break;
 	default:
 		pr_err("Unknown TMR function: 0x%02x.\n",
 				tmr->function);
diff --git a/include/target/target_core_base.h b/include/target/target_core_base.h
index 8e3da143a1ce..b3e3125fac97 100644
--- a/include/target/target_core_base.h
+++ b/include/target/target_core_base.h
@@ -211,6 +211,10 @@ enum tcm_tmreq_table {
 	TMR_LUN_RESET		= 5,
 	TMR_TARGET_WARM_RESET	= 6,
 	TMR_TARGET_COLD_RESET	= 7,
+	TMR_I_T_NEXUS_RESET	= 8,
+	TMR_QUERY_TASK		= 9,
+	TMR_QUERY_TASK_SET	= 10,
+	TMR_QUERY_ASYNC_EVENT	= 11,
 	TMR_LUN_RESET_PRO	= 0x80,
 	TMR_UNKNOWN		= 0xff,
 };
-- 
2.28.0

[PATCH v2 03/25] usb: gadget: f_tcm: Increase stream count

[Thinh Nguyen]

Some old builds of Microsoft Windows 10 UASP class driver reject USAP
device with stream count of 2^4. To keep compatibility with both Linux
and Windows, let's increase the stream count to 2^5. Also, internal
tests show that stream count of 2^5 increases performance slightly.

Signed-off-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com>
---
 Changes in v2:
 - None

 drivers/usb/gadget/function/tcm.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/usb/gadget/function/tcm.h b/drivers/usb/gadget/function/tcm.h
index 3cd565794ad7..6cb05dcd19ff 100644
--- a/drivers/usb/gadget/function/tcm.h
+++ b/drivers/usb/gadget/function/tcm.h
@@ -13,7 +13,7 @@
 #define USBG_NAMELEN 32
 
 #define fuas_to_gadget(f)	(f->function.config->cdev->gadget)
-#define UASP_SS_EP_COMP_LOG_STREAMS 4
+#define UASP_SS_EP_COMP_LOG_STREAMS 5
 #define UASP_SS_EP_COMP_NUM_STREAMS (1 << UASP_SS_EP_COMP_LOG_STREAMS)
 
 enum {
-- 
2.28.0

Re: [PATCH v2 03/25] usb: gadget: f_tcm: Increase stream count

[Sergei Shtylyov]

Hello!

On 7/19/22 4:26 AM, Thinh Nguyen wrote:

> Some old builds of Microsoft Windows 10 UASP class driver reject USAP

   UASP?

> device with stream count of 2^4. To keep compatibility with both Linux
> and Windows, let's increase the stream count to 2^5. Also, internal
> tests show that stream count of 2^5 increases performance slightly.
> 
> Signed-off-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com>
[...]

MBR, Sergey

[PATCH v2 04/25] usb: gadget: f_tcm: Increase bMaxBurst

[Thinh Nguyen]

Currently the default bMaxBurst is 0. Set default bMaxBurst to 15 (i.e.
16 bursts) to Data IN and OUT endpoints to improve performance. It
should be fine for a controller that supports less than 16 bursts. It
should be able to negotiate properly with the host at packet level for
the end of burst.

If the controller can't handle a burst of 16, and high performance isn't
important, the user can use BOT protocol from mass_storage gadget driver
instead.

Signed-off-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com>
---
 Changes in v2:
 - None

 drivers/usb/gadget/function/f_tcm.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/gadget/function/f_tcm.c b/drivers/usb/gadget/function/f_tcm.c
index 8e17ac831be0..270ec631481d 100644
--- a/drivers/usb/gadget/function/f_tcm.c
+++ b/drivers/usb/gadget/function/f_tcm.c
@@ -1774,7 +1774,7 @@ static struct usb_endpoint_descriptor uasp_ss_bi_desc = {
 static struct usb_ss_ep_comp_descriptor uasp_bi_ep_comp_desc = {
 	.bLength =		sizeof(uasp_bi_ep_comp_desc),
 	.bDescriptorType =	USB_DT_SS_ENDPOINT_COMP,
-	.bMaxBurst =		0,
+	.bMaxBurst =		15,
 	.bmAttributes =		UASP_SS_EP_COMP_LOG_STREAMS,
 	.wBytesPerInterval =	0,
 };
@@ -1782,7 +1782,7 @@ static struct usb_ss_ep_comp_descriptor uasp_bi_ep_comp_desc = {
 static struct usb_ss_ep_comp_descriptor bot_bi_ep_comp_desc = {
 	.bLength =		sizeof(bot_bi_ep_comp_desc),
 	.bDescriptorType =	USB_DT_SS_ENDPOINT_COMP,
-	.bMaxBurst =		0,
+	.bMaxBurst =		15,
 };
 
 static struct usb_endpoint_descriptor uasp_bo_desc = {
@@ -1817,12 +1817,14 @@ static struct usb_endpoint_descriptor uasp_ss_bo_desc = {
 static struct usb_ss_ep_comp_descriptor uasp_bo_ep_comp_desc = {
 	.bLength =		sizeof(uasp_bo_ep_comp_desc),
 	.bDescriptorType =	USB_DT_SS_ENDPOINT_COMP,
+	.bMaxBurst =		15,
 	.bmAttributes =		UASP_SS_EP_COMP_LOG_STREAMS,
 };
 
 static struct usb_ss_ep_comp_descriptor bot_bo_ep_comp_desc = {
 	.bLength =		sizeof(bot_bo_ep_comp_desc),
 	.bDescriptorType =	USB_DT_SS_ENDPOINT_COMP,
+	.bMaxBurst =		15,
 };
 
 static struct usb_endpoint_descriptor uasp_status_desc = {
-- 
2.28.0

[PATCH v2 05/25] usb: gadget: f_tcm: Don't set static stream_id

[Thinh Nguyen]

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>
---
 Changes in v2:
 - None

 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];
 
 	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)
-- 
2.28.0

[PATCH v2 06/25] usb: gadget: f_tcm: Allocate matching number of commands to streams

[Thinh Nguyen]

We can handle multiple commands concurently. Each command services a
stream id. At the moment, the driver will handle 32 outstanding streams,
which is equivalent to 32 commands. Make sure to allocate a matching
number of commands to the number of streams.

Signed-off-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com>
---
 Changes in v2:
 - None

 drivers/usb/gadget/function/f_tcm.c | 110 ++++++++++++++--------------
 drivers/usb/gadget/function/tcm.h   |   8 +-
 2 files changed, 61 insertions(+), 57 deletions(-)

diff --git a/drivers/usb/gadget/function/f_tcm.c b/drivers/usb/gadget/function/f_tcm.c
index 7721216dc9bc..6e0b54985932 100644
--- a/drivers/usb/gadget/function/f_tcm.c
+++ b/drivers/usb/gadget/function/f_tcm.c
@@ -50,7 +50,7 @@ static int bot_enqueue_cmd_cbw(struct f_uas *fu)
 	if (fu->flags & USBG_BOT_CMD_PEND)
 		return 0;
 
-	ret = usb_ep_queue(fu->ep_out, fu->cmd.req, GFP_ATOMIC);
+	ret = usb_ep_queue(fu->ep_out, fu->cmd[0].req, GFP_ATOMIC);
 	if (!ret)
 		fu->flags |= USBG_BOT_CMD_PEND;
 	return ret;
@@ -136,7 +136,7 @@ static void bot_send_bad_status(struct usbg_cmd *cmd)
 		}
 		req->complete = bot_err_compl;
 		req->context = cmd;
-		req->buf = fu->cmd.buf;
+		req->buf = fu->cmd[0].buf;
 		usb_ep_queue(ep, req, GFP_KERNEL);
 	} else {
 		bot_enqueue_sense_code(fu, cmd);
@@ -314,8 +314,8 @@ static int bot_prepare_reqs(struct f_uas *fu)
 	if (!fu->bot_req_out)
 		goto err_out;
 
-	fu->cmd.req = usb_ep_alloc_request(fu->ep_out, GFP_KERNEL);
-	if (!fu->cmd.req)
+	fu->cmd[0].req = usb_ep_alloc_request(fu->ep_out, GFP_KERNEL);
+	if (!fu->cmd[0].req)
 		goto err_cmd;
 
 	fu->bot_status.req = usb_ep_alloc_request(fu->ep_in, GFP_KERNEL);
@@ -327,27 +327,27 @@ static int bot_prepare_reqs(struct f_uas *fu)
 	fu->bot_status.req->complete = bot_status_complete;
 	fu->bot_status.csw.Signature = cpu_to_le32(US_BULK_CS_SIGN);
 
-	fu->cmd.buf = kmalloc(fu->ep_out->maxpacket, GFP_KERNEL);
-	if (!fu->cmd.buf)
+	fu->cmd[0].buf = kmalloc(fu->ep_out->maxpacket, GFP_KERNEL);
+	if (!fu->cmd[0].buf)
 		goto err_buf;
 
-	fu->cmd.req->complete = bot_cmd_complete;
-	fu->cmd.req->buf = fu->cmd.buf;
-	fu->cmd.req->length = fu->ep_out->maxpacket;
-	fu->cmd.req->context = fu;
+	fu->cmd[0].req->complete = bot_cmd_complete;
+	fu->cmd[0].req->buf = fu->cmd[0].buf;
+	fu->cmd[0].req->length = fu->ep_out->maxpacket;
+	fu->cmd[0].req->context = fu;
 
 	ret = bot_enqueue_cmd_cbw(fu);
 	if (ret)
 		goto err_queue;
 	return 0;
 err_queue:
-	kfree(fu->cmd.buf);
-	fu->cmd.buf = NULL;
+	kfree(fu->cmd[0].buf);
+	fu->cmd[0].buf = NULL;
 err_buf:
 	usb_ep_free_request(fu->ep_in, fu->bot_status.req);
 err_sts:
-	usb_ep_free_request(fu->ep_out, fu->cmd.req);
-	fu->cmd.req = NULL;
+	usb_ep_free_request(fu->ep_out, fu->cmd[0].req);
+	fu->cmd[0].req = NULL;
 err_cmd:
 	usb_ep_free_request(fu->ep_out, fu->bot_req_out);
 	fu->bot_req_out = NULL;
@@ -372,16 +372,16 @@ static void bot_cleanup_old_alt(struct f_uas *fu)
 
 	usb_ep_free_request(fu->ep_in, fu->bot_req_in);
 	usb_ep_free_request(fu->ep_out, fu->bot_req_out);
-	usb_ep_free_request(fu->ep_out, fu->cmd.req);
+	usb_ep_free_request(fu->ep_out, fu->cmd[0].req);
 	usb_ep_free_request(fu->ep_in, fu->bot_status.req);
 
-	kfree(fu->cmd.buf);
+	kfree(fu->cmd[0].buf);
 
 	fu->bot_req_in = NULL;
 	fu->bot_req_out = NULL;
-	fu->cmd.req = NULL;
+	fu->cmd[0].req = NULL;
 	fu->bot_status.req = NULL;
-	fu->cmd.buf = NULL;
+	fu->cmd[0].buf = NULL;
 }
 
 static void bot_set_alt(struct f_uas *fu)
@@ -482,10 +482,14 @@ static void uasp_cleanup_one_stream(struct f_uas *fu, struct uas_stream *stream)
 
 static void uasp_free_cmdreq(struct f_uas *fu)
 {
-	usb_ep_free_request(fu->ep_cmd, fu->cmd.req);
-	kfree(fu->cmd.buf);
-	fu->cmd.req = NULL;
-	fu->cmd.buf = NULL;
+	int i;
+
+	for (i = 0; i < USBG_NUM_CMDS; i++) {
+		usb_ep_free_request(fu->ep_cmd, fu->cmd[i].req);
+		kfree(fu->cmd[i].buf);
+		fu->cmd[i].req = NULL;
+		fu->cmd[i].buf = NULL;
+	}
 }
 
 static void uasp_cleanup_old_alt(struct f_uas *fu)
@@ -500,7 +504,7 @@ static void uasp_cleanup_old_alt(struct f_uas *fu)
 	usb_ep_disable(fu->ep_status);
 	usb_ep_disable(fu->ep_cmd);
 
-	for (i = 0; i < UASP_SS_EP_COMP_NUM_STREAMS; i++)
+	for (i = 0; i < USBG_NUM_CMDS; i++)
 		uasp_cleanup_one_stream(fu, &fu->stream[i]);
 	uasp_free_cmdreq(fu);
 }
@@ -603,7 +607,8 @@ static void uasp_status_data_cmpl(struct usb_ep *ep, struct usb_request *req)
 
 	case UASP_QUEUE_COMMAND:
 		transport_generic_free_cmd(&cmd->se_cmd, 0);
-		usb_ep_queue(fu->ep_cmd, fu->cmd.req, GFP_ATOMIC);
+		usb_ep_queue(fu->ep_cmd, cmd->req, GFP_ATOMIC);
+
 		break;
 
 	default:
@@ -718,7 +723,7 @@ static int uasp_send_write_request(struct usbg_cmd *cmd)
 	return ret;
 }
 
-static int usbg_submit_command(struct f_uas *, void *, unsigned int);
+static int usbg_submit_command(struct f_uas *, struct usb_request *);
 
 static void uasp_cmd_complete(struct usb_ep *ep, struct usb_request *req)
 {
@@ -728,7 +733,7 @@ static void uasp_cmd_complete(struct usb_ep *ep, struct usb_request *req)
 	if (req->status < 0)
 		return;
 
-	ret = usbg_submit_command(fu, req->buf, req->actual);
+	ret = usbg_submit_command(fu, req);
 	/*
 	 * Once we tune for performance enqueue the command req here again so
 	 * we can receive a second command while we processing this one. Pay
@@ -737,7 +742,7 @@ static void uasp_cmd_complete(struct usb_ep *ep, struct usb_request *req)
 	 */
 	if (!ret)
 		return;
-	usb_ep_queue(fu->ep_cmd, fu->cmd.req, GFP_ATOMIC);
+	usb_ep_queue(fu->ep_cmd, req, GFP_ATOMIC);
 }
 
 static int uasp_alloc_stream_res(struct f_uas *fu, struct uas_stream *stream)
@@ -766,24 +771,24 @@ static int uasp_alloc_stream_res(struct f_uas *fu, struct uas_stream *stream)
 	return -ENOMEM;
 }
 
-static int uasp_alloc_cmd(struct f_uas *fu)
+static int uasp_alloc_cmd(struct f_uas *fu, int i)
 {
-	fu->cmd.req = usb_ep_alloc_request(fu->ep_cmd, GFP_KERNEL);
-	if (!fu->cmd.req)
+	fu->cmd[i].req = usb_ep_alloc_request(fu->ep_cmd, GFP_KERNEL);
+	if (!fu->cmd[i].req)
 		goto err;
 
-	fu->cmd.buf = kmalloc(fu->ep_cmd->maxpacket, GFP_KERNEL);
-	if (!fu->cmd.buf)
+	fu->cmd[i].buf = kmalloc(fu->ep_cmd->maxpacket, GFP_KERNEL);
+	if (!fu->cmd[i].buf)
 		goto err_buf;
 
-	fu->cmd.req->complete = uasp_cmd_complete;
-	fu->cmd.req->buf = fu->cmd.buf;
-	fu->cmd.req->length = fu->ep_cmd->maxpacket;
-	fu->cmd.req->context = fu;
+	fu->cmd[i].req->complete = uasp_cmd_complete;
+	fu->cmd[i].req->buf = fu->cmd[i].buf;
+	fu->cmd[i].req->length = fu->ep_cmd->maxpacket;
+	fu->cmd[i].req->context = fu;
 	return 0;
 
 err_buf:
-	usb_ep_free_request(fu->ep_cmd, fu->cmd.req);
+	usb_ep_free_request(fu->ep_cmd, fu->cmd[i].req);
 err:
 	return -ENOMEM;
 }
@@ -792,26 +797,22 @@ static int uasp_prepare_reqs(struct f_uas *fu)
 {
 	int ret;
 	int i;
-	int max_streams;
-
-	if (fu->flags & USBG_USE_STREAMS)
-		max_streams = UASP_SS_EP_COMP_NUM_STREAMS;
-	else
-		max_streams = 1;
 
-	for (i = 0; i < max_streams; i++) {
+	for (i = 0; i < USBG_NUM_CMDS; i++) {
 		ret = uasp_alloc_stream_res(fu, &fu->stream[i]);
 		if (ret)
 			goto err_cleanup;
 	}
 
-	ret = uasp_alloc_cmd(fu);
-	if (ret)
-		goto err_free_stream;
+	for (i = 0; i < USBG_NUM_CMDS; i++) {
+		ret = uasp_alloc_cmd(fu, i);
+		if (ret)
+			goto err_free_stream;
 
-	ret = usb_ep_queue(fu->ep_cmd, fu->cmd.req, GFP_ATOMIC);
-	if (ret)
-		goto err_free_stream;
+		ret = usb_ep_queue(fu->ep_cmd, fu->cmd[i].req, GFP_ATOMIC);
+		if (ret)
+			goto err_free_stream;
+	}
 
 	return 0;
 
@@ -1081,10 +1082,9 @@ static struct usbg_cmd *usbg_get_cmd(struct f_uas *fu,
 
 static void usbg_release_cmd(struct se_cmd *);
 
-static int usbg_submit_command(struct f_uas *fu,
-		void *cmdbuf, unsigned int len)
+static int usbg_submit_command(struct f_uas *fu, struct usb_request *req)
 {
-	struct command_iu *cmd_iu = cmdbuf;
+	struct command_iu *cmd_iu = req->buf;
 	struct usbg_cmd *cmd;
 	struct usbg_tpg *tpg = fu->tpg;
 	struct tcm_usbg_nexus *tv_nexus;
@@ -1114,8 +1114,7 @@ static int usbg_submit_command(struct f_uas *fu,
 	}
 	memcpy(cmd->cmd_buf, cmd_iu->cdb, cmd_len);
 
-	cmd->stream = &fu->stream[cmd->tag %
-		UASP_SS_EP_COMP_NUM_STREAMS];
+	cmd->stream = &fu->stream[cmd->tag % USBG_NUM_CMDS];
 
 	switch (cmd_iu->prio_attr & 0x7) {
 	case UAS_HEAD_TAG:
@@ -1137,6 +1136,7 @@ static int usbg_submit_command(struct f_uas *fu,
 	}
 
 	cmd->unpacked_lun = scsilun_to_int(&cmd_iu->lun);
+	cmd->req = req;
 
 	INIT_WORK(&cmd->work, usbg_cmd_work);
 	queue_work(tpg->workqueue, &cmd->work);
diff --git a/drivers/usb/gadget/function/tcm.h b/drivers/usb/gadget/function/tcm.h
index 6cb05dcd19ff..bcbe35bb5015 100644
--- a/drivers/usb/gadget/function/tcm.h
+++ b/drivers/usb/gadget/function/tcm.h
@@ -16,6 +16,8 @@
 #define UASP_SS_EP_COMP_LOG_STREAMS 5
 #define UASP_SS_EP_COMP_NUM_STREAMS (1 << UASP_SS_EP_COMP_LOG_STREAMS)
 
+#define USBG_NUM_CMDS		UASP_SS_EP_COMP_NUM_STREAMS
+
 enum {
 	USB_G_STR_INT_UAS = 0,
 	USB_G_STR_INT_BBB,
@@ -75,6 +77,8 @@ struct usbg_cmd {
 	struct completion write_complete;
 	struct kref ref;
 
+	struct usb_request *req;
+
 	/* UAS only */
 	u16 tag;
 	u16 prio_attr;
@@ -117,14 +121,14 @@ struct f_uas {
 #define USBG_IS_BOT		(1 << 3)
 #define USBG_BOT_CMD_PEND	(1 << 4)
 
-	struct usbg_cdb		cmd;
+	struct usbg_cdb		cmd[USBG_NUM_CMDS];
 	struct usb_ep		*ep_in;
 	struct usb_ep		*ep_out;
 
 	/* UAS */
 	struct usb_ep		*ep_status;
 	struct usb_ep		*ep_cmd;
-	struct uas_stream	stream[UASP_SS_EP_COMP_NUM_STREAMS];
+	struct uas_stream	stream[USBG_NUM_CMDS];
 
 	/* BOT */
 	struct bot_status	bot_status;
-- 
2.28.0

[PATCH v2 07/25] usb: gadget: f_tcm: Limit number of sessions

[Thinh Nguyen]

Only allocate up to USBG_NUM_CMDS number of session tags. We should not
be using more than USBG_NUM_CMDS of tags due to the number of commands
limit we imposed. Each command uses a unique tag. Any more than that is
unnecessary. By limitting it, we can detect an issue in our driver
immediately should we run out of session tags.

Signed-off-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com>
---
 Changes in v2:
 - Add more comments in change log for the change reason.

 drivers/usb/gadget/function/tcm.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/usb/gadget/function/tcm.h b/drivers/usb/gadget/function/tcm.h
index bcbe35bb5015..df768559fb60 100644
--- a/drivers/usb/gadget/function/tcm.h
+++ b/drivers/usb/gadget/function/tcm.h
@@ -26,7 +26,7 @@ enum {
 #define USB_G_ALT_INT_BBB       0
 #define USB_G_ALT_INT_UAS       1
 
-#define USB_G_DEFAULT_SESSION_TAGS	128
+#define USB_G_DEFAULT_SESSION_TAGS	USBG_NUM_CMDS
 
 struct tcm_usbg_nexus {
 	struct se_session *tvn_se_sess;
-- 
2.28.0

[PATCH v2 08/25] usb: gadget: f_tcm: Handle multiple commands in parallel

[Thinh Nguyen]

Resubmit command on completion to fetch more commands and service them
in parallel. Increase the number of work in a queue. Each work will be
for each command allowing them to be processed concurrently. Also, set
them to be unbounded by cpu to improve performance.

Signed-off-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com>
---
 Changes in v2:
 - None

 drivers/usb/gadget/function/f_tcm.c | 20 ++++++++------------
 1 file changed, 8 insertions(+), 12 deletions(-)

diff --git a/drivers/usb/gadget/function/f_tcm.c b/drivers/usb/gadget/function/f_tcm.c
index 6e0b54985932..91d853682468 100644
--- a/drivers/usb/gadget/function/f_tcm.c
+++ b/drivers/usb/gadget/function/f_tcm.c
@@ -728,21 +728,16 @@ static int usbg_submit_command(struct f_uas *, struct usb_request *);
 static void uasp_cmd_complete(struct usb_ep *ep, struct usb_request *req)
 {
 	struct f_uas *fu = req->context;
-	int ret;
 
-	if (req->status < 0)
+	if (req->status == -ESHUTDOWN)
 		return;
 
-	ret = usbg_submit_command(fu, req);
-	/*
-	 * Once we tune for performance enqueue the command req here again so
-	 * we can receive a second command while we processing this one. Pay
-	 * attention to properly sync STAUS endpoint with DATA IN + OUT so you
-	 * don't break HS.
-	 */
-	if (!ret)
+	if (req->status < 0) {
+		usb_ep_queue(fu->ep_cmd, req, GFP_ATOMIC);
 		return;
-	usb_ep_queue(fu->ep_cmd, req, GFP_ATOMIC);
+	}
+
+	usbg_submit_command(fu, req);
 }
 
 static int uasp_alloc_stream_res(struct f_uas *fu, struct uas_stream *stream)
@@ -1357,7 +1352,8 @@ static struct se_portal_group *usbg_make_tpg(struct se_wwn *wwn,
 		goto unref_dep;
 	mutex_init(&tpg->tpg_mutex);
 	atomic_set(&tpg->tpg_port_count, 0);
-	tpg->workqueue = alloc_workqueue("tcm_usb_gadget", 0, 1);
+	tpg->workqueue = alloc_workqueue("tcm_usb_gadget",
+					 WQ_UNBOUND, WQ_UNBOUND_MAX_ACTIVE);
 	if (!tpg->workqueue)
 		goto free_tpg;
 
-- 
2.28.0

[PATCH v2 09/25] usb: gadget: f_tcm: Use extra number of commands

[Thinh Nguyen]

To properly respond to host sending more commands than the number of
streams the device advertises, the device needs to be able to reject the
command with a response. Allocate an extra request to handle 1 more
command than the number of streams.

Signed-off-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com>
---
 Changes in v2:
 - None

 drivers/usb/gadget/function/tcm.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/usb/gadget/function/tcm.h b/drivers/usb/gadget/function/tcm.h
index df768559fb60..c7e6d36afd3a 100644
--- a/drivers/usb/gadget/function/tcm.h
+++ b/drivers/usb/gadget/function/tcm.h
@@ -16,7 +16,7 @@
 #define UASP_SS_EP_COMP_LOG_STREAMS 5
 #define UASP_SS_EP_COMP_NUM_STREAMS (1 << UASP_SS_EP_COMP_LOG_STREAMS)
 
-#define USBG_NUM_CMDS		UASP_SS_EP_COMP_NUM_STREAMS
+#define USBG_NUM_CMDS		(UASP_SS_EP_COMP_NUM_STREAMS + 1)
 
 enum {
 	USB_G_STR_INT_UAS = 0,
-- 
2.28.0

[PATCH v2 10/25] usb: gadget: f_tcm: Return ATA cmd direction

[Thinh Nguyen]

Check ATA Pass-Through for direction.

Signed-off-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com>
---
 Changes in v2:
 - None

 drivers/usb/gadget/function/f_tcm.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/usb/gadget/function/f_tcm.c b/drivers/usb/gadget/function/f_tcm.c
index 91d853682468..6fea80afe2d7 100644
--- a/drivers/usb/gadget/function/f_tcm.c
+++ b/drivers/usb/gadget/function/f_tcm.c
@@ -898,6 +898,8 @@ static int get_cmd_dir(const unsigned char *cdb)
 	case READ_TOC:
 	case READ_FORMAT_CAPACITIES:
 	case REQUEST_SENSE:
+	case ATA_12:
+	case ATA_16:
 		ret = DMA_FROM_DEVICE;
 		break;
 
-- 
2.28.0

[PATCH v2 11/25] usb: gadget: f_tcm: Execute command on write completion

[Thinh Nguyen]

Don't just wait for the data write completion and execute the target
command. We need to verify if the request completed successfully and not
just sending invalid data. The verification is done in the write request
completion routine, so we can just run target_execute_cmd() there. The
wait for the data write is not needed.

Signed-off-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com>
---
 Changes in v2:
 - None

 drivers/usb/gadget/function/f_tcm.c | 8 +-------
 drivers/usb/gadget/function/tcm.h   | 1 -
 2 files changed, 1 insertion(+), 8 deletions(-)

diff --git a/drivers/usb/gadget/function/f_tcm.c b/drivers/usb/gadget/function/f_tcm.c
index 6fea80afe2d7..ec83f2f9a858 100644
--- a/drivers/usb/gadget/function/f_tcm.c
+++ b/drivers/usb/gadget/function/f_tcm.c
@@ -248,7 +248,6 @@ static int bot_send_write_request(struct usbg_cmd *cmd)
 	struct usb_gadget *gadget = fuas_to_gadget(fu);
 	int ret;
 
-	init_completion(&cmd->write_complete);
 	cmd->fu = fu;
 
 	if (!cmd->data_len) {
@@ -279,8 +278,6 @@ static int bot_send_write_request(struct usbg_cmd *cmd)
 	if (ret)
 		pr_err("%s(%d)\n", __func__, __LINE__);
 
-	wait_for_completion(&cmd->write_complete);
-	target_execute_cmd(se_cmd);
 cleanup:
 	return ret;
 }
@@ -685,7 +682,6 @@ static int uasp_send_write_request(struct usbg_cmd *cmd)
 	struct sense_iu *iu = &cmd->sense_iu;
 	int ret;
 
-	init_completion(&cmd->write_complete);
 	cmd->fu = fu;
 
 	iu->tag = cpu_to_be16(cmd->tag);
@@ -717,8 +713,6 @@ static int uasp_send_write_request(struct usbg_cmd *cmd)
 			pr_err("%s(%d)\n", __func__, __LINE__);
 	}
 
-	wait_for_completion(&cmd->write_complete);
-	target_execute_cmd(se_cmd);
 cleanup:
 	return ret;
 }
@@ -955,7 +949,7 @@ static void usbg_data_write_cmpl(struct usb_ep *ep, struct usb_request *req)
 				se_cmd->data_length);
 	}
 
-	complete(&cmd->write_complete);
+	target_execute_cmd(se_cmd);
 	return;
 
 cleanup:
diff --git a/drivers/usb/gadget/function/tcm.h b/drivers/usb/gadget/function/tcm.h
index c7e6d36afd3a..5157af1b166b 100644
--- a/drivers/usb/gadget/function/tcm.h
+++ b/drivers/usb/gadget/function/tcm.h
@@ -74,7 +74,6 @@ struct usbg_cmd {
 	struct se_cmd se_cmd;
 	void *data_buf; /* used if no sg support available */
 	struct f_uas *fu;
-	struct completion write_complete;
 	struct kref ref;
 
 	struct usb_request *req;
-- 
2.28.0

Re: [PATCH v2 11/25] usb: gadget: f_tcm: Execute command on write completion

[Sebastian Andrzej Siewior]

On 2022-07-18 18:27:12 [-0700], Thinh Nguyen wrote:
> index 6fea80afe2d7..ec83f2f9a858 100644
> --- a/drivers/usb/gadget/function/f_tcm.c
> +++ b/drivers/usb/gadget/function/f_tcm.c
> @@ -955,7 +949,7 @@ static void usbg_data_write_cmpl(struct usb_ep *ep, struct usb_request *req)
>  				se_cmd->data_length);
>  	}
>  
> -	complete(&cmd->write_complete);
> +	target_execute_cmd(se_cmd);

usbg_data_write_cmpl() is invoked from interrupt service routing which
may run with disabled interrupts. From looking at target_execute_cmd():
| void target_execute_cmd(struct se_cmd *cmd)
| {
…
|         spin_lock_irq(&cmd->t_state_lock);
…
|         spin_unlock_irq(&cmd->t_state_lock);
…
| }

which means interrupts will remain open after leaving
target_execute_cmd(). Now, why didn't the WARN_ONCE() in
__handle_irq_event_percpu() trigger? Am I missing something?

>  	return;

Sebastian

Re: [PATCH v2 11/25] usb: gadget: f_tcm: Execute command on write completion

[Thinh Nguyen]

On Fri, Aug 26, 2022, Sebastian Andrzej Siewior wrote:
> On 2022-07-18 18:27:12 [-0700], Thinh Nguyen wrote:
> > index 6fea80afe2d7..ec83f2f9a858 100644
> > --- a/drivers/usb/gadget/function/f_tcm.c
> > +++ b/drivers/usb/gadget/function/f_tcm.c
> > @@ -955,7 +949,7 @@ static void usbg_data_write_cmpl(struct usb_ep *ep, struct usb_request *req)
> >  				se_cmd->data_length);
> >  	}
> >  
> > -	complete(&cmd->write_complete);
> > +	target_execute_cmd(se_cmd);
> 
> usbg_data_write_cmpl() is invoked from interrupt service routing which
> may run with disabled interrupts. From looking at target_execute_cmd():

It will always be called with interrupts disabled as documented in
usb_request API.

> | void target_execute_cmd(struct se_cmd *cmd)
> | {
> …
> |         spin_lock_irq(&cmd->t_state_lock);
> …
> |         spin_unlock_irq(&cmd->t_state_lock);
> …
> | }
> 
> which means interrupts will remain open after leaving
> target_execute_cmd(). Now, why didn't the WARN_ONCE() in
> __handle_irq_event_percpu() trigger? Am I missing something?
> 
> >  	return;
> 

Since target_execute_cmd() is called in usbg_data_write_cmpl(),
interrupts are still disabled.

Thanks,
Thinh

Re: [PATCH v2 11/25] usb: gadget: f_tcm: Execute command on write completion

[Sebastian Andrzej Siewior]

On 2022-08-26 18:37:36 [+0000], Thinh Nguyen wrote:
> On Fri, Aug 26, 2022, Sebastian Andrzej Siewior wrote:
> > On 2022-07-18 18:27:12 [-0700], Thinh Nguyen wrote:
> > > index 6fea80afe2d7..ec83f2f9a858 100644
> > > --- a/drivers/usb/gadget/function/f_tcm.c
> > > +++ b/drivers/usb/gadget/function/f_tcm.c
> > > @@ -955,7 +949,7 @@ static void usbg_data_write_cmpl(struct usb_ep *ep, struct usb_request *req)
> > >  				se_cmd->data_length);
> > >  	}
> > >  
> > > -	complete(&cmd->write_complete);
> > > +	target_execute_cmd(se_cmd);
> > 
> > usbg_data_write_cmpl() is invoked from interrupt service routing which
> > may run with disabled interrupts. From looking at target_execute_cmd():
> 
> It will always be called with interrupts disabled as documented in
> usb_request API.
> 
> > | void target_execute_cmd(struct se_cmd *cmd)
> > | {
> > …
> > |         spin_lock_irq(&cmd->t_state_lock);
> > …
> > |         spin_unlock_irq(&cmd->t_state_lock);
> > …
> > | }
> > 
> > which means interrupts will remain open after leaving
> > target_execute_cmd(). Now, why didn't the WARN_ONCE() in
> > __handle_irq_event_percpu() trigger? Am I missing something?
> > 
> > >  	return;
> > 
> 
> Since target_execute_cmd() is called in usbg_data_write_cmpl(),
> interrupts are still disabled.

but you do realize that target_execute_cmd() will leave with enabled
interrupts and this is not desired? I _think_ this was the reason why I
ended up with the wait+complete construct instead of invoking this
function directly.
An _irqsave() in target_execute_cmd() would probably be all you need
here.

> Thanks,
> Thinh

Sebastian

Re: [PATCH v2 11/25] usb: gadget: f_tcm: Execute command on write completion

[Thinh Nguyen]

On Mon, Aug 29, 2022, Sebastian Andrzej Siewior wrote:
> On 2022-08-26 18:37:36 [+0000], Thinh Nguyen wrote:
> > On Fri, Aug 26, 2022, Sebastian Andrzej Siewior wrote:
> > > On 2022-07-18 18:27:12 [-0700], Thinh Nguyen wrote:
> > > > index 6fea80afe2d7..ec83f2f9a858 100644
> > > > --- a/drivers/usb/gadget/function/f_tcm.c
> > > > +++ b/drivers/usb/gadget/function/f_tcm.c
> > > > @@ -955,7 +949,7 @@ static void usbg_data_write_cmpl(struct usb_ep *ep, struct usb_request *req)
> > > >  				se_cmd->data_length);
> > > >  	}
> > > >  
> > > > -	complete(&cmd->write_complete);
> > > > +	target_execute_cmd(se_cmd);
> > > 
> > > usbg_data_write_cmpl() is invoked from interrupt service routing which
> > > may run with disabled interrupts. From looking at target_execute_cmd():
> > 
> > It will always be called with interrupts disabled as documented in
> > usb_request API.
> > 
> > > | void target_execute_cmd(struct se_cmd *cmd)
> > > | {
> > > …
> > > |         spin_lock_irq(&cmd->t_state_lock);
> > > …
> > > |         spin_unlock_irq(&cmd->t_state_lock);
> > > …
> > > | }
> > > 
> > > which means interrupts will remain open after leaving
> > > target_execute_cmd(). Now, why didn't the WARN_ONCE() in
> > > __handle_irq_event_percpu() trigger? Am I missing something?
> > > 
> > > >  	return;
> > > 
> > 
> > Since target_execute_cmd() is called in usbg_data_write_cmpl(),
> > interrupts are still disabled.
> 
> but you do realize that target_execute_cmd() will leave with enabled
> interrupts and this is not desired? I _think_ this was the reason why I
> ended up with the wait+complete construct instead of invoking this
> function directly.
> An _irqsave() in target_execute_cmd() would probably be all you need
> here.
> 

Ok. Maybe we should make a change in the target_execute_cmd() then. It
seems unreasonable to force the caller to workaround this such as the
wait+complete construct you did (and I don't recall we have changes in
place to know/guarantee that interrupts are enabled before executing
target_execute_cmd() previously either).

For the dwc3, we masked the interrupt at this point, so interrupt won't
be asserted here.

Thanks,
Thinh

Re: [PATCH v2 11/25] usb: gadget: f_tcm: Execute command on write completion

[Sebastian Andrzej Siewior]

On 2022-08-29 21:47:41 [+0000], Thinh Nguyen wrote:
> Ok. Maybe we should make a change in the target_execute_cmd() then. It
> seems unreasonable to force the caller to workaround this such as the
> wait+complete construct you did (and I don't recall we have changes in
> place to know/guarantee that interrupts are enabled before executing
> target_execute_cmd() previously either).

Sounds reasonable. Back then I wasn't sure if I'm putting all the puzzle
pieces correctly together so I preferred this over a target change I
wasn't sure was really needed. Anyway.

> For the dwc3, we masked the interrupt at this point, so interrupt won't
> be asserted here.

dwc3 has a irqrestore() after calling the routine so that will avoid the
splat. But lockdep should yell here.
Anyway, other interrupts on that CPU (timer for instance) could trigger.

> Thanks,
> Thinh

Sebastian

[PATCH v2 12/25] usb: gadget: f_tcm: Minor cleanup redundant code

[Thinh Nguyen]

The status request preparation is done in uasp_prepare_status(). Remove
duplicate code. No functional change here.

Signed-off-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com>
---
 Changes in v2:
 - None

 drivers/usb/gadget/function/f_tcm.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/usb/gadget/function/f_tcm.c b/drivers/usb/gadget/function/f_tcm.c
index ec83f2f9a858..53b178ad4f90 100644
--- a/drivers/usb/gadget/function/f_tcm.c
+++ b/drivers/usb/gadget/function/f_tcm.c
@@ -624,8 +624,6 @@ static int uasp_send_status_response(struct usbg_cmd *cmd)
 	struct sense_iu *iu = &cmd->sense_iu;
 
 	iu->tag = cpu_to_be16(cmd->tag);
-	stream->req_status->complete = uasp_status_data_cmpl;
-	stream->req_status->context = cmd;
 	cmd->fu = fu;
 	uasp_prepare_status(cmd);
 	return usb_ep_queue(fu->ep_status, stream->req_status, GFP_ATOMIC);
-- 
2.28.0

[PATCH v2 13/25] usb: gadget: f_tcm: Don't free command immediately

[Thinh Nguyen]

Don't prematurely free the command. Wait for the status completion of
the sense status. It can be freed then. Otherwise we will double-free
the command.

Fixes: cff834c16d23 ("usb-gadget/tcm: Convert to TARGET_SCF_ACK_KREF I/O krefs")
Signed-off-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com>
---
 Changes in v2:
 - None

 drivers/usb/gadget/function/f_tcm.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/usb/gadget/function/f_tcm.c b/drivers/usb/gadget/function/f_tcm.c
index 53b178ad4f90..cace5746c0f9 100644
--- a/drivers/usb/gadget/function/f_tcm.c
+++ b/drivers/usb/gadget/function/f_tcm.c
@@ -1045,7 +1045,6 @@ static void usbg_cmd_work(struct work_struct *work)
 out:
 	transport_send_check_condition_and_sense(se_cmd,
 			TCM_UNSUPPORTED_SCSI_OPCODE, 1);
-	transport_generic_free_cmd(&cmd->se_cmd, 0);
 }
 
 static struct usbg_cmd *usbg_get_cmd(struct f_uas *fu,
-- 
2.28.0

[PATCH v2 14/25] usb: gadget: f_tcm: Translate error to sense

[Thinh Nguyen]

When respond with check_condition error status, clear from_transport
input so the target layer can translate the sense reason reported by
f_tcm.

Fixes: c52661d60f63 ("usb-gadget: Initial merge of target module for UASP + BOT")
Signed-off-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com>
---
 Changes in v2:
 - None

 drivers/usb/gadget/function/f_tcm.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/gadget/function/f_tcm.c b/drivers/usb/gadget/function/f_tcm.c
index cace5746c0f9..28b560ab44fd 100644
--- a/drivers/usb/gadget/function/f_tcm.c
+++ b/drivers/usb/gadget/function/f_tcm.c
@@ -1044,7 +1044,7 @@ static void usbg_cmd_work(struct work_struct *work)
 
 out:
 	transport_send_check_condition_and_sense(se_cmd,
-			TCM_UNSUPPORTED_SCSI_OPCODE, 1);
+			TCM_UNSUPPORTED_SCSI_OPCODE, 0);
 }
 
 static struct usbg_cmd *usbg_get_cmd(struct f_uas *fu,
@@ -1160,7 +1160,7 @@ static void bot_cmd_work(struct work_struct *work)
 
 out:
 	transport_send_check_condition_and_sense(se_cmd,
-				TCM_UNSUPPORTED_SCSI_OPCODE, 1);
+				TCM_UNSUPPORTED_SCSI_OPCODE, 0);
 	transport_generic_free_cmd(&cmd->se_cmd, 0);
 }
 
-- 
2.28.0

[PATCH v2 15/25] usb: gadget: f_tcm: Cleanup unused variable

[Thinh Nguyen]

se_cmd is not used anywhere. Remove it.

Signed-off-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com>
---
 Changes in v2:
 - None

 drivers/usb/gadget/function/f_tcm.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/usb/gadget/function/f_tcm.c b/drivers/usb/gadget/function/f_tcm.c
index 28b560ab44fd..1e7d29f8aecb 100644
--- a/drivers/usb/gadget/function/f_tcm.c
+++ b/drivers/usb/gadget/function/f_tcm.c
@@ -675,7 +675,6 @@ 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 se_cmd *se_cmd = &cmd->se_cmd;
 	struct uas_stream *stream = cmd->stream;
 	struct sense_iu *iu = &cmd->sense_iu;
 	int ret;
-- 
2.28.0

Re: [PATCH v2 15/25] usb: gadget: f_tcm: Cleanup unused variable

[Sebastian Andrzej Siewior]

On 2022-07-18 18:27:39 [-0700], Thinh Nguyen wrote:
> se_cmd is not used anywhere. Remove it.

This should folded into the original commit where se_cmd has stopped
been using. I believe it was when target_execute_cmd() has been moved.

> Signed-off-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com>

Sebastian

Re: [PATCH v2 15/25] usb: gadget: f_tcm: Cleanup unused variable

[Thinh Nguyen]

On Fri, Aug 26, 2022, Sebastian Andrzej Siewior wrote:
> On 2022-07-18 18:27:39 [-0700], Thinh Nguyen wrote:
> > se_cmd is not used anywhere. Remove it.
> 
> This should folded into the original commit where se_cmd has stopped
> been using. I believe it was when target_execute_cmd() has been moved.

Right, I missed that.

Thanks,
Thinh

> 
> > Signed-off-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com>
> 
> Sebastian

[PATCH v2 16/25] usb: gadget: f_tcm: Update state on data write

[Thinh Nguyen]

When preparing request for data write state, the next state is
UASP_SEND_STATUS. When data write completes, the next state is
UASP_QUEUE_COMMAND. Without this change, the command will transition to
the wrong state.

Signed-off-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com>
---
 Changes in v2:
 - Move a related change from TASK MANAGEMENT updating cmd state to
   UASP_QUEUE_COMMAND to here.

 drivers/usb/gadget/function/f_tcm.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/usb/gadget/function/f_tcm.c b/drivers/usb/gadget/function/f_tcm.c
index 1e7d29f8aecb..d7318c84af98 100644
--- a/drivers/usb/gadget/function/f_tcm.c
+++ b/drivers/usb/gadget/function/f_tcm.c
@@ -934,6 +934,8 @@ 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;
 
+	cmd->state = UASP_QUEUE_COMMAND;
+
 	if (req->status < 0) {
 		pr_err("%s() state %d transfer failed\n", __func__, cmd->state);
 		goto cleanup;
@@ -976,6 +978,8 @@ static int usbg_prepare_w_request(struct usbg_cmd *cmd, struct usb_request *req)
 	req->complete = usbg_data_write_cmpl;
 	req->length = se_cmd->data_length;
 	req->context = cmd;
+
+	cmd->state = UASP_SEND_STATUS;
 	return 0;
 }
 
-- 
2.28.0

Re: [PATCH v2 16/25] usb: gadget: f_tcm: Update state on data write

[Sebastian Andrzej Siewior]

On 2022-07-18 18:27:45 [-0700], Thinh Nguyen wrote:
> When preparing request for data write state, the next state is
> UASP_SEND_STATUS. When data write completes, the next state is
> UASP_QUEUE_COMMAND. Without this change, the command will transition to
> the wrong state.

Why is this needed now, what is the outcome of not having it?
My point is, was this always broken, worked by chance and broke over
time while code was changed?

> Signed-off-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com>
> ---
>  Changes in v2:
>  - Move a related change from TASK MANAGEMENT updating cmd state to
>    UASP_QUEUE_COMMAND to here.
> 
>  drivers/usb/gadget/function/f_tcm.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/usb/gadget/function/f_tcm.c b/drivers/usb/gadget/function/f_tcm.c
> index 1e7d29f8aecb..d7318c84af98 100644
> --- a/drivers/usb/gadget/function/f_tcm.c
> +++ b/drivers/usb/gadget/function/f_tcm.c
> @@ -934,6 +934,8 @@ 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;
>  
> +	cmd->state = UASP_QUEUE_COMMAND;
> +
>  	if (req->status < 0) {
>  		pr_err("%s() state %d transfer failed\n", __func__, cmd->state);
>  		goto cleanup;
> @@ -976,6 +978,8 @@ static int usbg_prepare_w_request(struct usbg_cmd *cmd, struct usb_request *req)
>  	req->complete = usbg_data_write_cmpl;
>  	req->length = se_cmd->data_length;
>  	req->context = cmd;
> +
> +	cmd->state = UASP_SEND_STATUS;
>  	return 0;
>  }
>  

Sebastian

Re: [PATCH v2 16/25] usb: gadget: f_tcm: Update state on data write

[Thinh Nguyen]

On Fri, Aug 26, 2022, Sebastian Andrzej Siewior wrote:
> On 2022-07-18 18:27:45 [-0700], Thinh Nguyen wrote:
> > When preparing request for data write state, the next state is
> > UASP_SEND_STATUS. When data write completes, the next state is
> > UASP_QUEUE_COMMAND. Without this change, the command will transition to
> > the wrong state.
> 
> Why is this needed now, what is the outcome of not having it?
> My point is, was this always broken, worked by chance and broke over
> time while code was changed?

This patch should've been part of the patch "usb: gadget: f_tcm: Execute
command on write completion", and maybe I should've been more clear in
the commmit message.

Previously, the f_tcm doesn't take care of the case where the queued
data could fail/aborted. It attempts to execute the command anyway.

This change here takes care of both scenarios. If the request fails, the
next state is to send status, and if the request completes successfully,
the next state is to queue a new command.

Thanks,
Thinh


> 
> > Signed-off-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com>
> > ---
> >  Changes in v2:
> >  - Move a related change from TASK MANAGEMENT updating cmd state to
> >    UASP_QUEUE_COMMAND to here.
> > 
> >  drivers/usb/gadget/function/f_tcm.c | 4 ++++
> >  1 file changed, 4 insertions(+)
> > 
> > diff --git a/drivers/usb/gadget/function/f_tcm.c b/drivers/usb/gadget/function/f_tcm.c
> > index 1e7d29f8aecb..d7318c84af98 100644
> > --- a/drivers/usb/gadget/function/f_tcm.c
> > +++ b/drivers/usb/gadget/function/f_tcm.c
> > @@ -934,6 +934,8 @@ 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;
> >  
> > +	cmd->state = UASP_QUEUE_COMMAND;
> > +
> >  	if (req->status < 0) {
> >  		pr_err("%s() state %d transfer failed\n", __func__, cmd->state);
> >  		goto cleanup;
> > @@ -976,6 +978,8 @@ static int usbg_prepare_w_request(struct usbg_cmd *cmd, struct usb_request *req)
> >  	req->complete = usbg_data_write_cmpl;
> >  	req->length = se_cmd->data_length;
> >  	req->context = cmd;
> > +
> > +	cmd->state = UASP_SEND_STATUS;
> >  	return 0;
> >  }
> >  
> 
> Sebastian

[PATCH v2 17/25] usb: gadget: f_tcm: Handle abort command

[Thinh Nguyen]

Implemented aborted_task to cancel outstanding request.

Signed-off-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com>
---
 Changes in v2:
 - None

 drivers/usb/gadget/function/f_tcm.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/drivers/usb/gadget/function/f_tcm.c b/drivers/usb/gadget/function/f_tcm.c
index d7318c84af98..8f7eb7045e64 100644
--- a/drivers/usb/gadget/function/f_tcm.c
+++ b/drivers/usb/gadget/function/f_tcm.c
@@ -1278,6 +1278,22 @@ static void usbg_queue_tm_rsp(struct se_cmd *se_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;
+	int ret = 0;
+
+	if (stream->req_out->status == -EINPROGRESS)
+		ret = usb_ep_dequeue(fu->ep_out, stream->req_out);
+	else if (stream->req_in->status == -EINPROGRESS)
+		ret = usb_ep_dequeue(fu->ep_in, stream->req_in);
+	else if (stream->req_status->status == -EINPROGRESS)
+		ret = usb_ep_dequeue(fu->ep_status, stream->req_status);
+
+	if (ret)
+		pr_err("Unable to dequeue se_cmd out %p\n", se_cmd);
+
+	cmd->state = UASP_QUEUE_COMMAND;
 }
 
 static const char *usbg_check_wwn(const char *name)
-- 
2.28.0

Re: [PATCH v2 17/25] usb: gadget: f_tcm: Handle abort command

[Sebastian Andrzej Siewior]

On 2022-07-18 18:27:51 [-0700], Thinh Nguyen wrote:
> diff --git a/drivers/usb/gadget/function/f_tcm.c b/drivers/usb/gadget/function/f_tcm.c
> --- a/drivers/usb/gadget/function/f_tcm.c
> +++ b/drivers/usb/gadget/function/f_tcm.c
> @@ -1278,6 +1278,22 @@ static void usbg_queue_tm_rsp(struct se_cmd *se_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;
> +	int ret = 0;
> +
> +	if (stream->req_out->status == -EINPROGRESS)
> +		ret = usb_ep_dequeue(fu->ep_out, stream->req_out);
> +	else if (stream->req_in->status == -EINPROGRESS)
> +		ret = usb_ep_dequeue(fu->ep_in, stream->req_in);
> +	else if (stream->req_status->status == -EINPROGRESS)
> +		ret = usb_ep_dequeue(fu->ep_status, stream->req_status);
> +
> +	if (ret)
> +		pr_err("Unable to dequeue se_cmd out %p\n", se_cmd);

I know I wasn't the best example here. But if you continue to work on
that one, if would be nice if you could replace all the pr_err() here
with a dev_err() (or so) so it is bound to an actual device and the
reader can actually pin point the message to the actually device/
subsystem. I'm not sure if we lack a device or I was extremely lazy.
Again, not something you need change now.

> +	cmd->state = UASP_QUEUE_COMMAND;
>  }
>  
>  static const char *usbg_check_wwn(const char *name)

Sebastian

Re: [PATCH v2 17/25] usb: gadget: f_tcm: Handle abort command

[Thinh Nguyen]

On Fri, Aug 26, 2022, Sebastian Andrzej Siewior wrote:
> On 2022-07-18 18:27:51 [-0700], Thinh Nguyen wrote:
> > diff --git a/drivers/usb/gadget/function/f_tcm.c b/drivers/usb/gadget/function/f_tcm.c
> > --- a/drivers/usb/gadget/function/f_tcm.c
> > +++ b/drivers/usb/gadget/function/f_tcm.c
> > @@ -1278,6 +1278,22 @@ static void usbg_queue_tm_rsp(struct se_cmd *se_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;
> > +	int ret = 0;
> > +
> > +	if (stream->req_out->status == -EINPROGRESS)
> > +		ret = usb_ep_dequeue(fu->ep_out, stream->req_out);
> > +	else if (stream->req_in->status == -EINPROGRESS)
> > +		ret = usb_ep_dequeue(fu->ep_in, stream->req_in);
> > +	else if (stream->req_status->status == -EINPROGRESS)
> > +		ret = usb_ep_dequeue(fu->ep_status, stream->req_status);
> > +
> > +	if (ret)
> > +		pr_err("Unable to dequeue se_cmd out %p\n", se_cmd);
> 
> I know I wasn't the best example here. But if you continue to work on
> that one, if would be nice if you could replace all the pr_err() here
> with a dev_err() (or so) so it is bound to an actual device and the
> reader can actually pin point the message to the actually device/
> subsystem. I'm not sure if we lack a device or I was extremely lazy.
> Again, not something you need change now.

Yes. I noticed that too. However, I think that can be done later as it
doesn't seem critical. I tried to keep the code consistent for now.

Thanks,
Thinh

> 
> > +	cmd->state = UASP_QUEUE_COMMAND;
> >  }
> >  
> >  static const char *usbg_check_wwn(const char *name)
> 
> Sebastian

[PATCH v2 18/25] usb: gadget: f_tcm: Cleanup requests on ep disable

[Thinh Nguyen]

If the endpoint is disabled, then free the command. Normally we don't
free a command until it's completed its UASP status (failure or not).

Signed-off-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com>
---
 Changes in v2:
 - Use goto cleanup for a cleaner look.

 drivers/usb/gadget/function/f_tcm.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/gadget/function/f_tcm.c b/drivers/usb/gadget/function/f_tcm.c
index 8f7eb7045e64..a8a730e5126d 100644
--- a/drivers/usb/gadget/function/f_tcm.c
+++ b/drivers/usb/gadget/function/f_tcm.c
@@ -572,7 +572,7 @@ static void uasp_status_data_cmpl(struct usb_ep *ep, struct usb_request *req)
 	struct f_uas *fu = cmd->fu;
 	int ret;
 
-	if (req->status < 0)
+	if (req->status == -ESHUTDOWN)
 		goto cleanup;
 
 	switch (cmd->state) {
@@ -936,7 +936,10 @@ static void usbg_data_write_cmpl(struct usb_ep *ep, struct usb_request *req)
 
 	cmd->state = UASP_QUEUE_COMMAND;
 
-	if (req->status < 0) {
+	if (req->status == -ESHUTDOWN)
+		goto cleanup;
+
+	if (req->status) {
 		pr_err("%s() state %d transfer failed\n", __func__, cmd->state);
 		goto cleanup;
 	}
-- 
2.28.0

[PATCH v2 19/25] usb: gadget: f_tcm: Decrement command ref count on cleanup

[Thinh Nguyen]

We submitted the command with TARGET_SCF_ACK_KREF, which requires
acknowledgment of command completion. If the command fails, make sure to
decrement the ref count.

Fixes: cff834c16d23 ("usb-gadget/tcm: Convert to TARGET_SCF_ACK_KREF I/O krefs")
Signed-off-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com>
---
 Changes in v2:
 - New patch

 drivers/usb/gadget/function/f_tcm.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/usb/gadget/function/f_tcm.c b/drivers/usb/gadget/function/f_tcm.c
index a8a730e5126d..6cf95c9723a3 100644
--- a/drivers/usb/gadget/function/f_tcm.c
+++ b/drivers/usb/gadget/function/f_tcm.c
@@ -955,6 +955,7 @@ static void usbg_data_write_cmpl(struct usb_ep *ep, struct usb_request *req)
 	return;
 
 cleanup:
+	target_put_sess_cmd(se_cmd);
 	transport_generic_free_cmd(&cmd->se_cmd, 0);
 }
 
-- 
2.28.0

[PATCH v2 20/25] usb: gadget: f_tcm: Save CPU ID per command

[Thinh Nguyen]

Normally we don't care about the CPU id, but if we ever use
TARGET_SCF_USE_CPUID, then we need to save the cpuid.

Signed-off-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com>
---
 Changes in v2:
 - None

 drivers/usb/gadget/function/f_tcm.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/usb/gadget/function/f_tcm.c b/drivers/usb/gadget/function/f_tcm.c
index 6cf95c9723a3..084143213176 100644
--- a/drivers/usb/gadget/function/f_tcm.c
+++ b/drivers/usb/gadget/function/f_tcm.c
@@ -1069,6 +1069,7 @@ static struct usbg_cmd *usbg_get_cmd(struct f_uas *fu,
 	memset(cmd, 0, sizeof(*cmd));
 	cmd->se_cmd.map_tag = tag;
 	cmd->se_cmd.map_cpu = cpu;
+	cmd->se_cmd.cpuid = cpu;
 	cmd->se_cmd.tag = cmd->tag = scsi_tag;
 	cmd->fu = fu;
 
-- 
2.28.0

[PATCH v2 21/25] usb: gadget: f_tcm: Get stream by tag

[Thinh Nguyen]

In preparation for TASK MANAGEMENT handling of overlapped command,
implement uasp_get_stream_by_tag() to find the active stream/command
based on a given tag.

For simplicity, we use mod operation to quickly find an in-progress
matching command tag to check for 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).

Signed-off-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com>
---
 Changes in v2:
 - Splitted from TASK MANAGEMENT change

 drivers/usb/gadget/function/f_tcm.c | 30 ++++++++++++++++++++++-------
 1 file changed, 23 insertions(+), 7 deletions(-)

diff --git a/drivers/usb/gadget/function/f_tcm.c b/drivers/usb/gadget/function/f_tcm.c
index 084143213176..a10e74290664 100644
--- a/drivers/usb/gadget/function/f_tcm.c
+++ b/drivers/usb/gadget/function/f_tcm.c
@@ -506,6 +506,22 @@ 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)
+{
+	/*
+	 * For simplicity, we use mod operation to quickly find an in-progress
+	 * matching command tag to check for 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).
+	 */
+	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 +529,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 +562,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;
@@ -568,8 +584,8 @@ static void uasp_prepare_status(struct usbg_cmd *cmd)
 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);
 	int ret;
 
 	if (req->status == -ESHUTDOWN)
@@ -620,7 +636,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);
@@ -632,7 +648,7 @@ static int uasp_send_status_response(struct usbg_cmd *cmd)
 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;
 
@@ -675,7 +691,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;
 
@@ -1285,7 +1301,7 @@ 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)
-- 
2.28.0

Re: [PATCH v2 21/25] usb: gadget: f_tcm: Get stream by tag

[Sebastian Andrzej Siewior]

On 2022-07-18 18:28:16 [-0700], Thinh Nguyen wrote:
> diff --git a/drivers/usb/gadget/function/f_tcm.c b/drivers/usb/gadget/function/f_tcm.c
> index 084143213176..a10e74290664 100644
> --- a/drivers/usb/gadget/function/f_tcm.c
> +++ b/drivers/usb/gadget/function/f_tcm.c
> @@ -506,6 +506,22 @@ 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)
> +{
> +	/*
> +	 * For simplicity, we use mod operation to quickly find an in-progress
> +	 * matching command tag to check for 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).
> +	 */
> +	return &fu->stream[tag % USBG_NUM_CMDS];

Could you please avoid the assumption what tag actually is?
Please take a look at hashtable.h, hash_add(), hash_del(),
hash_for_each_possible_safe() is probably all you need.
That % looks efficient but gcc will try and remove the div operation
which is something the hash implementation (as of hash_min()) avoids. So
the only additional costs here is the additional hashtable which worth
the price given that you don't assume what tag can be.

Sebastian

Re: [PATCH v2 21/25] usb: gadget: f_tcm: Get stream by tag

[Thinh Nguyen]

On Fri, Aug 26, 2022, Sebastian Andrzej Siewior wrote:
> On 2022-07-18 18:28:16 [-0700], Thinh Nguyen wrote:
> > diff --git a/drivers/usb/gadget/function/f_tcm.c b/drivers/usb/gadget/function/f_tcm.c
> > index 084143213176..a10e74290664 100644
> > --- a/drivers/usb/gadget/function/f_tcm.c
> > +++ b/drivers/usb/gadget/function/f_tcm.c
> > @@ -506,6 +506,22 @@ 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)
> > +{
> > +	/*
> > +	 * For simplicity, we use mod operation to quickly find an in-progress
> > +	 * matching command tag to check for 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).
> > +	 */
> > +	return &fu->stream[tag % USBG_NUM_CMDS];
> 
> Could you please avoid the assumption what tag actually is?
> Please take a look at hashtable.h, hash_add(), hash_del(),
> hash_for_each_possible_safe() is probably all you need.
> That % looks efficient but gcc will try and remove the div operation
> which is something the hash implementation (as of hash_min()) avoids. So
> the only additional costs here is the additional hashtable which worth
> the price given that you don't assume what tag can be.
> 

Sure, I can look into it.

Thanks,
Thinh

[PATCH v2 22/25] usb: gadget: f_tcm: Send sense on cancelled transfer

[Thinh Nguyen]

If the transfer is cancelled due to a disconnect or driver tear down
(error code -ESHUTDOWN), then just free the command. However, if it got
cancelled due to other reasons, then send a sense CHECK CONDITION status
with TCM_CHECK_CONDITION_ABORT_CMD status to host notifying the delivery
failure. Note that this is separate from TASK MANAGEMENT function abort
task command, which will require a separate response IU.

See UAS-r04 section 8.

Signed-off-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com>
---
 Changes in v2:
 - New patch

 drivers/usb/gadget/function/f_tcm.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/drivers/usb/gadget/function/f_tcm.c b/drivers/usb/gadget/function/f_tcm.c
index a10e74290664..116be1c47ac9 100644
--- a/drivers/usb/gadget/function/f_tcm.c
+++ b/drivers/usb/gadget/function/f_tcm.c
@@ -949,11 +949,16 @@ 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);
 
 	cmd->state = UASP_QUEUE_COMMAND;
 
-	if (req->status == -ESHUTDOWN)
-		goto cleanup;
+	if (req->status == -ESHUTDOWN) {
+		stream->cmd = NULL;
+		target_put_sess_cmd(se_cmd);
+		transport_generic_free_cmd(&cmd->se_cmd, 0);
+		return;
+	}
 
 	if (req->status) {
 		pr_err("%s() state %d transfer failed\n", __func__, cmd->state);
@@ -972,7 +977,8 @@ static void usbg_data_write_cmpl(struct usb_ep *ep, struct usb_request *req)
 
 cleanup:
 	target_put_sess_cmd(se_cmd);
-	transport_generic_free_cmd(&cmd->se_cmd, 0);
+	transport_send_check_condition_and_sense(se_cmd,
+			TCM_CHECK_CONDITION_ABORT_CMD, 0);
 }
 
 static int usbg_prepare_w_request(struct usbg_cmd *cmd, struct usb_request *req)
-- 
2.28.0

[PATCH v2 23/25] usb: gadget: f_tcm: Handle TASK_MANAGEMENT commands

[Thinh Nguyen]

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>
---
 Changes in v2:
 - Splitted into smaller change. Remove handling of overlapped tag.
 - Directly respond to host if f_tcm detected failure with response IU rather
   than reporting to target core.

 drivers/usb/gadget/function/f_tcm.c | 206 +++++++++++++++++++++++++---
 drivers/usb/gadget/function/tcm.h   |   7 +-
 2 files changed, 193 insertions(+), 20 deletions(-)

diff --git a/drivers/usb/gadget/function/f_tcm.c b/drivers/usb/gadget/function/f_tcm.c
index 116be1c47ac9..8b99ee07df87 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,51 @@ 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:
+	case TMR_TASK_DOES_NOT_EXIST:
+		return RC_TMF_COMPLETE;
+	case TMR_LUN_DOES_NOT_EXIST:
+		return RC_INCORRECT_LUN;
+	case TMR_OVERLAPPED_TAG_ATTEMPTED:
+		return RC_OVERLAPPED_TAG;
+	case TMR_FUNCTION_REJECTED:
+	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 */
@@ -581,6 +627,33 @@ 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 != RC_RESPONSE_UNKNOWN)
+		rsp_iu->response_code = 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 usbg_release_cmd(struct se_cmd *se_cmd);
+static int uasp_send_tm_response(struct usbg_cmd *cmd);
+
 static void uasp_status_data_cmpl(struct usb_ep *ep, struct usb_request *req)
 {
 	struct usbg_cmd *cmd = req->context;
@@ -619,9 +692,26 @@ static void uasp_status_data_cmpl(struct usb_ep *ep, struct usb_request *req)
 		break;
 
 	case UASP_QUEUE_COMMAND:
-		transport_generic_free_cmd(&cmd->se_cmd, 0);
-		usb_ep_queue(fu->ep_cmd, cmd->req, GFP_ATOMIC);
 
+		stream->cmd = NULL;
+
+		/*
+		 * If no command submitted to target core here, just free the
+		 * bitmap index. This is for the cases where f_tcm handles
+		 * status response instead of the target core.
+		 */
+		if (cmd->tmr_rsp != RC_RESPONSE_UNKNOWN) {
+			struct se_session *se_sess;
+
+			se_sess = fu->tpg->tpg_nexus->tvn_se_sess;
+			sbitmap_queue_clear(&se_sess->sess_tag_pool,
+					    cmd->se_cmd.map_tag,
+					    cmd->se_cmd.map_cpu);
+		} else {
+			transport_generic_free_cmd(&cmd->se_cmd, 0);
+		}
+
+		usb_ep_queue(fu->ep_cmd, cmd->req, GFP_ATOMIC);
 		break;
 
 	default:
@@ -630,6 +720,7 @@ static void uasp_status_data_cmpl(struct usb_ep *ep, struct usb_request *req)
 	return;
 
 cleanup:
+	stream->cmd = NULL;
 	transport_generic_free_cmd(&cmd->se_cmd, 0);
 }
 
@@ -645,6 +736,18 @@ 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;
@@ -1045,9 +1148,24 @@ 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 usbg_cmd *cmd = container_of(work, struct usbg_cmd, work);
+	struct se_session *se_sess;
+	struct se_cmd *se_cmd;
+	int flags = TARGET_SCF_ACK_KREF;
+
+	se_cmd = &cmd->se_cmd;
+	se_sess = cmd->fu->tpg->tpg_nexus->tvn_se_sess;
+
+	target_submit_tmr(se_cmd, 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 se_cmd *se_cmd;
 	struct tcm_usbg_nexus *tv_nexus;
 	struct usbg_tpg *tpg;
@@ -1076,6 +1194,29 @@ 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);
+
+	/*
+	 * Failure is detected by f_tcm here. Skip submitting the command to the
+	 * target core if we already know the failing response and send the usb
+	 * response to the host directly.
+	 */
+	if (cmd->tmr_rsp != RC_RESPONSE_UNKNOWN)
+		goto skip;
+
+	if (cmd->tmr_func)
+		usbg_submit_tmr(cmd);
+	else
+		usbg_submit_cmd(cmd);
+
+	return;
+
+skip:
+	uasp_send_tm_response(cmd);
+}
+
 static struct usbg_cmd *usbg_get_cmd(struct f_uas *fu,
 		struct tcm_usbg_nexus *tv_nexus, u32 scsi_tag)
 {
@@ -1102,37 +1243,63 @@ 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 = RC_RESPONSE_UNKNOWN;
+
+	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 = RC_INVALID_INFO_UNIT;
+		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:
@@ -1153,9 +1320,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);
 
@@ -1301,6 +1466,9 @@ 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)
diff --git a/drivers/usb/gadget/function/tcm.h b/drivers/usb/gadget/function/tcm.h
index 5157af1b166b..7fafc4336984 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	RC_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 {
-- 
2.28.0

[PATCH v2 24/25] usb: gadget: f_tcm: Check overlapped command

[Thinh Nguyen]

If there's an overlapped command tag, cancel the command and respond
with RC_OVERLAPPED_TAG to host.

Signed-off-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com>
---
 Changes in v2:
 - New patch. It was part of TASK MANAGEMENT change.
 - Directly respond to host on overlapped tag instead of reporting to target
   core.

 drivers/usb/gadget/function/f_tcm.c | 75 ++++++++++++++++++++++++++++-
 1 file changed, 73 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/gadget/function/f_tcm.c b/drivers/usb/gadget/function/f_tcm.c
index 8b99ee07df87..dfa3e7c043a3 100644
--- a/drivers/usb/gadget/function/f_tcm.c
+++ b/drivers/usb/gadget/function/f_tcm.c
@@ -692,6 +692,15 @@ static void uasp_status_data_cmpl(struct usb_ep *ep, struct usb_request *req)
 		break;
 
 	case UASP_QUEUE_COMMAND:
+		/*
+		 * Overlapped command detected and cancelled.
+		 * So send overlapped attempted status.
+		 */
+		if (cmd->tmr_rsp == RC_OVERLAPPED_TAG &&
+		    req->status == -ECONNRESET) {
+			uasp_send_tm_response(cmd);
+			return;
+		}
 
 		stream->cmd = NULL;
 
@@ -700,7 +709,8 @@ static void uasp_status_data_cmpl(struct usb_ep *ep, struct usb_request *req)
 		 * bitmap index. This is for the cases where f_tcm handles
 		 * status response instead of the target core.
 		 */
-		if (cmd->tmr_rsp != RC_RESPONSE_UNKNOWN) {
+		if (cmd->tmr_rsp != RC_OVERLAPPED_TAG &&
+		    cmd->tmr_rsp != RC_RESPONSE_UNKNOWN) {
 			struct se_session *se_sess;
 
 			se_sess = fu->tpg->tpg_nexus->tvn_se_sess;
@@ -1080,6 +1090,14 @@ static void usbg_data_write_cmpl(struct usb_ep *ep, struct usb_request *req)
 
 cleanup:
 	target_put_sess_cmd(se_cmd);
+
+	/* Command was aborted due to overlapped tag */
+	if (cmd->state == UASP_QUEUE_COMMAND &&
+	    cmd->tmr_rsp == RC_OVERLAPPED_TAG) {
+		uasp_send_tm_response(cmd);
+		return;
+	}
+
 	transport_send_check_condition_and_sense(se_cmd,
 			TCM_CHECK_CONDITION_ABORT_CMD, 0);
 }
@@ -1148,9 +1166,10 @@ static int usbg_send_read_response(struct se_cmd *se_cmd)
 		return uasp_send_read_response(cmd);
 }
 
+static void usbg_aborted_task(struct se_cmd *se_cmd);
+
 static void usbg_submit_tmr(struct usbg_cmd *cmd)
 {
-	struct usbg_cmd *cmd = container_of(work, struct usbg_cmd, work);
 	struct se_session *se_sess;
 	struct se_cmd *se_cmd;
 	int flags = TARGET_SCF_ACK_KREF;
@@ -1214,6 +1233,51 @@ static void usbg_cmd_work(struct work_struct *work)
 	return;
 
 skip:
+	if (cmd->tmr_rsp == RC_OVERLAPPED_TAG) {
+		struct se_session *se_sess;
+		struct uas_stream *stream;
+
+		se_sess = cmd->fu->tpg->tpg_nexus->tvn_se_sess;
+		stream = uasp_get_stream_by_tag(cmd->fu, cmd->tag);
+
+		/*
+		 * 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;
+		}
+
+		/*
+		 * The command isn't submitted to the target core, so we're safe
+		 * to remove the bitmap index from the session tag pool.
+		 */
+		sbitmap_queue_clear(&se_sess->sess_tag_pool,
+				    cmd->se_cmd.map_tag,
+				    cmd->se_cmd.map_cpu);
+
+		/*
+		 * Overlap command tag detected. Cancel any pending transfer of
+		 * the command submitted to target core.
+		 */
+		stream->cmd->tmr_rsp = RC_OVERLAPPED_TAG;
+		usbg_aborted_task(&stream->cmd->se_cmd);
+
+		/* Send the response after the transfer is aborted. */
+		return;
+	}
+
 	uasp_send_tm_response(cmd);
 }
 
@@ -1282,6 +1346,13 @@ static int usbg_submit_command(struct f_uas *fu, struct usb_request *req)
 		goto skip;
 	}
 
+	stream = uasp_get_stream_by_tag(fu, scsi_tag);
+	if (stream->cmd) {
+		pr_err("Command tag %d overlapped\n", scsi_tag);
+		cmd->tmr_rsp = RC_OVERLAPPED_TAG;
+		goto skip;
+	}
+
 	stream->cmd = cmd;
 
 	if (iu->iu_id == IU_ID_TASK_MGMT) {
-- 
2.28.0

Re: [PATCH v2 24/25] usb: gadget: f_tcm: Check overlapped command

[Sebastian Andrzej Siewior]

On 2022-07-18 18:28:34 [-0700], Thinh Nguyen wrote:
> diff --git a/drivers/usb/gadget/function/f_tcm.c b/drivers/usb/gadget/function/f_tcm.c
> index 8b99ee07df87..dfa3e7c043a3 100644
> --- a/drivers/usb/gadget/function/f_tcm.c
> +++ b/drivers/usb/gadget/function/f_tcm.c
> @@ -1214,6 +1233,51 @@ static void usbg_cmd_work(struct work_struct *work)
>  	return;
>  
>  skip:
> +	if (cmd->tmr_rsp == RC_OVERLAPPED_TAG) {
> +		struct se_session *se_sess;
> +		struct uas_stream *stream;
> +
> +		se_sess = cmd->fu->tpg->tpg_nexus->tvn_se_sess;
> +		stream = uasp_get_stream_by_tag(cmd->fu, cmd->tag);
> +
> +		/*
> +		 * 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);

How likely is it for this to happen? Is there maybe some synchronisation
missing? If I see this right, in order to get here, you will already
spill the message "Command tag %d overlapped" which does not look good.
Why should the host re-use a tag which did not yet complete?

Sebastian

Re: [PATCH v2 24/25] usb: gadget: f_tcm: Check overlapped command

[Thinh Nguyen]

On Fri, Aug 26, 2022, Sebastian Andrzej Siewior wrote:
> On 2022-07-18 18:28:34 [-0700], Thinh Nguyen wrote:
> > diff --git a/drivers/usb/gadget/function/f_tcm.c b/drivers/usb/gadget/function/f_tcm.c
> > index 8b99ee07df87..dfa3e7c043a3 100644
> > --- a/drivers/usb/gadget/function/f_tcm.c
> > +++ b/drivers/usb/gadget/function/f_tcm.c
> > @@ -1214,6 +1233,51 @@ static void usbg_cmd_work(struct work_struct *work)
> >  	return;
> >  
> >  skip:
> > +	if (cmd->tmr_rsp == RC_OVERLAPPED_TAG) {
> > +		struct se_session *se_sess;
> > +		struct uas_stream *stream;
> > +
> > +		se_sess = cmd->fu->tpg->tpg_nexus->tvn_se_sess;
> > +		stream = uasp_get_stream_by_tag(cmd->fu, cmd->tag);
> > +
> > +		/*
> > +		 * 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);
> 
> How likely is it for this to happen? Is there maybe some synchronisation
> missing? If I see this right, in order to get here, you will already
> spill the message "Command tag %d overlapped" which does not look good.
> Why should the host re-use a tag which did not yet complete?
> 

Not often, but it can happen. For example, even though the device sent a
status on the wire and the host received it. The host may send a new
command with the same tag before the device is notified of the previous
command completion. Since they operate in different endpoints, the
device driver may get notification of the new command from the command
endpoint before the status completion of the status endpoint.

This is an attempt to maintain synchronization and prevent false overlap
check. It's a quick fix. I feel that we can handle this better. If you
can think of a better way to handle this, let me know.

BR,
Thinh

[PATCH v2 25/25] usb: gadget: f_tcm: Comply with UAS Task Management requirement

[Thinh Nguyen]

The UASP driver must support all the task management functions listed in
Table 20 of UAS-r04. However, not all of them are implemented in the
Target core. To remain compliant while indicate that the TMR did not go
through, report RC_TMF_FAILED instead of RC_TMF_NOT_SUPPORTED and print
a warning to the user.

Signed-off-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com>
---
 Changes in v2:
 - None

 drivers/usb/gadget/function/f_tcm.c | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/drivers/usb/gadget/function/f_tcm.c b/drivers/usb/gadget/function/f_tcm.c
index dfa3e7c043a3..727d7c3e61c2 100644
--- a/drivers/usb/gadget/function/f_tcm.c
+++ b/drivers/usb/gadget/function/f_tcm.c
@@ -643,6 +643,30 @@ static void uasp_prepare_response(struct usbg_cmd *cmd)
 		rsp_iu->response_code =
 			tcm_to_uasp_response(se_cmd->se_tmr_req->response);
 
+	/*
+	 * The UASP driver must support all the task management functions listed
+	 * in Table 20 of UAS-r04. To remain compliant while indicate that the
+	 * TMR did not go through, report RC_TMF_FAILED instead of
+	 * RC_TMF_NOT_SUPPORTED and print a warning to the user.
+	 */
+	switch (cmd->tmr_func) {
+	case TMR_ABORT_TASK:
+	case TMR_ABORT_TASK_SET:
+	case TMR_CLEAR_TASK_SET:
+	case TMR_LUN_RESET:
+	case TMR_I_T_NEXUS_RESET:
+	case TMR_CLEAR_ACA:
+	case TMR_QUERY_TASK:
+	case TMR_QUERY_TASK_SET:
+	case TMR_QUERY_ASYNC_EVENT:
+		if (rsp_iu->response_code == RC_TMF_NOT_SUPPORTED) {
+			pr_warn("TMR function %d not supported\n",
+				cmd->tmr_func);
+			rsp_iu->response_code = RC_TMF_FAILED;
+		}
+		break;
+	}
+
 	stream->req_status->is_last = 1;
 	stream->req_status->stream_id = cmd->tag;
 	stream->req_status->context = cmd;
-- 
2.28.0

Re: [PATCH v2 00/25] usb: gadget: f_tcm: Enhance UASP driver

[Greg Kroah-Hartman]

On Mon, Jul 18, 2022 at 06:26:01PM -0700, Thinh Nguyen wrote:
> The Linux UASP gadget driver is incomplete and remained broken for a long time.
> It was not implemented for performance either. This series adds some of the
> required features for the UASP driver to work. It also makes some changes to
> the target core.
> 
> This is tested against UASP CV and DWC_usb3x controller. It still needs some
> fixes in the target core, which will be separated from this series.
> 
> There are still more room for performance improvement and fixes. However, this
> series should be sufficient to bring up a working UASP device.
> 
> 
> Changes in v2:
>  - Remove most target core changes from this series and only keep the must-have
>    ones
>  - Split the task-management patch to smaller patches
>  - Don't send failure Task Management response to target core, reducing
>    dependency
>  - Add UASP bringup script example in cover page
>  - Make various small updates according to feedbacks

I would need a review by the target maintainers before being able to
take any of the USB gadget changes into the USB tree...

thanks,

greg k-h

Re: [PATCH v2 00/25] usb: gadget: f_tcm: Enhance UASP driver

[Thinh Nguyen]

Hi Martin, Dmitry, and others,

On Fri, Aug 19, 2022 at 10:31:23AM +0200, Greg Kroah-Hartman wrote:
> On Mon, Jul 18, 2022 at 06:26:01PM -0700, Thinh Nguyen wrote:
> > The Linux UASP gadget driver is incomplete and remained broken for a long time.
> > It was not implemented for performance either. This series adds some of the
> > required features for the UASP driver to work. It also makes some changes to
> > the target core.
> > 
> > This is tested against UASP CV and DWC_usb3x controller. It still needs some
> > fixes in the target core, which will be separated from this series.
> > 
> > There are still more room for performance improvement and fixes. However, this
> > series should be sufficient to bring up a working UASP device.
> > 
> > 
> > Changes in v2:
> >  - Remove most target core changes from this series and only keep the must-have
> >    ones
> >  - Split the task-management patch to smaller patches
> >  - Don't send failure Task Management response to target core, reducing
> >    dependency
> >  - Add UASP bringup script example in cover page
> >  - Make various small updates according to feedbacks
> 
> I would need a review by the target maintainers before being able to
> take any of the USB gadget changes into the USB tree...
> 

Do you have any comment on this series?

Thanks,
Thinh

Re: [PATCH v2 00/25] usb: gadget: f_tcm: Enhance UASP driver

[Thinh Nguyen]

On Fri, Aug 19, 2022, Thinh Nguyen wrote:
> Hi Martin, Dmitry, and others,
> 
> On Fri, Aug 19, 2022 at 10:31:23AM +0200, Greg Kroah-Hartman wrote:
> > On Mon, Jul 18, 2022 at 06:26:01PM -0700, Thinh Nguyen wrote:
> > > The Linux UASP gadget driver is incomplete and remained broken for a long time.
> > > It was not implemented for performance either. This series adds some of the
> > > required features for the UASP driver to work. It also makes some changes to
> > > the target core.
> > > 
> > > This is tested against UASP CV and DWC_usb3x controller. It still needs some
> > > fixes in the target core, which will be separated from this series.
> > > 
> > > There are still more room for performance improvement and fixes. However, this
> > > series should be sufficient to bring up a working UASP device.
> > > 
> > > 
> > > Changes in v2:
> > >  - Remove most target core changes from this series and only keep the must-have
> > >    ones
> > >  - Split the task-management patch to smaller patches
> > >  - Don't send failure Task Management response to target core, reducing
> > >    dependency
> > >  - Add UASP bringup script example in cover page
> > >  - Make various small updates according to feedbacks
> > 
> > I would need a review by the target maintainers before being able to
> > take any of the USB gadget changes into the USB tree...
> > 
> 
> Do you have any comment on this series?
> 

Hi target maintainers,

Gentle ping...

BR,
Thinh

Re: [PATCH v2 00/25] usb: gadget: f_tcm: Enhance UASP driver

[Sebastian Andrzej Siewior]

On 2022-07-18 18:26:01 [-0700], Thinh Nguyen wrote:
> The Linux UASP gadget driver is incomplete and remained broken for a long time.
> It was not implemented for performance either. This series adds some of the
> required features for the UASP driver to work. It also makes some changes to
> the target core.

Some patches here have fixes: tags and are in the middle of the series.
If they are indeed fixes which are needed for the driver function
regardless of the other changes, which are part of the series, then they
should be moved to the front of series _or_ submitted independently as
in "lets first fix the broken things and then make it pretty".

All in all I am happy to see that somebody is looking into the target
USB gadget.

Sebastian

Re: [PATCH v2 00/25] usb: gadget: f_tcm: Enhance UASP driver

[Thinh Nguyen]

On Fri, Aug 26, 2022, Sebastian Andrzej Siewior wrote:
> On 2022-07-18 18:26:01 [-0700], Thinh Nguyen wrote:
> > The Linux UASP gadget driver is incomplete and remained broken for a long time.
> > It was not implemented for performance either. This series adds some of the
> > required features for the UASP driver to work. It also makes some changes to
> > the target core.
> 
> Some patches here have fixes: tags and are in the middle of the series.
> If they are indeed fixes which are needed for the driver function
> regardless of the other changes, which are part of the series, then they
> should be moved to the front of series _or_ submitted independently as
> in "lets first fix the broken things and then make it pretty".
> 
> All in all I am happy to see that somebody is looking into the target
> USB gadget.
> 

Thanks for the reviews!

I can move the commits with fixes tag around. But for the driver to work
properly against different OSes (and maybe even for Linux), most of the
changes without the fixes tags are also needed and not just to make it
"pretty".

There are still more work for f_tcm, but this series (plus the fixes
series in target) is enough for the driver to work properly.

Thanks,
Thinh