usb: gadget: f_tcm: Enhance UASP driver
Subsystems (3)
v3 cover usb: gadget: f_tcm: Enhance UASP driver
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(-)
drivers/target/target_core_file.c +12 -2
@@ -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);
Signature · 13 lines
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(-)
drivers/target/target_core_transport.c +15 -4
@@ -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
Signature · 47 lines
Changes in v3: - v2: https://lore.kernel.org/linux-usb/[email protected]/ - 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 1/28 usb: gadget: f_tcm: Don't free command immediately
Don't prematurely free the command. Wait for the status completion of the sense status. It can be freed then. Otherwise we will double-free the command.
Fixes cff834c16d23 ("usb-gadget/tcm: Convert to TARGET_SCF_ACK_KREF I/O krefs")
Signed-off-by Thinh Nguyen <[email protected]>
--- drivers/usb/gadget/function/f_tcm.c | 2 -- 1 file changed, 2 deletions(-)
drivers/usb/gadget/function/f_tcm.c +0 -2
@@ -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,
Signature · 1 line
2.28.0
PATCH v3 2/28 usb: gadget: f_tcm: Translate error to sense
In reply to Thinh NguyenWhen respond with check_condition error status, clear from_transport input so the target layer can translate the sense reason reported by f_tcm.
Fixes c52661d60f63 ("usb-gadget: Initial merge of target module for UASP + BOT")
Signed-off-by Thinh Nguyen <[email protected]>
--- drivers/usb/gadget/function/f_tcm.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
drivers/usb/gadget/function/f_tcm.c +2 -2
@@ -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,
Signature · 1 line
2.28.0
PATCH v3 3/28 usb: gadget: f_tcm: Decrement command ref count on cleanup
In reply to Thinh NguyenWe submitted the command with TARGET_SCF_ACK_KREF, which requires acknowledgment of command completion. If the command fails, make sure to decrement the ref count.
Fixes cff834c16d23 ("usb-gadget/tcm: Convert to TARGET_SCF_ACK_KREF I/O krefs")
Signed-off-by Thinh Nguyen <[email protected]>
--- drivers/usb/gadget/function/f_tcm.c | 1 + 1 file changed, 1 insertion(+)
drivers/usb/gadget/function/f_tcm.c +1 -0
@@ -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 ); }
Signature · 1 line
2.28.0
PATCH v3 4/28 usb: gadget: f_tcm: Fix Get/SetInterface return value
In reply to Thinh NguyenCheck 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")
Signed-off-by Thinh Nguyen <[email protected]>
--- drivers/usb/gadget/function/f_tcm.c | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-)
drivers/usb/gadget/function/f_tcm.c +10 -2
@@ -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;
Signature · 1 line
2.28.0
PATCH v3 5/28 usb: gadget: f_tcm: ep_autoconfig with fullspeed endpoint
In reply to Thinh NguyenMatch 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")
Signed-off-by Thinh Nguyen <[email protected]>
--- drivers/usb/gadget/function/f_tcm.c | 32 +++++++++++++---------------- 1 file changed, 14 insertions(+), 18 deletions(-)
drivers/usb/gadget/function/f_tcm.c +14 -18
@@ -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,
Signature · 1 line
2.28.0
PATCH v3 6/28 usb: gadget: f_tcm: Don't prepare BOT write request twice
In reply to Thinh NguyenThe 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")
Signed-off-by Thinh Nguyen <[email protected]>
--- drivers/usb/gadget/function/f_tcm.c | 17 ----------------- 1 file changed, 17 deletions(-)
drivers/usb/gadget/function/f_tcm.c +0 -17
@@ -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;
Signature · 1 line
2.28.0
PATCH v3 7/28 usb: gadget: f_tcm: Increase stream count
In reply to Thinh NguyenSome 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 <[email protected]>
--- drivers/usb/gadget/function/tcm.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/usb/gadget/function/tcm.h +1 -1
@@ -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 {
Signature · 1 line
2.28.0
PATCH v3 8/28 usb: gadget: f_tcm: Increase bMaxBurst
In reply to Thinh NguyenCurrently 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 <[email protected]>
--- drivers/usb/gadget/function/f_tcm.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)
drivers/usb/gadget/function/f_tcm.c +4 -2
@@ -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 = {
Signature · 1 line
2.28.0
PATCH v3 9/28 usb: gadget: f_tcm: Limit number of sessions
In reply to Thinh NguyenOnly 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 <[email protected]>
--- drivers/usb/gadget/function/tcm.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/usb/gadget/function/tcm.h +1 -1
@@ -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_STREAMSstruct tcm_usbg_nexus {struct se_session *tvn_se_sess;
Signature · 1 line
2.28.0
PATCH v3 10/28 usb: gadget: f_tcm: Get stream by sbitmap number
In reply to Thinh NguyenWe 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 <[email protected]>
--- drivers/usb/gadget/function/f_tcm.c | 26 ++++++-------------------- drivers/usb/gadget/function/tcm.h | 1 - 2 files changed, 6 insertions(+), 21 deletions(-)
drivers/usb/gadget/function/f_tcm.c +6 -20
@@ -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)
drivers/usb/gadget/function/tcm.h +0 -1
@@ -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;
Signature · 1 line
2.28.0
PATCH v3 11/28 usb: gadget: f_tcm: Don't set static stream_id
In reply to Thinh NguyenHost 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 <[email protected]>
--- drivers/usb/gadget/function/f_tcm.c | 17 +++-------------- 1 file changed, 3 insertions(+), 14 deletions(-)
drivers/usb/gadget/function/f_tcm.c +3 -14
@@ -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;
Signature · 1 line
2.28.0
PATCH v3 12/28 usb: gadget: f_tcm: Allocate matching number of commands to streams
In reply to Thinh NguyenWe 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 <[email protected]>
--- drivers/usb/gadget/function/f_tcm.c | 107 ++++++++++++++-------------- drivers/usb/gadget/function/tcm.h | 10 ++- 2 files changed, 61 insertions(+), 56 deletions(-)
drivers/usb/gadget/function/f_tcm.c +54 -53
@@ -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);
drivers/usb/gadget/function/tcm.h +7 -3
@@ -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_CMDSstruct 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;
Signature · 1 line
2.28.0
PATCH v3 13/28 usb: gadget: f_tcm: Handle multiple commands in parallel
In reply to Thinh NguyenResubmit 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 <[email protected]>
--- drivers/usb/gadget/function/f_tcm.c | 20 ++++++++------------ 1 file changed, 8 insertions(+), 12 deletions(-)
drivers/usb/gadget/function/f_tcm.c +8 -12
@@ -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;
Signature · 1 line
2.28.0
PATCH v3 14/28 usb: gadget: f_tcm: Use extra number of commands
In reply to Thinh NguyenTo 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 <[email protected]>
--- drivers/usb/gadget/function/tcm.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/usb/gadget/function/tcm.h +1 -1
@@ -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 ,
Signature · 1 line
2.28.0
PATCH v3 15/28 usb: gadget: f_tcm: Return ATA cmd direction
In reply to Thinh NguyenCheck ATA Pass-Through for direction.
Signed-off-by Thinh Nguyen <[email protected]>
--- drivers/usb/gadget/function/f_tcm.c | 2 ++ 1 file changed, 2 insertions(+)
drivers/usb/gadget/function/f_tcm.c +2 -0
@@ -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 ;
Signature · 1 line
2.28.0
PATCH v3 16/28 usb: gadget: f_tcm: Execute command on write completion
In reply to Thinh NguyenDon'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 <[email protected]>
--- drivers/usb/gadget/function/f_tcm.c | 39 +++++++++++++++++++++++------ drivers/usb/gadget/function/tcm.h | 4 ++- 2 files changed, 34 insertions(+), 9 deletions(-)
drivers/usb/gadget/function/f_tcm.c +31 -8
@@ -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);
drivers/usb/gadget/function/tcm.h +3 -1
@@ -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;
Signature · 1 line
2.28.0
PATCH v3 17/28 usb: gadget: f_tcm: Minor cleanup redundant code
In reply to Thinh NguyenThe status request preparation is done in uasp_prepare_status(). Remove duplicate code. No functional change here.
Signed-off-by Thinh Nguyen <[email protected]>
--- drivers/usb/gadget/function/f_tcm.c | 2 -- 1 file changed, 2 deletions(-)
drivers/usb/gadget/function/f_tcm.c +0 -2
@@ -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 );
Signature · 1 line
2.28.0
PATCH v3 18/28 usb: gadget: f_tcm: Handle abort command
In reply to Thinh NguyenImplement usbg_aborted_task() to cancel aborted outstanding requests.
Signed-off-by Thinh Nguyen <[email protected]>
--- drivers/usb/gadget/function/f_tcm.c | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+)
drivers/usb/gadget/function/f_tcm.c +18 -0
@@ -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)
Signature · 1 line
2.28.0
PATCH v3 19/28 usb: gadget: f_tcm: Cleanup requests on ep disable
In reply to Thinh NguyenThere 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 <[email protected]>
--- drivers/usb/gadget/function/f_tcm.c | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-)
drivers/usb/gadget/function/f_tcm.c +13 -2
@@ -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; }
Signature · 1 line
2.28.0
PATCH v3 20/28 usb: gadget: f_tcm: Stop proceeding further on -ESHUTDOWN
In reply to Thinh NguyenIf 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 <[email protected]>
--- drivers/usb/gadget/function/f_tcm.c | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-)
drivers/usb/gadget/function/f_tcm.c +7 -3
@@ -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 )
Signature · 1 line
2.28.0
PATCH v3 21/28 usb: gadget: f_tcm: Save CPU ID per command
In reply to Thinh NguyenNormally 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 <[email protected]>
--- drivers/usb/gadget/function/f_tcm.c | 1 + 1 file changed, 1 insertion(+)
drivers/usb/gadget/function/f_tcm.c +1 -0
@@ -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;
Signature · 1 line
2.28.0
PATCH v3 22/28 usb: gadget: f_tcm: Send sense on cancelled transfer
In reply to Thinh NguyenIf 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 <[email protected]>
--- drivers/usb/gadget/function/f_tcm.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
drivers/usb/gadget/function/f_tcm.c +2 -1
@@ -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)
Signature · 1 line
2.28.0
PATCH v3 23/28 usb: gadget: f_tcm: Handle TASK_MANAGEMENT commands
In reply to Thinh NguyenHandle 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 <[email protected]>
--- drivers/usb/gadget/function/f_tcm.c | 220 +++++++++++++++++++++++++--- drivers/usb/gadget/function/tcm.h | 5 + 2 files changed, 206 insertions(+), 19 deletions(-)
drivers/usb/gadget/function/f_tcm.c +201 -19
@@ -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)
drivers/usb/gadget/function/tcm.h +5 -0
@@ -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;
Signature · 1 line
2.28.0
PATCH v3 24/28 usb: gadget: f_tcm: Check overlapped command
In reply to Thinh NguyenIf there's an overlapped command tag, cancel the command and respond with RC_OVERLAPPED_TAG to host.
Signed-off-by Thinh Nguyen <[email protected]>
--- drivers/usb/gadget/function/f_tcm.c | 123 +++++++++++++++++++++++++++- drivers/usb/gadget/function/tcm.h | 5 ++ 2 files changed, 127 insertions(+), 1 deletion(-)
drivers/usb/gadget/function/f_tcm.c +122 -1
@@ -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;
drivers/usb/gadget/function/tcm.h +5 -0
@@ -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;
Signature · 1 line
2.28.0
Re: [PATCH v3 24/28] usb: gadget: f_tcm: Check overlapped command
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/[email protected]/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 <[email protected]> | Reported-by: Dan Carpenter <[email protected]> | Closes: https://lore.kernel.org/r/[email protected]/
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 }
Signature · 2 lines
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
Quoted 62 lines · Dan Carpenter
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/[email protected]/config__;!!A4F2R9G_pg!YKeQa8JmJaKHAak1XzUO0sLWgipnVS9vCGr4PiZb8TEMwYAnaMG4XVSZ3aeoCV-54D_6YX6ylxj93-NYCMU04JujRj-r$ ) compiler: nios2-linux-gcc (GCC) 14.2.0If 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 <[email protected]> | Reported-by: Dan Carpenter <[email protected]> | Closes: https://urldefense.com/v3/__https://lore.kernel.org/r/[email protected]/__;!!A4F2R9G_pg!YKeQa8JmJaKHAak1XzUO0sLWgipnVS9vCGr4PiZb8TEMwYAnaMG4XVSZ3aeoCV-54D_6YX6ylxj93-NYCMU04FWUkKjj$smatch warnings:
drivers/usb/gadget/function/f_tcm.c:1308usbg_cmd_work()error: we previously assumed 'active_cmd' could be null (see line 1265)vim +/active_cmd +1308
drivers/usb/gadget/function/f_tcm.c287b3d115e5351 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)) {
Quoted 1 line
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.
Quoted 2 lines
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.
Quoted 45 lines
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;Quoted 2 lines
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
Sorry for the delayed response. I was on vacation.
Quoted 63 lines · Thinh Nguyen
Quoted 62 lines · 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/[email protected]/config ) compiler: nios2-linux-gcc (GCC) 14.2.0If 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 <[email protected]> | Reported-by: Dan Carpenter <[email protected]> | Closes: https://lore.kernel.org/r/[email protected]/smatch warnings:
drivers/usb/gadget/function/f_tcm.c:1308usbg_cmd_work()error: we previously assumed 'active_cmd' could be null (see line 1265)vim +/active_cmd +1308
drivers/usb/gadget/function/f_tcm.c287b3d115e5351 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)) {
Quoted 4 lines
Quoted 1 line
Testing for !stream is sufficient. Another option would be to write this
Just testing for !stream is sufficient to know whether
active_cmdis 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.
Quoted 56 lines
Quoted 3 lines
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_cmdis NULL, then the stream variable must also be NULL. This may not be obvious.Quoted 45 lines
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;
Quoted 6 lines
Quoted 2 lines
The inconsistent NULL check triggers a warning here.
We already check for !stream prior, so I didn't check for
active_cmdhere. 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
In reply to Thinh NguyenIf 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 <[email protected]>
--- drivers/usb/gadget/function/f_tcm.c | 16 +++++++++++++++- drivers/usb/gadget/function/tcm.h | 1 + 2 files changed, 16 insertions(+), 1 deletion(-)
drivers/usb/gadget/function/f_tcm.c +15 -1
@@ -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 ; }
drivers/usb/gadget/function/tcm.h +1 -0
@@ -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;
Signature · 1 line
2.28.0
PATCH v3 26/28 usb: gadget: f_tcm: Requeue command request on error
In reply to Thinh NguyenIf there's error on command request, make sure to requeue to receive the next one.
Signed-off-by Thinh Nguyen <[email protected]>
--- drivers/usb/gadget/function/f_tcm.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)
drivers/usb/gadget/function/f_tcm.c +6 -1
@@ -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) {
Signature · 1 line
2.28.0
PATCH v3 27/28 usb: gadget: f_tcm: Track BOT command kref
In reply to Thinh NguyenSet 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 <[email protected]>
--- drivers/usb/gadget/function/f_tcm.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
drivers/usb/gadget/function/f_tcm.c +2 -1
@@ -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:
Signature · 1 line
2.28.0
PATCH v3 28/28 usb: gadget: f_tcm: Refactor goto check_condition
In reply to Thinh NguyenMove 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 <[email protected]>
--- drivers/usb/gadget/function/f_tcm.c | 26 ++++++++++++-------------- 1 file changed, 12 insertions(+), 14 deletions(-)
drivers/usb/gadget/function/f_tcm.c +12 -14
@@ -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 ); }
Signature · 1 line
2.28.0
Greg Kroah-Hartman
maintainerQuoted 6 lines · 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.
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
Hi Greg,
Quoted 5 lines · Greg Kroah-Hartman
Quoted 4 lines · 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.
Quoted 7 lines
Quoted 2 lines
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
Hi Thinh,
Quoted 8 lines · Thinh Nguyen
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 intarget_core_iblock on my own, so a full test could start soonner.
B.R.
H. Akemi
(Removed Cc invalid emails to Nicholas and Andrzej)
Hi,
Quoted 12 lines · Homura Akemi
Hi Thinh,
Quoted 8 lines · Thinh Nguyen
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.
Quoted 8 lines
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 intarget_core_fileor 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 intarget_core_iblockon 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