[PATCH v3 00/28] usb: gadget: f_tcm: Enhance UASP driver

[PATCH v3 00/28] usb: gadget: f_tcm: Enhance UASP driver

[Thinh Nguyen]

Apologies for the delay; after two years and multiple requests to resume this
series, I squeezed some time to push an update. This series applies on top of
Greg's usb-testing branch.

If possible, please help test this series and get this merged as my resources
are nil for this work.

Example Bringup Steps
=====================
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");


Target Subsystem Fixes
======================
I have eliminated unnecessary changes related to the Target subsystem and
reworked f_tcm to minimize the modifications required in the Target subsystem.
There are unimplemented Task Management Requests in the Target subsystem, but
the basic flow should still work.

Regardless, you should still need to apply at least these 2 fixes:

1) Fix Data Corruption
----------------------

Properly increment the "len" base on the command requested length instead of
the SG entry length.

If you're using File backend, then you need to fix target_core_file. If you're
using other backend such as Ramdisk, then you need a similar fix there.

---
 drivers/target/target_core_file.c | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/drivers/target/target_core_file.c b/drivers/target/target_core_file.c
index 2d78ef74633c..d9fc048c1734 100644
--- a/drivers/target/target_core_file.c
+++ b/drivers/target/target_core_file.c
@@ -283,7 +283,12 @@ fd_execute_rw_aio(struct se_cmd *cmd, struct scatterlist *sgl, u32 sgl_nents,
 	for_each_sg(sgl, sg, sgl_nents, i) {
 		bvec_set_page(&aio_cmd->bvecs[i], sg_page(sg), sg->length,
 			      sg->offset);
-		len += sg->length;
+		if (len + sg->length >= cmd->data_length) {
+			len = cmd->data_length;
+			break;
+		} else {
+			len += sg->length;
+		}
 	}
 
 	iov_iter_bvec(&iter, is_write, aio_cmd->bvecs, sgl_nents, len);
@@ -328,7 +333,12 @@ static int fd_do_rw(struct se_cmd *cmd, struct file *fd,
 
 	for_each_sg(sgl, sg, sgl_nents, i) {
 		bvec_set_page(&bvec[i], sg_page(sg), sg->length, sg->offset);
-		len += sg->length;
+		if (len + sg->length >= data_length) {
+			len = data_length;
+			break;
+		} else {
+			len += sg->length;
+		}
 	}
 
 	iov_iter_bvec(&iter, is_write, bvec, sgl_nents, len);
-- 


2) Fix Sense Data Length
------------------------

The transport_get_sense_buffer() and transport_copy_sense_to_cmd() take
sense data length to be the allocated sense buffer length
TRANSPORT_SENSE_BUFFER. However, the sense data length is depending on
the sense data description. Check the sense data to set the proper
cmd->scsi_sense_length.

See SPC4-r37 section 4.5.2.1.
---
 drivers/target/target_core_transport.c | 19 +++++++++++++++----
 1 file changed, 15 insertions(+), 4 deletions(-)

diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
index 8d8f4ad4f59e..da75d6873ab5 100644
--- a/drivers/target/target_core_transport.c
+++ b/drivers/target/target_core_transport.c
@@ -804,8 +804,6 @@ static unsigned char *transport_get_sense_buffer(struct se_cmd *cmd)
 	if (cmd->se_cmd_flags & SCF_SENT_CHECK_CONDITION)
 		return NULL;
 
-	cmd->scsi_sense_length = TRANSPORT_SENSE_BUFFER;
-
 	pr_debug("HBA_[%u]_PLUG[%s]: Requesting sense for SAM STATUS: 0x%02x\n",
 		dev->se_hba->hba_id, dev->transport->name, cmd->scsi_status);
 	return cmd->sense_buffer;
@@ -824,7 +822,13 @@ void transport_copy_sense_to_cmd(struct se_cmd *cmd, unsigned char *sense)
 	}
 
 	cmd->se_cmd_flags |= SCF_TRANSPORT_TASK_SENSE;
+
+	/* Sense data length = min sense data + additional sense data length */
+	cmd->scsi_sense_length = min_t(u16, cmd_sense_buf[7] + 8,
+				       TRANSPORT_SENSE_BUFFER);
+
 	memcpy(cmd_sense_buf, sense, cmd->scsi_sense_length);
+
 	spin_unlock_irqrestore(&cmd->t_state_lock, flags);
 }
 EXPORT_SYMBOL(transport_copy_sense_to_cmd);
@@ -3521,12 +3525,19 @@ static void translate_sense_reason(struct se_cmd *cmd, sense_reason_t reason)
 
 	cmd->se_cmd_flags |= SCF_EMULATED_TASK_SENSE;
 	cmd->scsi_status = SAM_STAT_CHECK_CONDITION;
-	cmd->scsi_sense_length  = TRANSPORT_SENSE_BUFFER;
+
 	scsi_build_sense_buffer(desc_format, buffer, key, asc, ascq);
 	if (sd->add_sense_info)
 		WARN_ON_ONCE(scsi_set_sense_information(buffer,
-							cmd->scsi_sense_length,
+							TRANSPORT_SENSE_BUFFER,
 							cmd->sense_info) < 0);
+	/*
+	 * CHECK CONDITION returns sense data, and sense data is minimum 8
+	 * bytes long plus additional Sense Data Length.
+	 * See SPC4-r37 section 4.5.2.1.
+	 */
+	cmd->scsi_sense_length = min_t(u16, buffer[7] + 8,
+				       TRANSPORT_SENSE_BUFFER);
 }
 
 int
-- 


Changes in v3:
 - v2: https://lore.kernel.org/linux-usb/cover.1658192351.git.Thinh.Nguyen@synopsys.com/
 - Moved patches around so fixes patches go first
 - Use hashtable to map tag to uas stream
 - Move target_execute_cmd() out of interrupt context
 - Various cleanup
 - Additional fixes over the 2 years


Thinh Nguyen (28):
  usb: gadget: f_tcm: Don't free command immediately
  usb: gadget: f_tcm: Translate error to sense
  usb: gadget: f_tcm: Decrement command ref count on cleanup
  usb: gadget: f_tcm: Fix Get/SetInterface return value
  usb: gadget: f_tcm: ep_autoconfig with fullspeed endpoint
  usb: gadget: f_tcm: Don't prepare BOT write request twice
  usb: gadget: f_tcm: Increase stream count
  usb: gadget: f_tcm: Increase bMaxBurst
  usb: gadget: f_tcm: Limit number of sessions
  usb: gadget: f_tcm: Get stream by sbitmap number
  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: 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: Handle abort command
  usb: gadget: f_tcm: Cleanup requests on ep disable
  usb: gadget: f_tcm: Stop proceeding further on -ESHUTDOWN
  usb: gadget: f_tcm: Save CPU ID per command
  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: Stall on invalid CBW
  usb: gadget: f_tcm: Requeue command request on error
  usb: gadget: f_tcm: Track BOT command kref
  usb: gadget: f_tcm: Refactor goto check_condition

 drivers/usb/gadget/function/f_tcm.c | 711 ++++++++++++++++++++--------
 drivers/usb/gadget/function/tcm.h   |  28 +-
 2 files changed, 547 insertions(+), 192 deletions(-)


base-commit: d8d936c51388442f769a81e512b505dcf87c6a51
-- 
2.28.0

[PATCH v3 01/28] 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")
Cc: stable@vger.kernel.org
Signed-off-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com>
---
 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 e1914b20f816..6313302a5b96 100644
--- a/drivers/usb/gadget/function/f_tcm.c
+++ b/drivers/usb/gadget/function/f_tcm.c
@@ -1062,7 +1062,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,
@@ -1191,7 +1190,6 @@ static void bot_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 int bot_submit_command(struct f_uas *fu,
-- 
2.28.0

[PATCH v3 02/28] 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")
Cc: stable@vger.kernel.org
Signed-off-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com>
---
 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 6313302a5b96..88b8b94fdb1e 100644
--- a/drivers/usb/gadget/function/f_tcm.c
+++ b/drivers/usb/gadget/function/f_tcm.c
@@ -1061,7 +1061,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,
@@ -1189,7 +1189,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);
 }
 
 static int bot_submit_command(struct f_uas *fu,
-- 
2.28.0

[PATCH v3 03/28] 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")
Cc: stable@vger.kernel.org
Signed-off-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com>
---
 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 88b8b94fdb1e..5ce97723376e 100644
--- a/drivers/usb/gadget/function/f_tcm.c
+++ b/drivers/usb/gadget/function/f_tcm.c
@@ -969,6 +969,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 v3 04/28] usb: gadget: f_tcm: Fix Get/SetInterface return value

[Thinh Nguyen]

Check to make sure that the GetInterface and SetInterface are for valid
interface. Return proper alternate setting number on GetInterface.

Fixes: 0b8b1a1fede0 ("usb: gadget: f_tcm: Provide support to get alternate setting in tcm function")
Cc: stable@vger.kernel.org
Signed-off-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com>
---
 drivers/usb/gadget/function/f_tcm.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/gadget/function/f_tcm.c b/drivers/usb/gadget/function/f_tcm.c
index 5ce97723376e..f996878e1aea 100644
--- a/drivers/usb/gadget/function/f_tcm.c
+++ b/drivers/usb/gadget/function/f_tcm.c
@@ -2046,9 +2046,14 @@ static void tcm_delayed_set_alt(struct work_struct *wq)
 
 static int tcm_get_alt(struct usb_function *f, unsigned intf)
 {
-	if (intf == bot_intf_desc.bInterfaceNumber)
+	struct f_uas *fu = to_f_uas(f);
+
+	if (fu->iface != intf)
+		return -EOPNOTSUPP;
+
+	if (fu->flags & USBG_IS_BOT)
 		return USB_G_ALT_INT_BBB;
-	if (intf == uasp_intf_desc.bInterfaceNumber)
+	else if (fu->flags & USBG_IS_UAS)
 		return USB_G_ALT_INT_UAS;
 
 	return -EOPNOTSUPP;
@@ -2058,6 +2063,9 @@ static int tcm_set_alt(struct usb_function *f, unsigned intf, unsigned alt)
 {
 	struct f_uas *fu = to_f_uas(f);
 
+	if (fu->iface != intf)
+		return -EOPNOTSUPP;
+
 	if ((alt == USB_G_ALT_INT_BBB) || (alt == USB_G_ALT_INT_UAS)) {
 		struct guas_setup_wq *work;
 
-- 
2.28.0

[PATCH v3 05/28] usb: gadget: f_tcm: ep_autoconfig with fullspeed endpoint

[Thinh Nguyen]

Match usb endpoint using fullspeed endpoint descriptor to make sure the
wMaxPacketSize for fullspeed descriptors is automatically configured.

Fixes: c52661d60f63 ("usb-gadget: Initial merge of target module for UASP + BOT")
Cc: stable@vger.kernel.org
Signed-off-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com>
---
 drivers/usb/gadget/function/f_tcm.c | 32 +++++++++++++----------------
 1 file changed, 14 insertions(+), 18 deletions(-)

diff --git a/drivers/usb/gadget/function/f_tcm.c b/drivers/usb/gadget/function/f_tcm.c
index f996878e1aea..b35e0446d467 100644
--- a/drivers/usb/gadget/function/f_tcm.c
+++ b/drivers/usb/gadget/function/f_tcm.c
@@ -1966,43 +1966,39 @@ static int tcm_bind(struct usb_configuration *c, struct usb_function *f)
 	bot_intf_desc.bInterfaceNumber = iface;
 	uasp_intf_desc.bInterfaceNumber = iface;
 	fu->iface = iface;
-	ep = usb_ep_autoconfig_ss(gadget, &uasp_ss_bi_desc,
-			&uasp_bi_ep_comp_desc);
+	ep = usb_ep_autoconfig(gadget, &uasp_fs_bi_desc);
 	if (!ep)
 		goto ep_fail;
 
 	fu->ep_in = ep;
 
-	ep = usb_ep_autoconfig_ss(gadget, &uasp_ss_bo_desc,
-			&uasp_bo_ep_comp_desc);
+	ep = usb_ep_autoconfig(gadget, &uasp_fs_bo_desc);
 	if (!ep)
 		goto ep_fail;
 	fu->ep_out = ep;
 
-	ep = usb_ep_autoconfig_ss(gadget, &uasp_ss_status_desc,
-			&uasp_status_in_ep_comp_desc);
+	ep = usb_ep_autoconfig(gadget, &uasp_fs_status_desc);
 	if (!ep)
 		goto ep_fail;
 	fu->ep_status = ep;
 
-	ep = usb_ep_autoconfig_ss(gadget, &uasp_ss_cmd_desc,
-			&uasp_cmd_comp_desc);
+	ep = usb_ep_autoconfig(gadget, &uasp_fs_cmd_desc);
 	if (!ep)
 		goto ep_fail;
 	fu->ep_cmd = ep;
 
 	/* Assume endpoint addresses are the same for both speeds */
-	uasp_bi_desc.bEndpointAddress =	uasp_ss_bi_desc.bEndpointAddress;
-	uasp_bo_desc.bEndpointAddress = uasp_ss_bo_desc.bEndpointAddress;
+	uasp_bi_desc.bEndpointAddress =	uasp_fs_bi_desc.bEndpointAddress;
+	uasp_bo_desc.bEndpointAddress = uasp_fs_bo_desc.bEndpointAddress;
 	uasp_status_desc.bEndpointAddress =
-		uasp_ss_status_desc.bEndpointAddress;
-	uasp_cmd_desc.bEndpointAddress = uasp_ss_cmd_desc.bEndpointAddress;
-
-	uasp_fs_bi_desc.bEndpointAddress = uasp_ss_bi_desc.bEndpointAddress;
-	uasp_fs_bo_desc.bEndpointAddress = uasp_ss_bo_desc.bEndpointAddress;
-	uasp_fs_status_desc.bEndpointAddress =
-		uasp_ss_status_desc.bEndpointAddress;
-	uasp_fs_cmd_desc.bEndpointAddress = uasp_ss_cmd_desc.bEndpointAddress;
+		uasp_fs_status_desc.bEndpointAddress;
+	uasp_cmd_desc.bEndpointAddress = uasp_fs_cmd_desc.bEndpointAddress;
+
+	uasp_ss_bi_desc.bEndpointAddress = uasp_fs_bi_desc.bEndpointAddress;
+	uasp_ss_bo_desc.bEndpointAddress = uasp_fs_bo_desc.bEndpointAddress;
+	uasp_ss_status_desc.bEndpointAddress =
+		uasp_fs_status_desc.bEndpointAddress;
+	uasp_ss_cmd_desc.bEndpointAddress = uasp_fs_cmd_desc.bEndpointAddress;
 
 	ret = usb_assign_descriptors(f, uasp_fs_function_desc,
 			uasp_hs_function_desc, uasp_ss_function_desc,
-- 
2.28.0

[PATCH v3 06/28] usb: gadget: f_tcm: Don't prepare BOT write request twice

[Thinh Nguyen]

The duplicate kmalloc here is causing memory leak. The request
preparation in bot_send_write_request is also done in
usbg_prepare_w_request. Remove the duplicate work.

Fixes: c52661d60f63 ("usb-gadget: Initial merge of target module for UASP + BOT")
Cc: stable@vger.kernel.org
Signed-off-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com>
---
 drivers/usb/gadget/function/f_tcm.c | 17 -----------------
 1 file changed, 17 deletions(-)

diff --git a/drivers/usb/gadget/function/f_tcm.c b/drivers/usb/gadget/function/f_tcm.c
index b35e0446d467..4fd56ae056a3 100644
--- a/drivers/usb/gadget/function/f_tcm.c
+++ b/drivers/usb/gadget/function/f_tcm.c
@@ -245,7 +245,6 @@ static int bot_send_write_request(struct usbg_cmd *cmd)
 {
 	struct f_uas *fu = cmd->fu;
 	struct se_cmd *se_cmd = &cmd->se_cmd;
-	struct usb_gadget *gadget = fuas_to_gadget(fu);
 	int ret;
 
 	init_completion(&cmd->write_complete);
@@ -256,22 +255,6 @@ static int bot_send_write_request(struct usbg_cmd *cmd)
 		return -EINVAL;
 	}
 
-	if (!gadget->sg_supported) {
-		cmd->data_buf = kmalloc(se_cmd->data_length, GFP_KERNEL);
-		if (!cmd->data_buf)
-			return -ENOMEM;
-
-		fu->bot_req_out->buf = cmd->data_buf;
-	} else {
-		fu->bot_req_out->buf = NULL;
-		fu->bot_req_out->num_sgs = se_cmd->t_data_nents;
-		fu->bot_req_out->sg = se_cmd->t_data_sg;
-	}
-
-	fu->bot_req_out->complete = usbg_data_write_cmpl;
-	fu->bot_req_out->length = se_cmd->data_length;
-	fu->bot_req_out->context = cmd;
-
 	ret = usbg_prepare_w_request(cmd, fu->bot_req_out);
 	if (ret)
 		goto cleanup;
-- 
2.28.0

[PATCH v3 07/28] usb: gadget: f_tcm: Increase stream count

[Thinh Nguyen]

Some old builds of Microsoft Windows 10 UASP class driver reject 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>
---
 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

[PATCH v3 08/28] 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>
---
 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 4fd56ae056a3..1c93f07daa7b 100644
--- a/drivers/usb/gadget/function/f_tcm.c
+++ b/drivers/usb/gadget/function/f_tcm.c
@@ -1724,7 +1724,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,
 };
@@ -1732,7 +1732,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 = {
@@ -1767,12 +1767,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 v3 09/28] usb: gadget: f_tcm: Limit number of sessions

[Thinh Nguyen]

Only allocate up to UASP_SS_EP_COMP_NUM_STREAMS number of session tags.
We should not be using more than UASP_SS_EP_COMP_NUM_STREAMS of tags due
to the number of commands limit we imposed. Each command uses a unique
tag. Any more than that is unnecessary. By limiting 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>
---
 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 6cb05dcd19ff..385bc2cdefb6 100644
--- a/drivers/usb/gadget/function/tcm.h
+++ b/drivers/usb/gadget/function/tcm.h
@@ -24,7 +24,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	UASP_SS_EP_COMP_NUM_STREAMS
 
 struct tcm_usbg_nexus {
 	struct se_session *tvn_se_sess;
-- 
2.28.0

[PATCH v3 10/28] usb: gadget: f_tcm: Get stream by sbitmap number

[Thinh Nguyen]

We prepare same number of sbitmap as the number of streams. Use the
returned sbitmap number as index to the selected stream for a usbg_cmd.

Signed-off-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com>
---
 drivers/usb/gadget/function/f_tcm.c | 26 ++++++--------------------
 drivers/usb/gadget/function/tcm.h   |  1 -
 2 files changed, 6 insertions(+), 21 deletions(-)

diff --git a/drivers/usb/gadget/function/f_tcm.c b/drivers/usb/gadget/function/f_tcm.c
index 1c93f07daa7b..a908bbd04b09 100644
--- a/drivers/usb/gadget/function/f_tcm.c
+++ b/drivers/usb/gadget/function/f_tcm.c
@@ -491,7 +491,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 = &fu->stream[se_cmd->map_tag];
 
 	if (!gadget->sg_supported) {
 		cmd->data_buf = kmalloc(se_cmd->data_length, GFP_ATOMIC);
@@ -523,7 +523,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 = &cmd->fu->stream[se_cmd->map_tag];
 
 	cmd->state = UASP_QUEUE_COMMAND;
 	iu->iu_id = IU_ID_STATUS;
@@ -544,8 +544,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 = &fu->stream[cmd->se_cmd.map_tag];
 	int ret;
 
 	if (req->status < 0)
@@ -595,7 +595,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 = &fu->stream[cmd->se_cmd.map_tag];
 	struct sense_iu *iu = &cmd->sense_iu;
 
 	iu->tag = cpu_to_be16(cmd->tag);
@@ -609,7 +609,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 = &fu->stream[cmd->se_cmd.map_tag];
 	struct sense_iu *iu = &cmd->sense_iu;
 	int ret;
 
@@ -653,7 +653,7 @@ 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 uas_stream *stream = &fu->stream[se_cmd->map_tag];
 	struct sense_iu *iu = &cmd->sense_iu;
 	int ret;
 
@@ -1104,17 +1104,6 @@ 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];
-	}
-
 	switch (cmd_iu->prio_attr & 0x7) {
 	case UAS_HEAD_TAG:
 		cmd->prio_attr = TCM_HEAD_TAG;
@@ -1140,9 +1129,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)
diff --git a/drivers/usb/gadget/function/tcm.h b/drivers/usb/gadget/function/tcm.h
index 385bc2cdefb6..cf469c19eaca 100644
--- a/drivers/usb/gadget/function/tcm.h
+++ b/drivers/usb/gadget/function/tcm.h
@@ -80,7 +80,6 @@ struct usbg_cmd {
 	u16 prio_attr;
 	struct sense_iu sense_iu;
 	enum uas_state state;
-	struct uas_stream *stream;
 
 	/* BOT only */
 	__le32 bot_tag;
-- 
2.28.0

[PATCH v3 11/28] 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>
---
 drivers/usb/gadget/function/f_tcm.c | 17 +++--------------
 1 file changed, 3 insertions(+), 14 deletions(-)

diff --git a/drivers/usb/gadget/function/f_tcm.c b/drivers/usb/gadget/function/f_tcm.c
index a908bbd04b09..a594ed1359fc 100644
--- a/drivers/usb/gadget/function/f_tcm.c
+++ b/drivers/usb/gadget/function/f_tcm.c
@@ -511,6 +511,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;
@@ -535,6 +536,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;
@@ -765,19 +767,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;
@@ -798,7 +787,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)
@@ -975,6 +963,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;
-- 
2.28.0

[PATCH v3 12/28] 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>
---
 drivers/usb/gadget/function/f_tcm.c | 107 ++++++++++++++--------------
 drivers/usb/gadget/function/tcm.h   |  10 ++-
 2 files changed, 61 insertions(+), 56 deletions(-)

diff --git a/drivers/usb/gadget/function/f_tcm.c b/drivers/usb/gadget/function/f_tcm.c
index a594ed1359fc..a3e886294c40 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);
@@ -297,8 +297,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);
@@ -310,27 +310,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;
@@ -355,16 +355,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)
@@ -461,10 +461,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)
@@ -479,7 +483,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);
 }
@@ -582,7 +586,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:
@@ -697,7 +702,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)
 {
@@ -707,7 +712,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
@@ -716,7 +721,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)
@@ -745,24 +750,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;
 }
@@ -771,26 +776,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;
 
@@ -1060,10 +1061,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;
@@ -1113,6 +1113,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 cf469c19eaca..cd8d06419d5f 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,
@@ -24,7 +26,7 @@ enum {
 #define USB_G_ALT_INT_BBB       0
 #define USB_G_ALT_INT_UAS       1
 
-#define USB_G_DEFAULT_SESSION_TAGS	UASP_SS_EP_COMP_NUM_STREAMS
+#define USB_G_DEFAULT_SESSION_TAGS	USBG_NUM_CMDS
 
 struct tcm_usbg_nexus {
 	struct se_session *tvn_se_sess;
@@ -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;
@@ -116,14 +120,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 v3 13/28] 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>
---
 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 a3e886294c40..50e6a41aaa82 100644
--- a/drivers/usb/gadget/function/f_tcm.c
+++ b/drivers/usb/gadget/function/f_tcm.c
@@ -707,21 +707,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)
@@ -1309,7 +1304,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 v3 14/28] 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>
---
 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 cd8d06419d5f..9d614a7f2ac0 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 v3 15/28] 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>
---
 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 50e6a41aaa82..f43fa964d2b5 100644
--- a/drivers/usb/gadget/function/f_tcm.c
+++ b/drivers/usb/gadget/function/f_tcm.c
@@ -877,6 +877,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 v3 16/28] 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. Queue the same work of the command to execute the
target_execute_cmd() on data write.

Signed-off-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com>
---
 drivers/usb/gadget/function/f_tcm.c | 39 +++++++++++++++++++++++------
 drivers/usb/gadget/function/tcm.h   |  4 ++-
 2 files changed, 34 insertions(+), 9 deletions(-)

diff --git a/drivers/usb/gadget/function/f_tcm.c b/drivers/usb/gadget/function/f_tcm.c
index f43fa964d2b5..50c0703e8df6 100644
--- a/drivers/usb/gadget/function/f_tcm.c
+++ b/drivers/usb/gadget/function/f_tcm.c
@@ -244,10 +244,8 @@ static int usbg_prepare_w_request(struct usbg_cmd *, struct usb_request *);
 static int bot_send_write_request(struct usbg_cmd *cmd)
 {
 	struct f_uas *fu = cmd->fu;
-	struct se_cmd *se_cmd = &cmd->se_cmd;
 	int ret;
 
-	init_completion(&cmd->write_complete);
 	cmd->fu = fu;
 
 	if (!cmd->data_len) {
@@ -262,8 +260,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;
 }
@@ -664,7 +660,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);
@@ -696,8 +691,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;
 }
@@ -922,6 +915,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;
@@ -934,7 +929,8 @@ static void usbg_data_write_cmpl(struct usb_ep *ep, struct usb_request *req)
 				se_cmd->data_length);
 	}
 
-	complete(&cmd->write_complete);
+	cmd->flags |= USBG_CMD_PENDING_DATA_WRITE;
+	queue_work(cmd->fu->tpg->workqueue, &cmd->work);
 	return;
 
 cleanup:
@@ -965,6 +961,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;
 }
 
@@ -1012,6 +1010,17 @@ static void usbg_cmd_work(struct work_struct *work)
 	struct usbg_tpg *tpg;
 	int dir, flags = (TARGET_SCF_UNKNOWN_SIZE | TARGET_SCF_ACK_KREF);
 
+	/*
+	 * Note: each command will spawn its own process, and each stage of the
+	 * command is processed sequentially. Should this no longer be the case,
+	 * locking is needed.
+	 */
+	if (cmd->flags & USBG_CMD_PENDING_DATA_WRITE) {
+		target_execute_cmd(&cmd->se_cmd);
+		cmd->flags &= ~USBG_CMD_PENDING_DATA_WRITE;
+		return;
+	}
+
 	se_cmd = &cmd->se_cmd;
 	tpg = cmd->fu->tpg;
 	tv_nexus = tpg->tpg_nexus;
@@ -1028,6 +1037,7 @@ static void usbg_cmd_work(struct work_struct *work)
 	target_submit_cmd(se_cmd, tv_nexus->tvn_se_sess, cmd->cmd_buf,
 			  cmd->sense_iu.sense, cmd->unpacked_lun, 0,
 			  cmd->prio_attr, dir, flags);
+
 	return;
 
 out:
@@ -1111,6 +1121,7 @@ static int usbg_submit_command(struct f_uas *fu, struct usb_request *req)
 
 	cmd->unpacked_lun = scsilun_to_int(&cmd_iu->lun);
 	cmd->req = req;
+	cmd->flags = 0;
 
 	INIT_WORK(&cmd->work, usbg_cmd_work);
 	queue_work(tpg->workqueue, &cmd->work);
@@ -1126,6 +1137,17 @@ static void bot_cmd_work(struct work_struct *work)
 	struct usbg_tpg *tpg;
 	int dir;
 
+	/*
+	 * Note: each command will spawn its own process, and each stage of the
+	 * command is processed sequentially. Should this no longer be the case,
+	 * locking is needed.
+	 */
+	if (cmd->flags & USBG_CMD_PENDING_DATA_WRITE) {
+		target_execute_cmd(&cmd->se_cmd);
+		cmd->flags &= ~USBG_CMD_PENDING_DATA_WRITE;
+		return;
+	}
+
 	se_cmd = &cmd->se_cmd;
 	tpg = cmd->fu->tpg;
 	tv_nexus = tpg->tpg_nexus;
@@ -1190,6 +1212,7 @@ static int bot_submit_command(struct f_uas *fu,
 	cmd->is_read = cbw->Flags & US_BULK_FLAG_IN ? 1 : 0;
 	cmd->data_len = le32_to_cpu(cbw->DataTransferLength);
 	cmd->se_cmd.tag = le32_to_cpu(cmd->bot_tag);
+	cmd->flags = 0;
 
 	INIT_WORK(&cmd->work, bot_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 9d614a7f2ac0..adf4c415140f 100644
--- a/drivers/usb/gadget/function/tcm.h
+++ b/drivers/usb/gadget/function/tcm.h
@@ -74,11 +74,13 @@ 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;
 
+	u32 flags;
+#define USBG_CMD_PENDING_DATA_WRITE	BIT(0)
+
 	/* UAS only */
 	u16 tag;
 	u16 prio_attr;
-- 
2.28.0

[PATCH v3 17/28] 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>
---
 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 50c0703e8df6..696d34e04e7d 100644
--- a/drivers/usb/gadget/function/f_tcm.c
+++ b/drivers/usb/gadget/function/f_tcm.c
@@ -602,8 +602,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 v3 18/28] usb: gadget: f_tcm: Handle abort command

[Thinh Nguyen]

Implement usbg_aborted_task() to cancel aborted outstanding requests.

Signed-off-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com>
---
 drivers/usb/gadget/function/f_tcm.c | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/drivers/usb/gadget/function/f_tcm.c b/drivers/usb/gadget/function/f_tcm.c
index 696d34e04e7d..2a74414c7fd1 100644
--- a/drivers/usb/gadget/function/f_tcm.c
+++ b/drivers/usb/gadget/function/f_tcm.c
@@ -1257,6 +1257,24 @@ 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 usb_gadget *gadget = fuas_to_gadget(fu);
+	struct uas_stream *stream = &fu->stream[se_cmd->map_tag];
+	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)
+		dev_err(&gadget->dev, "Failed to abort cmd tag %d, (%d)\n",
+			cmd->tag, ret);
+
+	cmd->state = UASP_QUEUE_COMMAND;
 }
 
 static const char *usbg_check_wwn(const char *name)
-- 
2.28.0

[PATCH v3 19/28] usb: gadget: f_tcm: Cleanup requests on ep disable

[Thinh Nguyen]

There may be different reasons for the transfer to be cancelled. Don't
blindly free the command without checking its status. We may still need
to properly respond to cancelled command. Check and only free the
command on endpoint disable.

Signed-off-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com>
---
 drivers/usb/gadget/function/f_tcm.c | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/gadget/function/f_tcm.c b/drivers/usb/gadget/function/f_tcm.c
index 2a74414c7fd1..8a5aa58e166e 100644
--- a/drivers/usb/gadget/function/f_tcm.c
+++ b/drivers/usb/gadget/function/f_tcm.c
@@ -196,6 +196,11 @@ static void bot_read_compl(struct usb_ep *ep, struct usb_request *req)
 	if (req->status < 0)
 		pr_err("ERR %s(%d)\n", __func__, __LINE__);
 
+	if (req->status == -ESHUTDOWN) {
+		transport_generic_free_cmd(&cmd->se_cmd, 0);
+		return;
+	}
+
 	bot_send_status(cmd, true);
 }
 
@@ -550,7 +555,7 @@ static void uasp_status_data_cmpl(struct usb_ep *ep, struct usb_request *req)
 	struct uas_stream *stream = &fu->stream[cmd->se_cmd.map_tag];
 	int ret;
 
-	if (req->status < 0)
+	if (req->status == -ESHUTDOWN)
 		goto cleanup;
 
 	switch (cmd->state) {
@@ -915,7 +920,13 @@ 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) {
+		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);
 		goto cleanup;
 	}
-- 
2.28.0

[PATCH v3 20/28] usb: gadget: f_tcm: Stop proceeding further on -ESHUTDOWN

[Thinh Nguyen]

If the error code is -ESHUTDOWN, stop processing the request/command
further and prepare for teardown. -ESHUTDOWN is for device reset or
disconnection.

Signed-off-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com>
---
 drivers/usb/gadget/function/f_tcm.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/drivers/usb/gadget/function/f_tcm.c b/drivers/usb/gadget/function/f_tcm.c
index 8a5aa58e166e..6aa341c1472a 100644
--- a/drivers/usb/gadget/function/f_tcm.c
+++ b/drivers/usb/gadget/function/f_tcm.c
@@ -62,10 +62,11 @@ static void bot_status_complete(struct usb_ep *ep, struct usb_request *req)
 	struct f_uas *fu = cmd->fu;
 
 	transport_generic_free_cmd(&cmd->se_cmd, 0);
-	if (req->status < 0) {
-		pr_err("ERR %s(%d)\n", __func__, __LINE__);
+	if (req->status == -ESHUTDOWN)
 		return;
-	}
+
+	if (req->status < 0)
+		pr_err("ERR %s(%d)\n", __func__, __LINE__);
 
 	/* CSW completed, wait for next CBW */
 	bot_enqueue_cmd_cbw(fu);
@@ -276,6 +277,9 @@ static void bot_cmd_complete(struct usb_ep *ep, struct usb_request *req)
 	struct f_uas *fu = req->context;
 	int ret;
 
+	if (req->status == -ESHUTDOWN)
+		return;
+
 	fu->flags &= ~USBG_BOT_CMD_PEND;
 
 	if (req->status < 0)
-- 
2.28.0

[PATCH v3 21/28] 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>
---
 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 6aa341c1472a..da594767ba98 100644
--- a/drivers/usb/gadget/function/f_tcm.c
+++ b/drivers/usb/gadget/function/f_tcm.c
@@ -1073,6 +1073,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 v3 22/28] 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>
---
 drivers/usb/gadget/function/f_tcm.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/gadget/function/f_tcm.c b/drivers/usb/gadget/function/f_tcm.c
index da594767ba98..c6bdd6023588 100644
--- a/drivers/usb/gadget/function/f_tcm.c
+++ b/drivers/usb/gadget/function/f_tcm.c
@@ -948,7 +948,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 v3 23/28] 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>
---
 drivers/usb/gadget/function/f_tcm.c | 220 +++++++++++++++++++++++++---
 drivers/usb/gadget/function/tcm.h   |   5 +
 2 files changed, 206 insertions(+), 19 deletions(-)

diff --git a/drivers/usb/gadget/function/f_tcm.c b/drivers/usb/gadget/function/f_tcm.c
index c6bdd6023588..3e04ce40a4a0 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>
@@ -449,6 +450,45 @@ 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_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_CLEAR_ACA:
+		return TMR_CLEAR_ACA;
+	case TMF_I_T_NEXUS_RESET:
+	case TMF_QUERY_TASK:
+	case TMF_QUERY_TASK_SET:
+	case TMF_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 */
@@ -552,6 +592,61 @@ 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 = &cmd->fu->stream[se_cmd->map_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);
+
+	/*
+	 * 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 TMF_ABORT_TASK:
+	case TMF_ABORT_TASK_SET:
+	case TMF_CLEAR_TASK_SET:
+	case TMF_LOGICAL_UNIT_RESET:
+	case TMF_CLEAR_ACA:
+	case TMF_I_T_NEXUS_RESET:
+	case TMF_QUERY_TASK:
+	case TMF_QUERY_TASK_SET:
+	case TMF_QUERY_ASYNC_EVENT:
+		if (rsp_iu->response_code == RC_TMF_NOT_SUPPORTED) {
+			struct usb_gadget *gadget = fuas_to_gadget(cmd->fu);
+
+			dev_warn(&gadget->dev, "TMF function %d not supported\n",
+				 cmd->tmr_func);
+			rsp_iu->response_code = RC_TMF_FAILED;
+		}
+		break;
+	default:
+		break;
+	}
+
+	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;
@@ -590,9 +685,23 @@ 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);
+		/*
+		 * 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:
@@ -616,6 +725,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 = &fu->stream[cmd->se_cmd.map_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;
@@ -1016,9 +1137,23 @@ static int usbg_send_read_response(struct se_cmd *se_cmd)
 		return uasp_send_read_response(cmd);
 }
 
-static void usbg_cmd_work(struct work_struct *work)
+static void usbg_submit_tmr(struct usbg_cmd *cmd)
+{
+	struct se_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, uasp_to_tcm_func(cmd->tmr_func),
+			  GFP_ATOMIC, cmd->tag, flags);
+}
+
+static void usbg_submit_cmd(struct usbg_cmd *cmd)
 {
-	struct usbg_cmd *cmd = container_of(work, struct usbg_cmd, work);
 	struct se_cmd *se_cmd;
 	struct tcm_usbg_nexus *tv_nexus;
 	struct usbg_tpg *tpg;
@@ -1059,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)
 {
@@ -1085,34 +1243,58 @@ 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 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;
 	}
+
+	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->flags = 0;
+
+	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;
+	}
+
+	if (iu->iu_id == IU_ID_TASK_MGMT) {
+		struct task_mgmt_iu *tm_iu;
+
+		tm_iu = (struct task_mgmt_iu *)iu;
+		cmd->tmr_func = tm_iu->function;
+		goto skip;
+	}
+
+	cmd_len = (cmd_iu->len & ~0x3) + 16;
+	if (cmd_len > USBG_MAX_CMD) {
+		target_free_tag(tv_nexus->tvn_se_sess, &cmd->se_cmd);
+		return -EINVAL;
+	}
 	memcpy(cmd->cmd_buf, cmd_iu->cdb, cmd_len);
 
 	switch (cmd_iu->prio_attr & 0x7) {
@@ -1134,10 +1316,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;
-	cmd->flags = 0;
-
+skip:
 	INIT_WORK(&cmd->work, usbg_cmd_work);
 	queue_work(tpg->workqueue, &cmd->work);
 
@@ -1270,6 +1449,9 @@ static void usbg_release_cmd(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 adf4c415140f..d37358f09819 100644
--- a/drivers/usb/gadget/function/tcm.h
+++ b/drivers/usb/gadget/function/tcm.h
@@ -85,8 +85,13 @@ struct usbg_cmd {
 	u16 tag;
 	u16 prio_attr;
 	struct sense_iu sense_iu;
+	struct response_iu response_iu;
 	enum uas_state state;
 
+	int tmr_func;
+	int tmr_rsp;
+#define	RC_RESPONSE_UNKNOWN	0xff
+
 	/* BOT only */
 	__le32 bot_tag;
 	unsigned int csw_code;
-- 
2.28.0

[PATCH v3 24/28] 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>
---
 drivers/usb/gadget/function/f_tcm.c | 123 +++++++++++++++++++++++++++-
 drivers/usb/gadget/function/tcm.h   |   5 ++
 2 files changed, 127 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/gadget/function/f_tcm.c b/drivers/usb/gadget/function/f_tcm.c
index 3e04ce40a4a0..0c7a41568f40 100644
--- a/drivers/usb/gadget/function/f_tcm.c
+++ b/drivers/usb/gadget/function/f_tcm.c
@@ -685,12 +685,25 @@ 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;
+		}
+
+		hash_del(&stream->node);
+
 		/*
 		 * 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) {
+		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;
@@ -702,6 +715,7 @@ static void uasp_status_data_cmpl(struct usb_ep *ep, struct usb_request *req)
 		}
 
 		usb_ep_queue(fu->ep_cmd, cmd->req, GFP_ATOMIC);
+		complete(&stream->cmd_completion);
 		break;
 
 	default:
@@ -710,6 +724,7 @@ static void uasp_status_data_cmpl(struct usb_ep *ep, struct usb_request *req)
 	return;
 
 cleanup:
+	hash_del(&stream->node);
 	transport_generic_free_cmd(&cmd->se_cmd, 0);
 }
 
@@ -842,6 +857,8 @@ static void uasp_cmd_complete(struct usb_ep *ep, struct usb_request *req)
 
 static int uasp_alloc_stream_res(struct f_uas *fu, struct uas_stream *stream)
 {
+	init_completion(&stream->cmd_completion);
+
 	stream->req_in = usb_ep_alloc_request(fu->ep_in, GFP_KERNEL);
 	if (!stream->req_in)
 		goto out;
@@ -1046,6 +1063,9 @@ static void usbg_data_write_cmpl(struct usb_ep *ep, struct usb_request *req)
 	cmd->state = UASP_QUEUE_COMMAND;
 
 	if (req->status == -ESHUTDOWN) {
+		struct uas_stream *stream = &cmd->fu->stream[se_cmd->map_tag];
+
+		hash_del(&stream->node);
 		target_put_sess_cmd(se_cmd);
 		transport_generic_free_cmd(&cmd->se_cmd, 0);
 		return;
@@ -1069,6 +1089,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);
 }
@@ -1137,6 +1165,8 @@ 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 se_session *se_sess;
@@ -1214,6 +1244,74 @@ static void usbg_cmd_work(struct work_struct *work)
 	return;
 
 skip:
+	if (cmd->tmr_rsp == RC_OVERLAPPED_TAG) {
+		struct f_uas *fu = cmd->fu;
+		struct se_session *se_sess;
+		struct uas_stream *stream = NULL;
+		struct hlist_node *tmp;
+		struct usbg_cmd *active_cmd = NULL;
+
+		se_sess = cmd->fu->tpg->tpg_nexus->tvn_se_sess;
+
+		hash_for_each_possible_safe(fu->stream_hash, stream, tmp, node, cmd->tag) {
+			int i = stream - &fu->stream[0];
+
+			active_cmd = &((struct usbg_cmd *)se_sess->sess_cmd_map)[i];
+			if (active_cmd->tag == cmd->tag)
+				break;
+		}
+
+		/* Sanity check */
+		if (!stream || (active_cmd && active_cmd->tag != cmd->tag)) {
+			usbg_submit_command(cmd->fu, cmd->req);
+			return;
+		}
+
+		reinit_completion(&stream->cmd_completion);
+
+		/*
+		 * A UASP command consists of the command, data, and status
+		 * stages, each operating sequentially from different endpoints.
+		 *
+		 * Each USB endpoint operates independently, and depending on
+		 * hardware implementation, a completion callback for a transfer
+		 * from one endpoint may not reflect the order of completion on
+		 * the wire. This is particularly true for devices with
+		 * endpoints that have independent interrupts and event buffers.
+		 *
+		 * The driver must still detect misbehaving hosts and respond
+		 * with an overlap status. To reduce false overlap failures,
+		 * allow the active and matching stream ID a brief 1ms to
+		 * complete before responding with an overlap command failure.
+		 * Overlap failure should be rare.
+		 */
+		wait_for_completion_timeout(&stream->cmd_completion, msecs_to_jiffies(1));
+
+		/* If the previous stream is completed, retry the command. */
+		if (!hash_hashed(&stream->node)) {
+			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.
+		 */
+		active_cmd->tmr_rsp = RC_OVERLAPPED_TAG;
+		usbg_aborted_task(&active_cmd->se_cmd);
+
+		/* Send the response after the transfer is aborted. */
+		return;
+	}
+
 	uasp_send_tm_response(cmd);
 }
 
@@ -1247,6 +1345,8 @@ static int usbg_submit_command(struct f_uas *fu, struct usb_request *req)
 	struct usbg_cmd *cmd;
 	struct usbg_tpg *tpg = fu->tpg;
 	struct tcm_usbg_nexus *tv_nexus;
+	struct uas_stream *stream;
+	struct hlist_node *tmp;
 	struct command_iu *cmd_iu;
 	u32 cmd_len;
 	u16 scsi_tag;
@@ -1282,6 +1382,23 @@ static int usbg_submit_command(struct f_uas *fu, struct usb_request *req)
 		goto skip;
 	}
 
+	hash_for_each_possible_safe(fu->stream_hash, stream, tmp, node, scsi_tag) {
+		struct usbg_cmd *active_cmd;
+		struct se_session *se_sess;
+		int i = stream - &fu->stream[0];
+
+		se_sess = cmd->fu->tpg->tpg_nexus->tvn_se_sess;
+		active_cmd = &((struct usbg_cmd *)se_sess->sess_cmd_map)[i];
+
+		if (active_cmd->tag == scsi_tag) {
+			cmd->tmr_rsp = RC_OVERLAPPED_TAG;
+			goto skip;
+		}
+	}
+
+	stream = &fu->stream[cmd->se_cmd.map_tag];
+	hash_add(fu->stream_hash, &stream->node, scsi_tag);
+
 	if (iu->iu_id == IU_ID_TASK_MGMT) {
 		struct task_mgmt_iu *tm_iu;
 
@@ -1293,6 +1410,7 @@ static int usbg_submit_command(struct f_uas *fu, struct usb_request *req)
 	cmd_len = (cmd_iu->len & ~0x3) + 16;
 	if (cmd_len > USBG_MAX_CMD) {
 		target_free_tag(tv_nexus->tvn_se_sess, &cmd->se_cmd);
+		hash_del(&stream->node);
 		return -EINVAL;
 	}
 	memcpy(cmd->cmd_buf, cmd_iu->cdb, cmd_len);
@@ -1443,6 +1561,7 @@ static void usbg_release_cmd(struct se_cmd *se_cmd)
 			se_cmd);
 	struct se_session *se_sess = se_cmd->se_sess;
 
+	cmd->tag = 0;
 	kfree(cmd->data_buf);
 	target_free_tag(se_sess, se_cmd);
 }
@@ -2467,6 +2586,8 @@ static struct usb_function *tcm_alloc(struct usb_function_instance *fi)
 	fu->function.disable = tcm_disable;
 	fu->function.free_func = tcm_free;
 	fu->tpg = tpg_instances[i].tpg;
+
+	hash_init(fu->stream_hash);
 	mutex_unlock(&tpg_instances_lock);
 
 	return &fu->function;
diff --git a/drivers/usb/gadget/function/tcm.h b/drivers/usb/gadget/function/tcm.h
index d37358f09819..f6d6c86d10b3 100644
--- a/drivers/usb/gadget/function/tcm.h
+++ b/drivers/usb/gadget/function/tcm.h
@@ -4,6 +4,7 @@
 
 #include <linux/kref.h>
 /* #include <linux/usb/uas.h> */
+#include <linux/hashtable.h>
 #include <linux/usb/composite.h>
 #include <linux/usb/uas.h>
 #include <linux/usb/storage.h>
@@ -103,6 +104,9 @@ struct uas_stream {
 	struct usb_request	*req_in;
 	struct usb_request	*req_out;
 	struct usb_request	*req_status;
+
+	struct completion	cmd_completion;
+	struct hlist_node	node;
 };
 
 struct usbg_cdb {
@@ -135,6 +139,7 @@ struct f_uas {
 	struct usb_ep		*ep_status;
 	struct usb_ep		*ep_cmd;
 	struct uas_stream	stream[USBG_NUM_CMDS];
+	DECLARE_HASHTABLE(stream_hash, UASP_SS_EP_COMP_LOG_STREAMS);
 
 	/* BOT */
 	struct bot_status	bot_status;
-- 
2.28.0

Re: [PATCH v3 24/28] usb: gadget: f_tcm: Check overlapped command

[Dan Carpenter]

Hi Thinh,

kernel test robot noticed the following build warnings:

url:    https://github.com/intel-lab-lkp/linux/commits/Thinh-Nguyen/usb-gadget-f_tcm-Don-t-free-command-immediately/20241211-092317
base:   d8d936c51388442f769a81e512b505dcf87c6a51
patch link:    https://lore.kernel.org/r/6bffc2903d0cd1e7c7afca837053a48e883d8903.1733876548.git.Thinh.Nguyen%40synopsys.com
patch subject: [PATCH v3 24/28] usb: gadget: f_tcm: Check overlapped command
config: nios2-randconfig-r071-20241219 (https://download.01.org/0day-ci/archive/20241219/202412192132.XB16SilM-lkp@intel.com/config)
compiler: nios2-linux-gcc (GCC) 14.2.0

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
| Closes: https://lore.kernel.org/r/202412192132.XB16SilM-lkp@intel.com/

smatch warnings:
drivers/usb/gadget/function/f_tcm.c:1308 usbg_cmd_work() error: we previously assumed 'active_cmd' could be null (see line 1265)

vim +/active_cmd +1308 drivers/usb/gadget/function/f_tcm.c

287b3d115e5351 Thinh Nguyen          2024-12-11  1227  static void usbg_cmd_work(struct work_struct *work)
287b3d115e5351 Thinh Nguyen          2024-12-11  1228  {
287b3d115e5351 Thinh Nguyen          2024-12-11  1229  	struct usbg_cmd *cmd = container_of(work, struct usbg_cmd, work);
287b3d115e5351 Thinh Nguyen          2024-12-11  1230  
287b3d115e5351 Thinh Nguyen          2024-12-11  1231  	/*
287b3d115e5351 Thinh Nguyen          2024-12-11  1232  	 * Failure is detected by f_tcm here. Skip submitting the command to the
287b3d115e5351 Thinh Nguyen          2024-12-11  1233  	 * target core if we already know the failing response and send the usb
287b3d115e5351 Thinh Nguyen          2024-12-11  1234  	 * response to the host directly.
287b3d115e5351 Thinh Nguyen          2024-12-11  1235  	 */
287b3d115e5351 Thinh Nguyen          2024-12-11  1236  	if (cmd->tmr_rsp != RC_RESPONSE_UNKNOWN)
287b3d115e5351 Thinh Nguyen          2024-12-11  1237  		goto skip;
287b3d115e5351 Thinh Nguyen          2024-12-11  1238  
287b3d115e5351 Thinh Nguyen          2024-12-11  1239  	if (cmd->tmr_func)
287b3d115e5351 Thinh Nguyen          2024-12-11  1240  		usbg_submit_tmr(cmd);
287b3d115e5351 Thinh Nguyen          2024-12-11  1241  	else
287b3d115e5351 Thinh Nguyen          2024-12-11  1242  		usbg_submit_cmd(cmd);
287b3d115e5351 Thinh Nguyen          2024-12-11  1243  
287b3d115e5351 Thinh Nguyen          2024-12-11  1244  	return;
287b3d115e5351 Thinh Nguyen          2024-12-11  1245  
287b3d115e5351 Thinh Nguyen          2024-12-11  1246  skip:
7735c10c74d903 Thinh Nguyen          2024-12-11  1247  	if (cmd->tmr_rsp == RC_OVERLAPPED_TAG) {
7735c10c74d903 Thinh Nguyen          2024-12-11  1248  		struct f_uas *fu = cmd->fu;
7735c10c74d903 Thinh Nguyen          2024-12-11  1249  		struct se_session *se_sess;
7735c10c74d903 Thinh Nguyen          2024-12-11  1250  		struct uas_stream *stream = NULL;
7735c10c74d903 Thinh Nguyen          2024-12-11  1251  		struct hlist_node *tmp;
7735c10c74d903 Thinh Nguyen          2024-12-11  1252  		struct usbg_cmd *active_cmd = NULL;
7735c10c74d903 Thinh Nguyen          2024-12-11  1253  
7735c10c74d903 Thinh Nguyen          2024-12-11  1254  		se_sess = cmd->fu->tpg->tpg_nexus->tvn_se_sess;
7735c10c74d903 Thinh Nguyen          2024-12-11  1255  
7735c10c74d903 Thinh Nguyen          2024-12-11  1256  		hash_for_each_possible_safe(fu->stream_hash, stream, tmp, node, cmd->tag) {
7735c10c74d903 Thinh Nguyen          2024-12-11  1257  			int i = stream - &fu->stream[0];
7735c10c74d903 Thinh Nguyen          2024-12-11  1258  
7735c10c74d903 Thinh Nguyen          2024-12-11  1259  			active_cmd = &((struct usbg_cmd *)se_sess->sess_cmd_map)[i];
7735c10c74d903 Thinh Nguyen          2024-12-11  1260  			if (active_cmd->tag == cmd->tag)
7735c10c74d903 Thinh Nguyen          2024-12-11  1261  				break;
7735c10c74d903 Thinh Nguyen          2024-12-11  1262  		}
7735c10c74d903 Thinh Nguyen          2024-12-11  1263  
7735c10c74d903 Thinh Nguyen          2024-12-11  1264  		/* Sanity check */
7735c10c74d903 Thinh Nguyen          2024-12-11 @1265  		if (!stream || (active_cmd && active_cmd->tag != cmd->tag)) {

Testing for !stream is sufficient.  Another option would be to write this
as:
	if (!stream || !active_cmd || active_cmd->tag != cmd->tag)) {

7735c10c74d903 Thinh Nguyen          2024-12-11  1266  			usbg_submit_command(cmd->fu, cmd->req);
7735c10c74d903 Thinh Nguyen          2024-12-11  1267  			return;
7735c10c74d903 Thinh Nguyen          2024-12-11  1268  		}
7735c10c74d903 Thinh Nguyen          2024-12-11  1269  
7735c10c74d903 Thinh Nguyen          2024-12-11  1270  		reinit_completion(&stream->cmd_completion);
7735c10c74d903 Thinh Nguyen          2024-12-11  1271  
7735c10c74d903 Thinh Nguyen          2024-12-11  1272  		/*
7735c10c74d903 Thinh Nguyen          2024-12-11  1273  		 * A UASP command consists of the command, data, and status
7735c10c74d903 Thinh Nguyen          2024-12-11  1274  		 * stages, each operating sequentially from different endpoints.
7735c10c74d903 Thinh Nguyen          2024-12-11  1275  		 *
7735c10c74d903 Thinh Nguyen          2024-12-11  1276  		 * Each USB endpoint operates independently, and depending on
7735c10c74d903 Thinh Nguyen          2024-12-11  1277  		 * hardware implementation, a completion callback for a transfer
7735c10c74d903 Thinh Nguyen          2024-12-11  1278  		 * from one endpoint may not reflect the order of completion on
7735c10c74d903 Thinh Nguyen          2024-12-11  1279  		 * the wire. This is particularly true for devices with
7735c10c74d903 Thinh Nguyen          2024-12-11  1280  		 * endpoints that have independent interrupts and event buffers.
7735c10c74d903 Thinh Nguyen          2024-12-11  1281  		 *
7735c10c74d903 Thinh Nguyen          2024-12-11  1282  		 * The driver must still detect misbehaving hosts and respond
7735c10c74d903 Thinh Nguyen          2024-12-11  1283  		 * with an overlap status. To reduce false overlap failures,
7735c10c74d903 Thinh Nguyen          2024-12-11  1284  		 * allow the active and matching stream ID a brief 1ms to
7735c10c74d903 Thinh Nguyen          2024-12-11  1285  		 * complete before responding with an overlap command failure.
7735c10c74d903 Thinh Nguyen          2024-12-11  1286  		 * Overlap failure should be rare.
7735c10c74d903 Thinh Nguyen          2024-12-11  1287  		 */
7735c10c74d903 Thinh Nguyen          2024-12-11  1288  		wait_for_completion_timeout(&stream->cmd_completion, msecs_to_jiffies(1));
7735c10c74d903 Thinh Nguyen          2024-12-11  1289  
7735c10c74d903 Thinh Nguyen          2024-12-11  1290  		/* If the previous stream is completed, retry the command. */
7735c10c74d903 Thinh Nguyen          2024-12-11  1291  		if (!hash_hashed(&stream->node)) {
7735c10c74d903 Thinh Nguyen          2024-12-11  1292  			usbg_submit_command(cmd->fu, cmd->req);
7735c10c74d903 Thinh Nguyen          2024-12-11  1293  			return;
7735c10c74d903 Thinh Nguyen          2024-12-11  1294  		}
7735c10c74d903 Thinh Nguyen          2024-12-11  1295  
7735c10c74d903 Thinh Nguyen          2024-12-11  1296  		/*
7735c10c74d903 Thinh Nguyen          2024-12-11  1297  		 * The command isn't submitted to the target core, so we're safe
7735c10c74d903 Thinh Nguyen          2024-12-11  1298  		 * to remove the bitmap index from the session tag pool.
7735c10c74d903 Thinh Nguyen          2024-12-11  1299  		 */
7735c10c74d903 Thinh Nguyen          2024-12-11  1300  		sbitmap_queue_clear(&se_sess->sess_tag_pool,
7735c10c74d903 Thinh Nguyen          2024-12-11  1301  				    cmd->se_cmd.map_tag,
7735c10c74d903 Thinh Nguyen          2024-12-11  1302  				    cmd->se_cmd.map_cpu);
7735c10c74d903 Thinh Nguyen          2024-12-11  1303  
7735c10c74d903 Thinh Nguyen          2024-12-11  1304  		/*
7735c10c74d903 Thinh Nguyen          2024-12-11  1305  		 * Overlap command tag detected. Cancel any pending transfer of
7735c10c74d903 Thinh Nguyen          2024-12-11  1306  		 * the command submitted to target core.
7735c10c74d903 Thinh Nguyen          2024-12-11  1307  		 */
7735c10c74d903 Thinh Nguyen          2024-12-11 @1308  		active_cmd->tmr_rsp = RC_OVERLAPPED_TAG;

The inconsistent NULL check triggers a warning here.

7735c10c74d903 Thinh Nguyen          2024-12-11  1309  		usbg_aborted_task(&active_cmd->se_cmd);
7735c10c74d903 Thinh Nguyen          2024-12-11  1310  
7735c10c74d903 Thinh Nguyen          2024-12-11  1311  		/* Send the response after the transfer is aborted. */
7735c10c74d903 Thinh Nguyen          2024-12-11  1312  		return;
7735c10c74d903 Thinh Nguyen          2024-12-11  1313  	}
7735c10c74d903 Thinh Nguyen          2024-12-11  1314  
287b3d115e5351 Thinh Nguyen          2024-12-11  1315  	uasp_send_tm_response(cmd);
287b3d115e5351 Thinh Nguyen          2024-12-11  1316  }

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH v3 24/28] usb: gadget: f_tcm: Check overlapped command

[Thinh Nguyen]

On Thu, Dec 19, 2024, Dan Carpenter wrote:
> Hi Thinh,
> 
> kernel test robot noticed the following build warnings:
> 
> url:    https://urldefense.com/v3/__https://github.com/intel-lab-lkp/linux/commits/Thinh-Nguyen/usb-gadget-f_tcm-Don-t-free-command-immediately/20241211-092317__;!!A4F2R9G_pg!YKeQa8JmJaKHAak1XzUO0sLWgipnVS9vCGr4PiZb8TEMwYAnaMG4XVSZ3aeoCV-54D_6YX6ylxj93-NYCMU04FiCU0xX$ 
> base:   d8d936c51388442f769a81e512b505dcf87c6a51
> patch link:    https://urldefense.com/v3/__https://lore.kernel.org/r/6bffc2903d0cd1e7c7afca837053a48e883d8903.1733876548.git.Thinh.Nguyen*40synopsys.com__;JQ!!A4F2R9G_pg!YKeQa8JmJaKHAak1XzUO0sLWgipnVS9vCGr4PiZb8TEMwYAnaMG4XVSZ3aeoCV-54D_6YX6ylxj93-NYCMU04BGCyVAg$ 
> patch subject: [PATCH v3 24/28] usb: gadget: f_tcm: Check overlapped command
> config: nios2-randconfig-r071-20241219 (https://urldefense.com/v3/__https://download.01.org/0day-ci/archive/20241219/202412192132.XB16SilM-lkp@intel.com/config__;!!A4F2R9G_pg!YKeQa8JmJaKHAak1XzUO0sLWgipnVS9vCGr4PiZb8TEMwYAnaMG4XVSZ3aeoCV-54D_6YX6ylxj93-NYCMU04JujRj-r$ )
> compiler: nios2-linux-gcc (GCC) 14.2.0
> 
> If you fix the issue in a separate patch/commit (i.e. not just a new version of
> the same patch/commit), kindly add following tags
> | Reported-by: kernel test robot <lkp@intel.com>
> | Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
> | Closes: https://urldefense.com/v3/__https://lore.kernel.org/r/202412192132.XB16SilM-lkp@intel.com/__;!!A4F2R9G_pg!YKeQa8JmJaKHAak1XzUO0sLWgipnVS9vCGr4PiZb8TEMwYAnaMG4XVSZ3aeoCV-54D_6YX6ylxj93-NYCMU04FWUkKjj$ 
> 
> smatch warnings:
> drivers/usb/gadget/function/f_tcm.c:1308 usbg_cmd_work() error: we previously assumed 'active_cmd' could be null (see line 1265)
> 
> vim +/active_cmd +1308 drivers/usb/gadget/function/f_tcm.c
> 
> 287b3d115e5351 Thinh Nguyen          2024-12-11  1227  static void usbg_cmd_work(struct work_struct *work)
> 287b3d115e5351 Thinh Nguyen          2024-12-11  1228  {
> 287b3d115e5351 Thinh Nguyen          2024-12-11  1229  	struct usbg_cmd *cmd = container_of(work, struct usbg_cmd, work);
> 287b3d115e5351 Thinh Nguyen          2024-12-11  1230  
> 287b3d115e5351 Thinh Nguyen          2024-12-11  1231  	/*
> 287b3d115e5351 Thinh Nguyen          2024-12-11  1232  	 * Failure is detected by f_tcm here. Skip submitting the command to the
> 287b3d115e5351 Thinh Nguyen          2024-12-11  1233  	 * target core if we already know the failing response and send the usb
> 287b3d115e5351 Thinh Nguyen          2024-12-11  1234  	 * response to the host directly.
> 287b3d115e5351 Thinh Nguyen          2024-12-11  1235  	 */
> 287b3d115e5351 Thinh Nguyen          2024-12-11  1236  	if (cmd->tmr_rsp != RC_RESPONSE_UNKNOWN)
> 287b3d115e5351 Thinh Nguyen          2024-12-11  1237  		goto skip;
> 287b3d115e5351 Thinh Nguyen          2024-12-11  1238  
> 287b3d115e5351 Thinh Nguyen          2024-12-11  1239  	if (cmd->tmr_func)
> 287b3d115e5351 Thinh Nguyen          2024-12-11  1240  		usbg_submit_tmr(cmd);
> 287b3d115e5351 Thinh Nguyen          2024-12-11  1241  	else
> 287b3d115e5351 Thinh Nguyen          2024-12-11  1242  		usbg_submit_cmd(cmd);
> 287b3d115e5351 Thinh Nguyen          2024-12-11  1243  
> 287b3d115e5351 Thinh Nguyen          2024-12-11  1244  	return;
> 287b3d115e5351 Thinh Nguyen          2024-12-11  1245  
> 287b3d115e5351 Thinh Nguyen          2024-12-11  1246  skip:
> 7735c10c74d903 Thinh Nguyen          2024-12-11  1247  	if (cmd->tmr_rsp == RC_OVERLAPPED_TAG) {
> 7735c10c74d903 Thinh Nguyen          2024-12-11  1248  		struct f_uas *fu = cmd->fu;
> 7735c10c74d903 Thinh Nguyen          2024-12-11  1249  		struct se_session *se_sess;
> 7735c10c74d903 Thinh Nguyen          2024-12-11  1250  		struct uas_stream *stream = NULL;
> 7735c10c74d903 Thinh Nguyen          2024-12-11  1251  		struct hlist_node *tmp;
> 7735c10c74d903 Thinh Nguyen          2024-12-11  1252  		struct usbg_cmd *active_cmd = NULL;
> 7735c10c74d903 Thinh Nguyen          2024-12-11  1253  
> 7735c10c74d903 Thinh Nguyen          2024-12-11  1254  		se_sess = cmd->fu->tpg->tpg_nexus->tvn_se_sess;
> 7735c10c74d903 Thinh Nguyen          2024-12-11  1255  
> 7735c10c74d903 Thinh Nguyen          2024-12-11  1256  		hash_for_each_possible_safe(fu->stream_hash, stream, tmp, node, cmd->tag) {
> 7735c10c74d903 Thinh Nguyen          2024-12-11  1257  			int i = stream - &fu->stream[0];
> 7735c10c74d903 Thinh Nguyen          2024-12-11  1258  
> 7735c10c74d903 Thinh Nguyen          2024-12-11  1259  			active_cmd = &((struct usbg_cmd *)se_sess->sess_cmd_map)[i];
> 7735c10c74d903 Thinh Nguyen          2024-12-11  1260  			if (active_cmd->tag == cmd->tag)
> 7735c10c74d903 Thinh Nguyen          2024-12-11  1261  				break;
> 7735c10c74d903 Thinh Nguyen          2024-12-11  1262  		}
> 7735c10c74d903 Thinh Nguyen          2024-12-11  1263  
> 7735c10c74d903 Thinh Nguyen          2024-12-11  1264  		/* Sanity check */
> 7735c10c74d903 Thinh Nguyen          2024-12-11 @1265  		if (!stream || (active_cmd && active_cmd->tag != cmd->tag)) {
> 
> Testing for !stream is sufficient.  Another option would be to write this

Just testing for !stream is sufficient to know whether active_cmd is
NULL, but we still need to check for matching tag also.

> as:
> 	if (!stream || !active_cmd || active_cmd->tag != cmd->tag)) {

Perhaps we can just do this:

	if (!active_cmd || active_cmd->tag != cmd->tag)) {

If active_cmd is NULL, then the stream variable must also be NULL. This
may not be obvious.

> 
> 7735c10c74d903 Thinh Nguyen          2024-12-11  1266  			usbg_submit_command(cmd->fu, cmd->req);
> 7735c10c74d903 Thinh Nguyen          2024-12-11  1267  			return;
> 7735c10c74d903 Thinh Nguyen          2024-12-11  1268  		}
> 7735c10c74d903 Thinh Nguyen          2024-12-11  1269  
> 7735c10c74d903 Thinh Nguyen          2024-12-11  1270  		reinit_completion(&stream->cmd_completion);
> 7735c10c74d903 Thinh Nguyen          2024-12-11  1271  
> 7735c10c74d903 Thinh Nguyen          2024-12-11  1272  		/*
> 7735c10c74d903 Thinh Nguyen          2024-12-11  1273  		 * A UASP command consists of the command, data, and status
> 7735c10c74d903 Thinh Nguyen          2024-12-11  1274  		 * stages, each operating sequentially from different endpoints.
> 7735c10c74d903 Thinh Nguyen          2024-12-11  1275  		 *
> 7735c10c74d903 Thinh Nguyen          2024-12-11  1276  		 * Each USB endpoint operates independently, and depending on
> 7735c10c74d903 Thinh Nguyen          2024-12-11  1277  		 * hardware implementation, a completion callback for a transfer
> 7735c10c74d903 Thinh Nguyen          2024-12-11  1278  		 * from one endpoint may not reflect the order of completion on
> 7735c10c74d903 Thinh Nguyen          2024-12-11  1279  		 * the wire. This is particularly true for devices with
> 7735c10c74d903 Thinh Nguyen          2024-12-11  1280  		 * endpoints that have independent interrupts and event buffers.
> 7735c10c74d903 Thinh Nguyen          2024-12-11  1281  		 *
> 7735c10c74d903 Thinh Nguyen          2024-12-11  1282  		 * The driver must still detect misbehaving hosts and respond
> 7735c10c74d903 Thinh Nguyen          2024-12-11  1283  		 * with an overlap status. To reduce false overlap failures,
> 7735c10c74d903 Thinh Nguyen          2024-12-11  1284  		 * allow the active and matching stream ID a brief 1ms to
> 7735c10c74d903 Thinh Nguyen          2024-12-11  1285  		 * complete before responding with an overlap command failure.
> 7735c10c74d903 Thinh Nguyen          2024-12-11  1286  		 * Overlap failure should be rare.
> 7735c10c74d903 Thinh Nguyen          2024-12-11  1287  		 */
> 7735c10c74d903 Thinh Nguyen          2024-12-11  1288  		wait_for_completion_timeout(&stream->cmd_completion, msecs_to_jiffies(1));
> 7735c10c74d903 Thinh Nguyen          2024-12-11  1289  
> 7735c10c74d903 Thinh Nguyen          2024-12-11  1290  		/* If the previous stream is completed, retry the command. */
> 7735c10c74d903 Thinh Nguyen          2024-12-11  1291  		if (!hash_hashed(&stream->node)) {
> 7735c10c74d903 Thinh Nguyen          2024-12-11  1292  			usbg_submit_command(cmd->fu, cmd->req);
> 7735c10c74d903 Thinh Nguyen          2024-12-11  1293  			return;
> 7735c10c74d903 Thinh Nguyen          2024-12-11  1294  		}
> 7735c10c74d903 Thinh Nguyen          2024-12-11  1295  
> 7735c10c74d903 Thinh Nguyen          2024-12-11  1296  		/*
> 7735c10c74d903 Thinh Nguyen          2024-12-11  1297  		 * The command isn't submitted to the target core, so we're safe
> 7735c10c74d903 Thinh Nguyen          2024-12-11  1298  		 * to remove the bitmap index from the session tag pool.
> 7735c10c74d903 Thinh Nguyen          2024-12-11  1299  		 */
> 7735c10c74d903 Thinh Nguyen          2024-12-11  1300  		sbitmap_queue_clear(&se_sess->sess_tag_pool,
> 7735c10c74d903 Thinh Nguyen          2024-12-11  1301  				    cmd->se_cmd.map_tag,
> 7735c10c74d903 Thinh Nguyen          2024-12-11  1302  				    cmd->se_cmd.map_cpu);
> 7735c10c74d903 Thinh Nguyen          2024-12-11  1303  
> 7735c10c74d903 Thinh Nguyen          2024-12-11  1304  		/*
> 7735c10c74d903 Thinh Nguyen          2024-12-11  1305  		 * Overlap command tag detected. Cancel any pending transfer of
> 7735c10c74d903 Thinh Nguyen          2024-12-11  1306  		 * the command submitted to target core.
> 7735c10c74d903 Thinh Nguyen          2024-12-11  1307  		 */
> 7735c10c74d903 Thinh Nguyen          2024-12-11 @1308  		active_cmd->tmr_rsp = RC_OVERLAPPED_TAG;
> 
> The inconsistent NULL check triggers a warning here.
> 

We already check for !stream prior, so I didn't check for active_cmd
here. This is more of a consistency issue. If possible and if needed, we
can make this more consistent after the merge?

Thanks,
Thinh

Re: [PATCH v3 24/28] usb: gadget: f_tcm: Check overlapped command

[Dan Carpenter]

Sorry for the delayed response.  I was on vacation.

On Fri, Dec 20, 2024 at 02:31:20AM +0000, Thinh Nguyen wrote:
> On Thu, Dec 19, 2024, Dan Carpenter wrote:
> > Hi Thinh,
> > 
> > kernel test robot noticed the following build warnings:
> > 
> > url:    https://github.com/intel-lab-lkp/linux/commits/Thinh-Nguyen/usb-gadget-f_tcm-Don-t-free-command-immediately/20241211-092317 
> > base:   d8d936c51388442f769a81e512b505dcf87c6a51
> > patch link:    https://lore.kernel.org/r/6bffc2903d0cd1e7c7afca837053a48e883d8903.1733876548.git.Thinh.Nguyen%40synopsys.com 
> > patch subject: [PATCH v3 24/28] usb: gadget: f_tcm: Check overlapped command
> > config: nios2-randconfig-r071-20241219 (https://download.01.org/0day-ci/archive/20241219/202412192132.XB16SilM-lkp@intel.com/config )
> > compiler: nios2-linux-gcc (GCC) 14.2.0
> > 
> > If you fix the issue in a separate patch/commit (i.e. not just a new version of
> > the same patch/commit), kindly add following tags
> > | Reported-by: kernel test robot <lkp@intel.com>
> > | Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
> > | Closes: https://lore.kernel.org/r/202412192132.XB16SilM-lkp@intel.com/ 
> > 
> > smatch warnings:
> > drivers/usb/gadget/function/f_tcm.c:1308 usbg_cmd_work() error: we previously assumed 'active_cmd' could be null (see line 1265)
> > 
> > vim +/active_cmd +1308 drivers/usb/gadget/function/f_tcm.c
> > 
> > 287b3d115e5351 Thinh Nguyen          2024-12-11  1227  static void usbg_cmd_work(struct work_struct *work)
> > 287b3d115e5351 Thinh Nguyen          2024-12-11  1228  {
> > 287b3d115e5351 Thinh Nguyen          2024-12-11  1229  	struct usbg_cmd *cmd = container_of(work, struct usbg_cmd, work);
> > 287b3d115e5351 Thinh Nguyen          2024-12-11  1230  
> > 287b3d115e5351 Thinh Nguyen          2024-12-11  1231  	/*
> > 287b3d115e5351 Thinh Nguyen          2024-12-11  1232  	 * Failure is detected by f_tcm here. Skip submitting the command to the
> > 287b3d115e5351 Thinh Nguyen          2024-12-11  1233  	 * target core if we already know the failing response and send the usb
> > 287b3d115e5351 Thinh Nguyen          2024-12-11  1234  	 * response to the host directly.
> > 287b3d115e5351 Thinh Nguyen          2024-12-11  1235  	 */
> > 287b3d115e5351 Thinh Nguyen          2024-12-11  1236  	if (cmd->tmr_rsp != RC_RESPONSE_UNKNOWN)
> > 287b3d115e5351 Thinh Nguyen          2024-12-11  1237  		goto skip;
> > 287b3d115e5351 Thinh Nguyen          2024-12-11  1238  
> > 287b3d115e5351 Thinh Nguyen          2024-12-11  1239  	if (cmd->tmr_func)
> > 287b3d115e5351 Thinh Nguyen          2024-12-11  1240  		usbg_submit_tmr(cmd);
> > 287b3d115e5351 Thinh Nguyen          2024-12-11  1241  	else
> > 287b3d115e5351 Thinh Nguyen          2024-12-11  1242  		usbg_submit_cmd(cmd);
> > 287b3d115e5351 Thinh Nguyen          2024-12-11  1243  
> > 287b3d115e5351 Thinh Nguyen          2024-12-11  1244  	return;
> > 287b3d115e5351 Thinh Nguyen          2024-12-11  1245  
> > 287b3d115e5351 Thinh Nguyen          2024-12-11  1246  skip:
> > 7735c10c74d903 Thinh Nguyen          2024-12-11  1247  	if (cmd->tmr_rsp == RC_OVERLAPPED_TAG) {
> > 7735c10c74d903 Thinh Nguyen          2024-12-11  1248  		struct f_uas *fu = cmd->fu;
> > 7735c10c74d903 Thinh Nguyen          2024-12-11  1249  		struct se_session *se_sess;
> > 7735c10c74d903 Thinh Nguyen          2024-12-11  1250  		struct uas_stream *stream = NULL;
> > 7735c10c74d903 Thinh Nguyen          2024-12-11  1251  		struct hlist_node *tmp;
> > 7735c10c74d903 Thinh Nguyen          2024-12-11  1252  		struct usbg_cmd *active_cmd = NULL;
> > 7735c10c74d903 Thinh Nguyen          2024-12-11  1253  
> > 7735c10c74d903 Thinh Nguyen          2024-12-11  1254  		se_sess = cmd->fu->tpg->tpg_nexus->tvn_se_sess;
> > 7735c10c74d903 Thinh Nguyen          2024-12-11  1255  
> > 7735c10c74d903 Thinh Nguyen          2024-12-11  1256  		hash_for_each_possible_safe(fu->stream_hash, stream, tmp, node, cmd->tag) {
> > 7735c10c74d903 Thinh Nguyen          2024-12-11  1257  			int i = stream - &fu->stream[0];
> > 7735c10c74d903 Thinh Nguyen          2024-12-11  1258  
> > 7735c10c74d903 Thinh Nguyen          2024-12-11  1259  			active_cmd = &((struct usbg_cmd *)se_sess->sess_cmd_map)[i];
> > 7735c10c74d903 Thinh Nguyen          2024-12-11  1260  			if (active_cmd->tag == cmd->tag)
> > 7735c10c74d903 Thinh Nguyen          2024-12-11  1261  				break;
> > 7735c10c74d903 Thinh Nguyen          2024-12-11  1262  		}
> > 7735c10c74d903 Thinh Nguyen          2024-12-11  1263  
> > 7735c10c74d903 Thinh Nguyen          2024-12-11  1264  		/* Sanity check */
> > 7735c10c74d903 Thinh Nguyen          2024-12-11 @1265  		if (!stream || (active_cmd && active_cmd->tag != cmd->tag)) {
> > 
> > Testing for !stream is sufficient.  Another option would be to write this
> 
> Just testing for !stream is sufficient to know whether active_cmd is
> NULL, but we still need to check for matching tag also.

Yes.  Sorry, I was unclear.  That's what I meant.  We could write it
as:

	if (!stream || active_cmd->tag != cmd->tag) {

There is no need to check if active_cmd is non-NULL since we know that
stream is non-NULL.  However, if we DO check it, then we should check
it consistently everywhere.

> 
> > as:
> > 	if (!stream || !active_cmd || active_cmd->tag != cmd->tag)) {
> 
> Perhaps we can just do this:
> 
> 	if (!active_cmd || active_cmd->tag != cmd->tag)) {
> 
> If active_cmd is NULL, then the stream variable must also be NULL. This
> may not be obvious.
> 
> > 
> > 7735c10c74d903 Thinh Nguyen          2024-12-11  1266  			usbg_submit_command(cmd->fu, cmd->req);
> > 7735c10c74d903 Thinh Nguyen          2024-12-11  1267  			return;
> > 7735c10c74d903 Thinh Nguyen          2024-12-11  1268  		}
> > 7735c10c74d903 Thinh Nguyen          2024-12-11  1269  
> > 7735c10c74d903 Thinh Nguyen          2024-12-11  1270  		reinit_completion(&stream->cmd_completion);
> > 7735c10c74d903 Thinh Nguyen          2024-12-11  1271  
> > 7735c10c74d903 Thinh Nguyen          2024-12-11  1272  		/*
> > 7735c10c74d903 Thinh Nguyen          2024-12-11  1273  		 * A UASP command consists of the command, data, and status
> > 7735c10c74d903 Thinh Nguyen          2024-12-11  1274  		 * stages, each operating sequentially from different endpoints.
> > 7735c10c74d903 Thinh Nguyen          2024-12-11  1275  		 *
> > 7735c10c74d903 Thinh Nguyen          2024-12-11  1276  		 * Each USB endpoint operates independently, and depending on
> > 7735c10c74d903 Thinh Nguyen          2024-12-11  1277  		 * hardware implementation, a completion callback for a transfer
> > 7735c10c74d903 Thinh Nguyen          2024-12-11  1278  		 * from one endpoint may not reflect the order of completion on
> > 7735c10c74d903 Thinh Nguyen          2024-12-11  1279  		 * the wire. This is particularly true for devices with
> > 7735c10c74d903 Thinh Nguyen          2024-12-11  1280  		 * endpoints that have independent interrupts and event buffers.
> > 7735c10c74d903 Thinh Nguyen          2024-12-11  1281  		 *
> > 7735c10c74d903 Thinh Nguyen          2024-12-11  1282  		 * The driver must still detect misbehaving hosts and respond
> > 7735c10c74d903 Thinh Nguyen          2024-12-11  1283  		 * with an overlap status. To reduce false overlap failures,
> > 7735c10c74d903 Thinh Nguyen          2024-12-11  1284  		 * allow the active and matching stream ID a brief 1ms to
> > 7735c10c74d903 Thinh Nguyen          2024-12-11  1285  		 * complete before responding with an overlap command failure.
> > 7735c10c74d903 Thinh Nguyen          2024-12-11  1286  		 * Overlap failure should be rare.
> > 7735c10c74d903 Thinh Nguyen          2024-12-11  1287  		 */
> > 7735c10c74d903 Thinh Nguyen          2024-12-11  1288  		wait_for_completion_timeout(&stream->cmd_completion, msecs_to_jiffies(1));
> > 7735c10c74d903 Thinh Nguyen          2024-12-11  1289  
> > 7735c10c74d903 Thinh Nguyen          2024-12-11  1290  		/* If the previous stream is completed, retry the command. */
> > 7735c10c74d903 Thinh Nguyen          2024-12-11  1291  		if (!hash_hashed(&stream->node)) {
> > 7735c10c74d903 Thinh Nguyen          2024-12-11  1292  			usbg_submit_command(cmd->fu, cmd->req);
> > 7735c10c74d903 Thinh Nguyen          2024-12-11  1293  			return;
> > 7735c10c74d903 Thinh Nguyen          2024-12-11  1294  		}
> > 7735c10c74d903 Thinh Nguyen          2024-12-11  1295  
> > 7735c10c74d903 Thinh Nguyen          2024-12-11  1296  		/*
> > 7735c10c74d903 Thinh Nguyen          2024-12-11  1297  		 * The command isn't submitted to the target core, so we're safe
> > 7735c10c74d903 Thinh Nguyen          2024-12-11  1298  		 * to remove the bitmap index from the session tag pool.
> > 7735c10c74d903 Thinh Nguyen          2024-12-11  1299  		 */
> > 7735c10c74d903 Thinh Nguyen          2024-12-11  1300  		sbitmap_queue_clear(&se_sess->sess_tag_pool,
> > 7735c10c74d903 Thinh Nguyen          2024-12-11  1301  				    cmd->se_cmd.map_tag,
> > 7735c10c74d903 Thinh Nguyen          2024-12-11  1302  				    cmd->se_cmd.map_cpu);
> > 7735c10c74d903 Thinh Nguyen          2024-12-11  1303  
> > 7735c10c74d903 Thinh Nguyen          2024-12-11  1304  		/*
> > 7735c10c74d903 Thinh Nguyen          2024-12-11  1305  		 * Overlap command tag detected. Cancel any pending transfer of
> > 7735c10c74d903 Thinh Nguyen          2024-12-11  1306  		 * the command submitted to target core.
> > 7735c10c74d903 Thinh Nguyen          2024-12-11  1307  		 */
> > 7735c10c74d903 Thinh Nguyen          2024-12-11 @1308  		active_cmd->tmr_rsp = RC_OVERLAPPED_TAG;
> > 
> > The inconsistent NULL check triggers a warning here.
> > 
> 
> We already check for !stream prior, so I didn't check for active_cmd
> here. This is more of a consistency issue. If possible and if needed, we
> can make this more consistent after the merge?

This is not a run time bug, yes.  It's just an inconsistent NULL check,
but the NULL check is not necessary so that's not a problem.  I don't
have a vote on how you merge it.  ;)  You can do that however you want.

regards,
dan carpenter

[PATCH v3 25/28] usb: gadget: f_tcm: Stall on invalid CBW

[Thinh Nguyen]

If the BOT command CBW is invalid, make sure to respond by setting
status endpoint STALL until the next proper CBW or reset.

Signed-off-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com>
---
 drivers/usb/gadget/function/f_tcm.c | 16 +++++++++++++++-
 drivers/usb/gadget/function/tcm.h   |  1 +
 2 files changed, 16 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/gadget/function/f_tcm.c b/drivers/usb/gadget/function/f_tcm.c
index 0c7a41568f40..7ea48845f8c3 100644
--- a/drivers/usb/gadget/function/f_tcm.c
+++ b/drivers/usb/gadget/function/f_tcm.c
@@ -287,8 +287,17 @@ static void bot_cmd_complete(struct usb_ep *ep, struct usb_request *req)
 		return;
 
 	ret = bot_submit_command(fu, req->buf, req->actual);
-	if (ret)
+	if (ret) {
 		pr_err("%s(%d): %d\n", __func__, __LINE__, ret);
+		if (!(fu->flags & USBG_BOT_WEDGED))
+			usb_ep_set_wedge(fu->ep_in);
+
+		fu->flags |= USBG_BOT_WEDGED;
+		bot_enqueue_cmd_cbw(fu);
+	} else if (fu->flags & USBG_BOT_WEDGED) {
+		fu->flags &= ~USBG_BOT_WEDGED;
+		usb_ep_clear_halt(fu->ep_in);
+	}
 }
 
 static int bot_prepare_reqs(struct f_uas *fu)
@@ -442,6 +451,11 @@ static int usbg_bot_setup(struct usb_function *f,
 
 	case US_BULK_RESET_REQUEST:
 		/* XXX maybe we should remove previous requests for IN + OUT */
+		if (fu->flags & USBG_BOT_WEDGED) {
+			fu->flags &= ~USBG_BOT_WEDGED;
+			usb_ep_clear_halt(fu->ep_in);
+		}
+
 		bot_enqueue_cmd_cbw(fu);
 		return 0;
 	}
diff --git a/drivers/usb/gadget/function/tcm.h b/drivers/usb/gadget/function/tcm.h
index f6d6c86d10b3..009974d81d66 100644
--- a/drivers/usb/gadget/function/tcm.h
+++ b/drivers/usb/gadget/function/tcm.h
@@ -130,6 +130,7 @@ struct f_uas {
 #define USBG_USE_STREAMS	(1 << 2)
 #define USBG_IS_BOT		(1 << 3)
 #define USBG_BOT_CMD_PEND	(1 << 4)
+#define USBG_BOT_WEDGED		(1 << 5)
 
 	struct usbg_cdb		cmd[USBG_NUM_CMDS];
 	struct usb_ep		*ep_in;
-- 
2.28.0

[PATCH v3 26/28] usb: gadget: f_tcm: Requeue command request on error

[Thinh Nguyen]

If there's error on command request, make sure to requeue to receive the
next one.

Signed-off-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com>
---
 drivers/usb/gadget/function/f_tcm.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/gadget/function/f_tcm.c b/drivers/usb/gadget/function/f_tcm.c
index 7ea48845f8c3..be7d8df360d9 100644
--- a/drivers/usb/gadget/function/f_tcm.c
+++ b/drivers/usb/gadget/function/f_tcm.c
@@ -283,8 +283,13 @@ static void bot_cmd_complete(struct usb_ep *ep, struct usb_request *req)
 
 	fu->flags &= ~USBG_BOT_CMD_PEND;
 
-	if (req->status < 0)
+	if (req->status < 0) {
+		struct usb_gadget *gadget = fuas_to_gadget(fu);
+
+		dev_err(&gadget->dev, "BOT command req err (%d)\n", req->status);
+		bot_enqueue_cmd_cbw(fu);
 		return;
+	}
 
 	ret = bot_submit_command(fu, req->buf, req->actual);
 	if (ret) {
-- 
2.28.0

[PATCH v3 27/28] usb: gadget: f_tcm: Track BOT command kref

[Thinh Nguyen]

Set TARGET_SCF_ACK_KREF flag and allow f_tcm to take the BOT command
reference. A usb request may be canceled, the f_tcm knows this. Let it
decides if the command should be freed. This is the same as how the UAS
interface is done.

Signed-off-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com>
---
 drivers/usb/gadget/function/f_tcm.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/gadget/function/f_tcm.c b/drivers/usb/gadget/function/f_tcm.c
index be7d8df360d9..72e7d4558eef 100644
--- a/drivers/usb/gadget/function/f_tcm.c
+++ b/drivers/usb/gadget/function/f_tcm.c
@@ -1466,6 +1466,7 @@ static void bot_cmd_work(struct work_struct *work)
 	struct se_cmd *se_cmd;
 	struct tcm_usbg_nexus *tv_nexus;
 	struct usbg_tpg *tpg;
+	int flags = TARGET_SCF_ACK_KREF;
 	int dir;
 
 	/*
@@ -1494,7 +1495,7 @@ static void bot_cmd_work(struct work_struct *work)
 
 	target_submit_cmd(se_cmd, tv_nexus->tvn_se_sess,
 			  cmd->cmd_buf, cmd->sense_iu.sense, cmd->unpacked_lun,
-			  cmd->data_len, cmd->prio_attr, dir, 0);
+			  cmd->data_len, cmd->prio_attr, dir, flags);
 	return;
 
 out:
-- 
2.28.0

[PATCH v3 28/28] usb: gadget: f_tcm: Refactor goto check_condition

[Thinh Nguyen]

Move the command initialization before the check_condition to after the
goto statement for a cleaner look. No functional change here.

Signed-off-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com>
---
 drivers/usb/gadget/function/f_tcm.c | 26 ++++++++++++--------------
 1 file changed, 12 insertions(+), 14 deletions(-)

diff --git a/drivers/usb/gadget/function/f_tcm.c b/drivers/usb/gadget/function/f_tcm.c
index 72e7d4558eef..5a2e1237f85c 100644
--- a/drivers/usb/gadget/function/f_tcm.c
+++ b/drivers/usb/gadget/function/f_tcm.c
@@ -1223,14 +1223,8 @@ static void usbg_submit_cmd(struct usbg_cmd *cmd)
 	tpg = cmd->fu->tpg;
 	tv_nexus = tpg->tpg_nexus;
 	dir = get_cmd_dir(cmd->cmd_buf);
-	if (dir < 0) {
-		__target_init_cmd(se_cmd,
-				  tv_nexus->tvn_se_sess->se_tpg->se_tpg_tfo,
-				  tv_nexus->tvn_se_sess, cmd->data_len, DMA_NONE,
-				  cmd->prio_attr, cmd->sense_iu.sense,
-				  cmd->unpacked_lun, NULL);
+	if (dir < 0)
 		goto out;
-	}
 
 	target_submit_cmd(se_cmd, tv_nexus->tvn_se_sess, cmd->cmd_buf,
 			  cmd->sense_iu.sense, cmd->unpacked_lun, 0,
@@ -1239,6 +1233,11 @@ static void usbg_submit_cmd(struct usbg_cmd *cmd)
 	return;
 
 out:
+	__target_init_cmd(se_cmd,
+			  tv_nexus->tvn_se_sess->se_tpg->se_tpg_tfo,
+			  tv_nexus->tvn_se_sess, cmd->data_len, DMA_NONE,
+			  cmd->prio_attr, cmd->sense_iu.sense,
+			  cmd->unpacked_lun, NULL);
 	transport_send_check_condition_and_sense(se_cmd,
 			TCM_UNSUPPORTED_SCSI_OPCODE, 0);
 }
@@ -1484,14 +1483,8 @@ static void bot_cmd_work(struct work_struct *work)
 	tpg = cmd->fu->tpg;
 	tv_nexus = tpg->tpg_nexus;
 	dir = get_cmd_dir(cmd->cmd_buf);
-	if (dir < 0) {
-		__target_init_cmd(se_cmd,
-				  tv_nexus->tvn_se_sess->se_tpg->se_tpg_tfo,
-				  tv_nexus->tvn_se_sess, cmd->data_len, DMA_NONE,
-				  cmd->prio_attr, cmd->sense_iu.sense,
-				  cmd->unpacked_lun, NULL);
+	if (dir < 0)
 		goto out;
-	}
 
 	target_submit_cmd(se_cmd, tv_nexus->tvn_se_sess,
 			  cmd->cmd_buf, cmd->sense_iu.sense, cmd->unpacked_lun,
@@ -1499,6 +1492,11 @@ static void bot_cmd_work(struct work_struct *work)
 	return;
 
 out:
+	__target_init_cmd(se_cmd,
+			  tv_nexus->tvn_se_sess->se_tpg->se_tpg_tfo,
+			  tv_nexus->tvn_se_sess, cmd->data_len, DMA_NONE,
+			  cmd->prio_attr, cmd->sense_iu.sense,
+			  cmd->unpacked_lun, NULL);
 	transport_send_check_condition_and_sense(se_cmd,
 				TCM_UNSUPPORTED_SCSI_OPCODE, 0);
 }
-- 
2.28.0

Re: [PATCH v3 00/28] usb: gadget: f_tcm: Enhance UASP driver

[Greg Kroah-Hartman]

On Wed, Dec 11, 2024 at 12:31:30AM +0000, Thinh Nguyen wrote:
> Apologies for the delay; after two years and multiple requests to resume this
> series, I squeezed some time to push an update. This series applies on top of
> Greg's usb-testing branch.
> 
> If possible, please help test this series and get this merged as my resources
> are nil for this work.

You have many bugfixes in the first few commits of this series, but if I
apply the whole series, those will not get into Linus's tree until
6.14-rc1.  Is that ok or should they go separately into 6.13-final now?

thanks,

greg k-h

Re: [PATCH v3 00/28] usb: gadget: f_tcm: Enhance UASP driver

[Thinh Nguyen]

Hi Greg,

On Wed, Dec 11, 2024, Greg Kroah-Hartman wrote:
> On Wed, Dec 11, 2024 at 12:31:30AM +0000, Thinh Nguyen wrote:
> > Apologies for the delay; after two years and multiple requests to resume this
> > series, I squeezed some time to push an update. This series applies on top of
> > Greg's usb-testing branch.
> > 
> > If possible, please help test this series and get this merged as my resources
> > are nil for this work.
> 
> You have many bugfixes in the first few commits of this series, but if I
> apply the whole series, those will not get into Linus's tree until
> 6.14-rc1.  Is that ok or should they go separately into 6.13-final now?
> 

Yes, it should be ok for all of these to land in 6.14-rc1. With just the
fix changes in this series will not make f_tcm any more usable because
of interopability and performance issues. IMHO, adding the entire series
in your next branch would allow more people to run proper tests.
Splitting this means for a few weeks, until a rebase, neither the
usb-linus branch or the usb-testing branch would have a proper working
UASP driver.

Thanks,
Thinh

Re: [PATCH v3 00/28] usb: gadget: f_tcm: Enhance UASP driver

[Homura Akemi]

Hi Thinh,

On 2024-12-11 0:31 UTC, Thinh Nguyen <Thinh.Nguyen@synopsys.com> wrote:
> 1) Fix Data Corruption
> ----------------------
>
> Properly increment the "len" base on the command requested length instead of
> the SG entry length.
>
> If you're using File backend, then you need to fix target_core_file. If you're
> using other backend such as Ramdisk, then you need a similar fix there.

I am trying to do some basic rw aging test with this serie on my gadget board.
When it comes to target_core_iblock, the rw code is less similar to the one in
target_core_file or ramdisk.
Could you just spend some extra time explaining what cause the Data
Corruption issue and how can this fix it ? So that I could perform
similar fix in
target_core_iblock on my own, so a full test could start soonner.

B.R.
H. Akemi

Re: [PATCH v3 00/28] usb: gadget: f_tcm: Enhance UASP driver

[Thinh Nguyen]

(Removed Cc invalid emails to Nicholas and Andrzej)

Hi,

On Fri, Dec 20, 2024, Homura Akemi wrote:
> Hi Thinh,
> 
> On 2024-12-11 0:31 UTC, Thinh Nguyen <Thinh.Nguyen@synopsys.com> wrote:
> > 1) Fix Data Corruption
> > ----------------------
> >
> > Properly increment the "len" base on the command requested length instead of
> > the SG entry length.
> >
> > If you're using File backend, then you need to fix target_core_file. If you're
> > using other backend such as Ramdisk, then you need a similar fix there.
> 
> I am trying to do some basic rw aging test with this serie on my gadget board.
> When it comes to target_core_iblock, the rw code is less similar to the one in
> target_core_file or ramdisk.
> Could you just spend some extra time explaining what cause the Data
> Corruption issue and how can this fix it ? So that I could perform
> similar fix in
> target_core_iblock on my own, so a full test could start soonner.
> 

When we prepare a new generic command for read/write, we call to
target_alloc_sgl(). This will allocate PAGE_SIZE SG entries enough to
handle the se_cmd read/write base on its length. The total length of all
the SG entries combine will be at least se_cmd->data_length.

The typical block size is 512 byte. A page size is typically 4KB. So,
the se_cmd->data_length may not be a multiple of PAGE_SiZE. In
target_core_file, it execute_rw() with this logic:

	for_each_sg(sgl, sg, sgl_nents, i) {
		bvec_set_page(&aio_cmd->bvecs[i], sg_page(sg), sg->length,
			      sg->offset);
		len += sg->length;
	}

// It sets the length to be the iter to be total length of the
// allocated SG entries and not the requested command length:

	iov_iter_bvec(&iter, is_write, aio_cmd->bvecs, sgl_nents, len);

	aio_cmd->cmd = cmd;
	aio_cmd->len = len;
	aio_cmd->iocb.ki_pos = cmd->t_task_lba * dev->dev_attrib.block_size;
	aio_cmd->iocb.ki_filp = file;
	aio_cmd->iocb.ki_complete = cmd_rw_aio_complete;
	aio_cmd->iocb.ki_flags = IOCB_DIRECT;

	if (is_write && (cmd->se_cmd_flags & SCF_FUA))
		aio_cmd->iocb.ki_flags |= IOCB_DSYNC;

// So when we do f_op read/write, we may do more than needed and may
// write bogus data from the extra SG entry length.

	if (is_write)
		ret = file->f_op->write_iter(&aio_cmd->iocb, &iter);
	else
		ret = file->f_op->read_iter(&aio_cmd->iocb, &iter);


I did not review target_core_iblock. It may or may not do things
properly.

BR,
Thinh