[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

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