The Linux UASP gadget driver is incomplete and remained broken for a long time.
It was not implemented for performance either. This series adds some of the
required features for the UASP driver to work. It also makes some changes to
the target core.
This is tested against UASP CV and DWC_usb3x controller. It still needs some
fixes in the target core, which will be separated from this series.
There are still more room for performance improvement and fixes. However, this
series should be sufficient to bring up a working UASP device.
Changes in v2:
- Remove most target core changes from this series and only keep the must-have
ones
- Split the task-management patch to smaller patches
- Don't send failure Task Management response to target core, reducing
dependency
- Add UASP bringup script example in cover page
- Make various small updates according to feedbacks
Example script
==============
To test UASP, here's an example perl script snippet to bring it up.
Note: the script was cut down and quickly rewritten, so sorry if I make
mistakes. :)
my $MY_UAS_VID = xxxx;
my $MY_UAS_PID = yyyy;
my $SERIAL = "1234";
my $VENDOR = "VENDOR";
my $MY_VER = "VER";
my $vendor_id = "my_vid";
my $product_id = "my_pid";
my $revision = "my_rev";
# Must update:
my $backing_storage = "/tmp/some_file";
my $backing_storage_size = 1024*1024*16;
my $use_ramdisk = 0;
my $g = "/sys/kernel/config/usb_gadget/g1";
system("modprobe libcomposite");
system("modprobe usb_f_tcm");
system("mkdir -p $g");
system("mkdir -p $g/configs/c.1");
system("mkdir -p $g/functions/tcm.0");
system("mkdir -p $g/strings/0x409");
system("mkdir -p $g/configs/c.1/strings/0x409");
my $tp = "/sys/kernel/config/target/usb_gadget/naa.0/tpgt_1";
my $tf;
my $ctrl;
if ($use_ramdisk) {
$tf = "/sys/kernel/config/target/core/rd_mcp_0/ramdisk";
$ctrl = 'rd_pages=524288';
} else {
$tf = "/sys/kernel/config/target/core/fileio_0/fileio";
$ctrl = 'fd_dev_name=$backing_storage,fd_dev_size=$backing_storage_size,fd_async_io=1';
}
system("mkdir -p /etc/target");
system("mkdir -p $tp");
system("mkdir -p $tf");
system("mkdir -p $tp/lun/lun_0");
system("echo naa.0 > $tp/nexus");
system("echo $ctrl > $tf/control");
system("echo 1 > $tf/attrib/emulate_ua_intlck_ctrl");
system("echo 123 > $tf/wwn/vpd_unit_serial");
system("echo $vendor_id > $tf/wwn/vendor_id");
system("echo $product_id > $tf/wwn/product_id");
system("echo $revision > $tf/wwn/revision");
system("echo 1 > $tf/enable");
system("ln -s $tf $tp/lun/lun_0/virtual_scsi_port");
system("echo 1 > $tp/enable");
system("echo $MY_UAS_PID > $g/idProduct");
system("ln -s $g/functions/tcm.0 $g/configs/c.1");
system("echo $MY_UAS_VID > $g/idVendor");
system("echo $SERIAL > $g/strings/0x409/serialnumber");
system("echo $VENDOR > $g/strings/0x409/manufacturer");
system("echo \"$MY_VER\" > $g/strings/0x409/product");
system("echo \"Conf 1\" > $g/configs/c.1/strings/0x409/configuration");
system("echo super-speed-plus > $g/max_speed");
# Make sure the UDC is available
system("echo $my_udc > $g/UDC");
Thinh Nguyen (25):
target: Add overlapped response to tmrsp_table
target: Add common TMR enum
usb: gadget: f_tcm: Increase stream count
usb: gadget: f_tcm: Increase bMaxBurst
usb: gadget: f_tcm: Don't set static stream_id
usb: gadget: f_tcm: Allocate matching number of commands to streams
usb: gadget: f_tcm: Limit number of sessions
usb: gadget: f_tcm: Handle multiple commands in parallel
usb: gadget: f_tcm: Use extra number of commands
usb: gadget: f_tcm: Return ATA cmd direction
usb: gadget: f_tcm: Execute command on write completion
usb: gadget: f_tcm: Minor cleanup redundant code
usb: gadget: f_tcm: Don't free command immediately
usb: gadget: f_tcm: Translate error to sense
usb: gadget: f_tcm: Cleanup unused variable
usb: gadget: f_tcm: Update state on data write
usb: gadget: f_tcm: Handle abort command
usb: gadget: f_tcm: Cleanup requests on ep disable
usb: gadget: f_tcm: Decrement command ref count on cleanup
usb: gadget: f_tcm: Save CPU ID per command
usb: gadget: f_tcm: Get stream by tag
usb: gadget: f_tcm: Send sense on cancelled transfer
usb: gadget: f_tcm: Handle TASK_MANAGEMENT commands
usb: gadget: f_tcm: Check overlapped command
usb: gadget: f_tcm: Comply with UAS Task Management requirement
drivers/target/target_core_transport.c | 10 +
drivers/usb/gadget/function/f_tcm.c | 536 +++++++++++++++++++------
drivers/usb/gadget/function/tcm.h | 20 +-
include/target/target_core_base.h | 5 +
4 files changed, 436 insertions(+), 135 deletions(-)
base-commit: 88a15fbb47db483d06b12b1ae69f114b96361a96
--
2.28.0
Handle target_core_fabric_ops TASK MANAGEMENT functions and their
response. If a TASK MANAGEMENT command is received, the driver will
interpret the function TMF_*, translate to TMR_*, and fire off a command
work executing target_submit_tmr(). On completion, it will handle the
TASK MANAGEMENT response through uasp_send_tm_response().
Signed-off-by: Thinh Nguyen <[email protected]>
---
Changes in v2:
- Splitted into smaller change. Remove handling of overlapped tag.
- Directly respond to host if f_tcm detected failure with response IU rather
than reporting to target core.
drivers/usb/gadget/function/f_tcm.c | 206 +++++++++++++++++++++++++---
drivers/usb/gadget/function/tcm.h | 7 +-
2 files changed, 193 insertions(+), 20 deletions(-)
diff --git a/drivers/usb/gadget/function/f_tcm.c b/drivers/usb/gadget/function/f_tcm.c
index 116be1c47ac9..8b99ee07df87 100644
--- a/drivers/usb/gadget/function/f_tcm.c
+++ b/drivers/usb/gadget/function/f_tcm.c
@@ -12,6 +12,7 @@
#include <linux/string.h>
#include <linux/configfs.h>
#include <linux/ctype.h>
+#include <linux/delay.h>
#include <linux/usb/ch9.h>
#include <linux/usb/composite.h>
#include <linux/usb/gadget.h>
@@ -462,6 +463,51 @@ static int usbg_bot_setup(struct usb_function *f,
/* Start uas.c code */
+static int tcm_to_uasp_response(enum tcm_tmrsp_table code)
+{
+ switch (code) {
+ case TMR_FUNCTION_FAILED:
+ return RC_TMF_FAILED;
+ case TMR_FUNCTION_COMPLETE:
+ case TMR_TASK_DOES_NOT_EXIST:
+ return RC_TMF_COMPLETE;
+ case TMR_LUN_DOES_NOT_EXIST:
+ return RC_INCORRECT_LUN;
+ case TMR_OVERLAPPED_TAG_ATTEMPTED:
+ return RC_OVERLAPPED_TAG;
+ case TMR_FUNCTION_REJECTED:
+ case TMR_TASK_MGMT_FUNCTION_NOT_SUPPORTED:
+ default:
+ return RC_TMF_NOT_SUPPORTED;
+ }
+}
+
+static unsigned char uasp_to_tcm_func(int code)
+{
+ switch (code) {
+ case TMF_ABORT_TASK:
+ return TMR_ABORT_TASK;
+ case TMF_ABORT_TASK_SET:
+ return TMR_ABORT_TASK_SET;
+ case TMF_CLEAR_TASK_SET:
+ return TMR_CLEAR_TASK_SET;
+ case TMF_LOGICAL_UNIT_RESET:
+ return TMR_LUN_RESET;
+ case TMF_I_T_NEXUS_RESET:
+ return TMR_I_T_NEXUS_RESET;
+ case TMF_CLEAR_ACA:
+ return TMR_CLEAR_ACA;
+ case TMF_QUERY_TASK:
+ return TMR_QUERY_TASK;
+ case TMF_QUERY_TASK_SET:
+ return TMR_QUERY_TASK_SET;
+ case TMF_QUERY_ASYNC_EVENT:
+ return TMR_QUERY_ASYNC_EVENT;
+ default:
+ return TMR_UNKNOWN;
+ }
+}
+
static void uasp_cleanup_one_stream(struct f_uas *fu, struct uas_stream *stream)
{
/* We have either all three allocated or none */
@@ -581,6 +627,33 @@ static void uasp_prepare_status(struct usbg_cmd *cmd)
stream->req_status->complete = uasp_status_data_cmpl;
}
+static void uasp_prepare_response(struct usbg_cmd *cmd)
+{
+ struct se_cmd *se_cmd = &cmd->se_cmd;
+ struct response_iu *rsp_iu = &cmd->response_iu;
+ struct uas_stream *stream = uasp_get_stream_by_tag(cmd->fu, cmd->tag);
+
+ cmd->state = UASP_QUEUE_COMMAND;
+ rsp_iu->iu_id = IU_ID_RESPONSE;
+ rsp_iu->tag = cpu_to_be16(cmd->tag);
+
+ if (cmd->tmr_rsp != RC_RESPONSE_UNKNOWN)
+ rsp_iu->response_code = cmd->tmr_rsp;
+ else
+ rsp_iu->response_code =
+ tcm_to_uasp_response(se_cmd->se_tmr_req->response);
+
+ stream->req_status->is_last = 1;
+ stream->req_status->stream_id = cmd->tag;
+ stream->req_status->context = cmd;
+ stream->req_status->length = sizeof(struct response_iu);
+ stream->req_status->buf = rsp_iu;
+ stream->req_status->complete = uasp_status_data_cmpl;
+}
+
+static void usbg_release_cmd(struct se_cmd *se_cmd);
+static int uasp_send_tm_response(struct usbg_cmd *cmd);
+
static void uasp_status_data_cmpl(struct usb_ep *ep, struct usb_request *req)
{
struct usbg_cmd *cmd = req->context;
@@ -619,9 +692,26 @@ static void uasp_status_data_cmpl(struct usb_ep *ep, struct usb_request *req)
break;
case UASP_QUEUE_COMMAND:
- transport_generic_free_cmd(&cmd->se_cmd, 0);
- usb_ep_queue(fu->ep_cmd, cmd->req, GFP_ATOMIC);
+ stream->cmd = NULL;
+
+ /*
+ * If no command submitted to target core here, just free the
+ * bitmap index. This is for the cases where f_tcm handles
+ * status response instead of the target core.
+ */
+ if (cmd->tmr_rsp != RC_RESPONSE_UNKNOWN) {
+ struct se_session *se_sess;
+
+ se_sess = fu->tpg->tpg_nexus->tvn_se_sess;
+ sbitmap_queue_clear(&se_sess->sess_tag_pool,
+ cmd->se_cmd.map_tag,
+ cmd->se_cmd.map_cpu);
+ } else {
+ transport_generic_free_cmd(&cmd->se_cmd, 0);
+ }
+
+ usb_ep_queue(fu->ep_cmd, cmd->req, GFP_ATOMIC);
break;
default:
@@ -630,6 +720,7 @@ static void uasp_status_data_cmpl(struct usb_ep *ep, struct usb_request *req)
return;
cleanup:
+ stream->cmd = NULL;
transport_generic_free_cmd(&cmd->se_cmd, 0);
}
@@ -645,6 +736,18 @@ static int uasp_send_status_response(struct usbg_cmd *cmd)
return usb_ep_queue(fu->ep_status, stream->req_status, GFP_ATOMIC);
}
+static int uasp_send_tm_response(struct usbg_cmd *cmd)
+{
+ struct f_uas *fu = cmd->fu;
+ struct uas_stream *stream = uasp_get_stream_by_tag(fu, cmd->tag);
+ struct response_iu *iu = &cmd->response_iu;
+
+ iu->tag = cpu_to_be16(cmd->tag);
+ cmd->fu = fu;
+ uasp_prepare_response(cmd);
+ return usb_ep_queue(fu->ep_status, stream->req_status, GFP_ATOMIC);
+}
+
static int uasp_send_read_response(struct usbg_cmd *cmd)
{
struct f_uas *fu = cmd->fu;
@@ -1045,9 +1148,24 @@ static int usbg_send_read_response(struct se_cmd *se_cmd)
return uasp_send_read_response(cmd);
}
-static void usbg_cmd_work(struct work_struct *work)
+static void usbg_submit_tmr(struct usbg_cmd *cmd)
{
struct usbg_cmd *cmd = container_of(work, struct usbg_cmd, work);
+ struct se_session *se_sess;
+ struct se_cmd *se_cmd;
+ int flags = TARGET_SCF_ACK_KREF;
+
+ se_cmd = &cmd->se_cmd;
+ se_sess = cmd->fu->tpg->tpg_nexus->tvn_se_sess;
+
+ target_submit_tmr(se_cmd, se_sess,
+ cmd->response_iu.add_response_info,
+ cmd->unpacked_lun, NULL, cmd->tmr_func,
+ GFP_ATOMIC, cmd->tag, flags);
+}
+
+static void usbg_submit_cmd(struct usbg_cmd *cmd)
+{
struct se_cmd *se_cmd;
struct tcm_usbg_nexus *tv_nexus;
struct usbg_tpg *tpg;
@@ -1076,6 +1194,29 @@ static void usbg_cmd_work(struct work_struct *work)
TCM_UNSUPPORTED_SCSI_OPCODE, 0);
}
+static void usbg_cmd_work(struct work_struct *work)
+{
+ struct usbg_cmd *cmd = container_of(work, struct usbg_cmd, work);
+
+ /*
+ * Failure is detected by f_tcm here. Skip submitting the command to the
+ * target core if we already know the failing response and send the usb
+ * response to the host directly.
+ */
+ if (cmd->tmr_rsp != RC_RESPONSE_UNKNOWN)
+ goto skip;
+
+ if (cmd->tmr_func)
+ usbg_submit_tmr(cmd);
+ else
+ usbg_submit_cmd(cmd);
+
+ return;
+
+skip:
+ uasp_send_tm_response(cmd);
+}
+
static struct usbg_cmd *usbg_get_cmd(struct f_uas *fu,
struct tcm_usbg_nexus *tv_nexus, u32 scsi_tag)
{
@@ -1102,37 +1243,63 @@ static void usbg_release_cmd(struct se_cmd *);
static int usbg_submit_command(struct f_uas *fu, struct usb_request *req)
{
- struct command_iu *cmd_iu = req->buf;
+ struct iu *iu = req->buf;
struct usbg_cmd *cmd;
struct usbg_tpg *tpg = fu->tpg;
struct tcm_usbg_nexus *tv_nexus;
+ struct uas_stream *stream;
+ struct command_iu *cmd_iu;
u32 cmd_len;
u16 scsi_tag;
- if (cmd_iu->iu_id != IU_ID_COMMAND) {
- pr_err("Unsupported type %d\n", cmd_iu->iu_id);
- return -EINVAL;
- }
-
tv_nexus = tpg->tpg_nexus;
if (!tv_nexus) {
pr_err("Missing nexus, ignoring command\n");
return -EINVAL;
}
- cmd_len = (cmd_iu->len & ~0x3) + 16;
- if (cmd_len > USBG_MAX_CMD)
- return -EINVAL;
-
- scsi_tag = be16_to_cpup(&cmd_iu->tag);
+ scsi_tag = be16_to_cpup(&iu->tag);
cmd = usbg_get_cmd(fu, tv_nexus, scsi_tag);
if (IS_ERR(cmd)) {
pr_err("usbg_get_cmd failed\n");
return -ENOMEM;
}
- memcpy(cmd->cmd_buf, cmd_iu->cdb, cmd_len);
- cmd->stream = &fu->stream[cmd->tag % USBG_NUM_CMDS];
+ cmd->req = req;
+ cmd->fu = fu;
+ cmd->tag = scsi_tag;
+ cmd->se_cmd.tag = scsi_tag;
+ cmd->tmr_func = 0;
+ cmd->tmr_rsp = RC_RESPONSE_UNKNOWN;
+
+ cmd_iu = (struct command_iu *)iu;
+
+ /* Command and Task Management IUs share the same LUN offset */
+ cmd->unpacked_lun = scsilun_to_int(&cmd_iu->lun);
+
+ if (iu->iu_id != IU_ID_COMMAND && iu->iu_id != IU_ID_TASK_MGMT) {
+ cmd->tmr_rsp = RC_INVALID_INFO_UNIT;
+ goto skip;
+ }
+
+ stream->cmd = cmd;
+
+ if (iu->iu_id == IU_ID_TASK_MGMT) {
+ struct task_mgmt_iu *tm_iu;
+
+ tm_iu = (struct task_mgmt_iu *)iu;
+ cmd->tmr_func = uasp_to_tcm_func(tm_iu->function);
+ goto skip;
+ }
+
+ cmd_len = (cmd_iu->len & ~0x3) + 16;
+ if (cmd_len > USBG_MAX_CMD) {
+ pr_err("invalid len %d\n", cmd_len);
+ target_free_tag(tv_nexus->tvn_se_sess, &cmd->se_cmd);
+ stream->cmd = NULL;
+ return -EINVAL;
+ }
+ memcpy(cmd->cmd_buf, cmd_iu->cdb, cmd_len);
switch (cmd_iu->prio_attr & 0x7) {
case UAS_HEAD_TAG:
@@ -1153,9 +1320,7 @@ static int usbg_submit_command(struct f_uas *fu, struct usb_request *req)
break;
}
- cmd->unpacked_lun = scsilun_to_int(&cmd_iu->lun);
- cmd->req = req;
-
+skip:
INIT_WORK(&cmd->work, usbg_cmd_work);
queue_work(tpg->workqueue, &cmd->work);
@@ -1301,6 +1466,9 @@ static int usbg_get_cmd_state(struct se_cmd *se_cmd)
static void usbg_queue_tm_rsp(struct se_cmd *se_cmd)
{
+ struct usbg_cmd *cmd = container_of(se_cmd, struct usbg_cmd, se_cmd);
+
+ uasp_send_tm_response(cmd);
}
static void usbg_aborted_task(struct se_cmd *se_cmd)
diff --git a/drivers/usb/gadget/function/tcm.h b/drivers/usb/gadget/function/tcm.h
index 5157af1b166b..7fafc4336984 100644
--- a/drivers/usb/gadget/function/tcm.h
+++ b/drivers/usb/gadget/function/tcm.h
@@ -82,8 +82,11 @@ struct usbg_cmd {
u16 tag;
u16 prio_attr;
struct sense_iu sense_iu;
+ struct response_iu response_iu;
enum uas_state state;
- struct uas_stream *stream;
+ int tmr_func;
+ int tmr_rsp;
+#define RC_RESPONSE_UNKNOWN 0xff
/* BOT only */
__le32 bot_tag;
@@ -96,6 +99,8 @@ struct uas_stream {
struct usb_request *req_in;
struct usb_request *req_out;
struct usb_request *req_status;
+
+ struct usbg_cmd *cmd;
};
struct usbg_cdb {
--
2.28.0
In preparation for TASK MANAGEMENT handling of overlapped command,
implement uasp_get_stream_by_tag() to find the active stream/command
based on a given tag.
For simplicity, we use mod operation to quickly find an in-progress
matching command tag to check for overlapped command. The assumption is
that the UASP class driver will limit to using tag id from 1 to
USBG_NUM_CMDS. This is based on observation from the Windows and Linux
UASP storage class driver behavior. If an unusual UASP class driver uses
a tag greater than USBG_NUM_CMDS, then this method may no longer work
due to possible stream id collision. In that case, we need to use a
proper algorithm to fetch the stream (or simply walk through all active
streams to check for overlap).
Signed-off-by: Thinh Nguyen <[email protected]>
---
Changes in v2:
- Splitted from TASK MANAGEMENT change
drivers/usb/gadget/function/f_tcm.c | 30 ++++++++++++++++++++++-------
1 file changed, 23 insertions(+), 7 deletions(-)
diff --git a/drivers/usb/gadget/function/f_tcm.c b/drivers/usb/gadget/function/f_tcm.c
index 084143213176..a10e74290664 100644
--- a/drivers/usb/gadget/function/f_tcm.c
+++ b/drivers/usb/gadget/function/f_tcm.c
@@ -506,6 +506,22 @@ static void uasp_cleanup_old_alt(struct f_uas *fu)
uasp_free_cmdreq(fu);
}
+static struct uas_stream *uasp_get_stream_by_tag(struct f_uas *fu, u16 tag)
+{
+ /*
+ * For simplicity, we use mod operation to quickly find an in-progress
+ * matching command tag to check for overlapped command. The assumption
+ * is that the UASP class driver will limit to using tag id from 1 to
+ * USBG_NUM_CMDS. This is based on observation from the Windows and
+ * Linux UASP storage class driver behavior. If an unusual UASP class
+ * driver uses a tag greater than USBG_NUM_CMDS, then this method may no
+ * longer work due to possible stream id collision. In that case, we
+ * need to use a proper algorithm to fetch the stream (or simply walk
+ * through all active streams to check for overlap).
+ */
+ return &fu->stream[tag % USBG_NUM_CMDS];
+}
+
static void uasp_status_data_cmpl(struct usb_ep *ep, struct usb_request *req);
static int uasp_prepare_r_request(struct usbg_cmd *cmd)
@@ -513,7 +529,7 @@ static int uasp_prepare_r_request(struct usbg_cmd *cmd)
struct se_cmd *se_cmd = &cmd->se_cmd;
struct f_uas *fu = cmd->fu;
struct usb_gadget *gadget = fuas_to_gadget(fu);
- struct uas_stream *stream = cmd->stream;
+ struct uas_stream *stream = uasp_get_stream_by_tag(fu, cmd->tag);
if (!gadget->sg_supported) {
cmd->data_buf = kmalloc(se_cmd->data_length, GFP_ATOMIC);
@@ -546,7 +562,7 @@ static void uasp_prepare_status(struct usbg_cmd *cmd)
{
struct se_cmd *se_cmd = &cmd->se_cmd;
struct sense_iu *iu = &cmd->sense_iu;
- struct uas_stream *stream = cmd->stream;
+ struct uas_stream *stream = uasp_get_stream_by_tag(cmd->fu, cmd->tag);
cmd->state = UASP_QUEUE_COMMAND;
iu->iu_id = IU_ID_STATUS;
@@ -568,8 +584,8 @@ static void uasp_prepare_status(struct usbg_cmd *cmd)
static void uasp_status_data_cmpl(struct usb_ep *ep, struct usb_request *req)
{
struct usbg_cmd *cmd = req->context;
- struct uas_stream *stream = cmd->stream;
struct f_uas *fu = cmd->fu;
+ struct uas_stream *stream = uasp_get_stream_by_tag(fu, cmd->tag);
int ret;
if (req->status == -ESHUTDOWN)
@@ -620,7 +636,7 @@ static void uasp_status_data_cmpl(struct usb_ep *ep, struct usb_request *req)
static int uasp_send_status_response(struct usbg_cmd *cmd)
{
struct f_uas *fu = cmd->fu;
- struct uas_stream *stream = cmd->stream;
+ struct uas_stream *stream = uasp_get_stream_by_tag(fu, cmd->tag);
struct sense_iu *iu = &cmd->sense_iu;
iu->tag = cpu_to_be16(cmd->tag);
@@ -632,7 +648,7 @@ static int uasp_send_status_response(struct usbg_cmd *cmd)
static int uasp_send_read_response(struct usbg_cmd *cmd)
{
struct f_uas *fu = cmd->fu;
- struct uas_stream *stream = cmd->stream;
+ struct uas_stream *stream = uasp_get_stream_by_tag(fu, cmd->tag);
struct sense_iu *iu = &cmd->sense_iu;
int ret;
@@ -675,7 +691,7 @@ static int uasp_send_read_response(struct usbg_cmd *cmd)
static int uasp_send_write_request(struct usbg_cmd *cmd)
{
struct f_uas *fu = cmd->fu;
- struct uas_stream *stream = cmd->stream;
+ struct uas_stream *stream = uasp_get_stream_by_tag(fu, cmd->tag);
struct sense_iu *iu = &cmd->sense_iu;
int ret;
@@ -1285,7 +1301,7 @@ static void usbg_aborted_task(struct se_cmd *se_cmd)
{
struct usbg_cmd *cmd = container_of(se_cmd, struct usbg_cmd, se_cmd);
struct f_uas *fu = cmd->fu;
- struct uas_stream *stream = cmd->stream;
+ struct uas_stream *stream = uasp_get_stream_by_tag(fu, cmd->tag);
int ret = 0;
if (stream->req_out->status == -EINPROGRESS)
--
2.28.0
If there's an overlapped command tag, cancel the command and respond
with RC_OVERLAPPED_TAG to host.
Signed-off-by: Thinh Nguyen <[email protected]>
---
Changes in v2:
- New patch. It was part of TASK MANAGEMENT change.
- Directly respond to host on overlapped tag instead of reporting to target
core.
drivers/usb/gadget/function/f_tcm.c | 75 ++++++++++++++++++++++++++++-
1 file changed, 73 insertions(+), 2 deletions(-)
diff --git a/drivers/usb/gadget/function/f_tcm.c b/drivers/usb/gadget/function/f_tcm.c
index 8b99ee07df87..dfa3e7c043a3 100644
--- a/drivers/usb/gadget/function/f_tcm.c
+++ b/drivers/usb/gadget/function/f_tcm.c
@@ -692,6 +692,15 @@ static void uasp_status_data_cmpl(struct usb_ep *ep, struct usb_request *req)
break;
case UASP_QUEUE_COMMAND:
+ /*
+ * Overlapped command detected and cancelled.
+ * So send overlapped attempted status.
+ */
+ if (cmd->tmr_rsp == RC_OVERLAPPED_TAG &&
+ req->status == -ECONNRESET) {
+ uasp_send_tm_response(cmd);
+ return;
+ }
stream->cmd = NULL;
@@ -700,7 +709,8 @@ static void uasp_status_data_cmpl(struct usb_ep *ep, struct usb_request *req)
* bitmap index. This is for the cases where f_tcm handles
* status response instead of the target core.
*/
- if (cmd->tmr_rsp != RC_RESPONSE_UNKNOWN) {
+ if (cmd->tmr_rsp != RC_OVERLAPPED_TAG &&
+ cmd->tmr_rsp != RC_RESPONSE_UNKNOWN) {
struct se_session *se_sess;
se_sess = fu->tpg->tpg_nexus->tvn_se_sess;
@@ -1080,6 +1090,14 @@ static void usbg_data_write_cmpl(struct usb_ep *ep, struct usb_request *req)
cleanup:
target_put_sess_cmd(se_cmd);
+
+ /* Command was aborted due to overlapped tag */
+ if (cmd->state == UASP_QUEUE_COMMAND &&
+ cmd->tmr_rsp == RC_OVERLAPPED_TAG) {
+ uasp_send_tm_response(cmd);
+ return;
+ }
+
transport_send_check_condition_and_sense(se_cmd,
TCM_CHECK_CONDITION_ABORT_CMD, 0);
}
@@ -1148,9 +1166,10 @@ static int usbg_send_read_response(struct se_cmd *se_cmd)
return uasp_send_read_response(cmd);
}
+static void usbg_aborted_task(struct se_cmd *se_cmd);
+
static void usbg_submit_tmr(struct usbg_cmd *cmd)
{
- struct usbg_cmd *cmd = container_of(work, struct usbg_cmd, work);
struct se_session *se_sess;
struct se_cmd *se_cmd;
int flags = TARGET_SCF_ACK_KREF;
@@ -1214,6 +1233,51 @@ static void usbg_cmd_work(struct work_struct *work)
return;
skip:
+ if (cmd->tmr_rsp == RC_OVERLAPPED_TAG) {
+ struct se_session *se_sess;
+ struct uas_stream *stream;
+
+ se_sess = cmd->fu->tpg->tpg_nexus->tvn_se_sess;
+ stream = uasp_get_stream_by_tag(cmd->fu, cmd->tag);
+
+ /*
+ * There's no guarantee of a matching completion order between
+ * different endpoints. i.e. The device may receive a new (CDB)
+ * command request completion of the command endpoint before it
+ * gets notified of the previous command status completion from
+ * a status endpoint. The driver still needs to detect
+ * misbehaving host and respond with an overlap command tag. To
+ * prevent false overlapped tag failure, give the active and
+ * matching stream id a short time (1ms) to complete before
+ * respond with overlapped command failure.
+ */
+ msleep(1);
+
+ /* If the stream is completed, retry the command. */
+ if (!stream->cmd) {
+ usbg_submit_command(cmd->fu, cmd->req);
+ return;
+ }
+
+ /*
+ * The command isn't submitted to the target core, so we're safe
+ * to remove the bitmap index from the session tag pool.
+ */
+ sbitmap_queue_clear(&se_sess->sess_tag_pool,
+ cmd->se_cmd.map_tag,
+ cmd->se_cmd.map_cpu);
+
+ /*
+ * Overlap command tag detected. Cancel any pending transfer of
+ * the command submitted to target core.
+ */
+ stream->cmd->tmr_rsp = RC_OVERLAPPED_TAG;
+ usbg_aborted_task(&stream->cmd->se_cmd);
+
+ /* Send the response after the transfer is aborted. */
+ return;
+ }
+
uasp_send_tm_response(cmd);
}
@@ -1282,6 +1346,13 @@ static int usbg_submit_command(struct f_uas *fu, struct usb_request *req)
goto skip;
}
+ stream = uasp_get_stream_by_tag(fu, scsi_tag);
+ if (stream->cmd) {
+ pr_err("Command tag %d overlapped\n", scsi_tag);
+ cmd->tmr_rsp = RC_OVERLAPPED_TAG;
+ goto skip;
+ }
+
stream->cmd = cmd;
if (iu->iu_id == IU_ID_TASK_MGMT) {
--
2.28.0
We submitted the command with TARGET_SCF_ACK_KREF, which requires
acknowledgment of command completion. If the command fails, make sure to
decrement the ref count.
Fixes: cff834c16d23 ("usb-gadget/tcm: Convert to TARGET_SCF_ACK_KREF I/O krefs")
Signed-off-by: Thinh Nguyen <[email protected]>
---
Changes in v2:
- New patch
drivers/usb/gadget/function/f_tcm.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/usb/gadget/function/f_tcm.c b/drivers/usb/gadget/function/f_tcm.c
index a8a730e5126d..6cf95c9723a3 100644
--- a/drivers/usb/gadget/function/f_tcm.c
+++ b/drivers/usb/gadget/function/f_tcm.c
@@ -955,6 +955,7 @@ static void usbg_data_write_cmpl(struct usb_ep *ep, struct usb_request *req)
return;
cleanup:
+ target_put_sess_cmd(se_cmd);
transport_generic_free_cmd(&cmd->se_cmd, 0);
}
--
2.28.0
Some old builds of Microsoft Windows 10 UASP class driver reject USAP
device with stream count of 2^4. To keep compatibility with both Linux
and Windows, let's increase the stream count to 2^5. Also, internal
tests show that stream count of 2^5 increases performance slightly.
Signed-off-by: Thinh Nguyen <[email protected]>
---
Changes in v2:
- None
drivers/usb/gadget/function/tcm.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/usb/gadget/function/tcm.h b/drivers/usb/gadget/function/tcm.h
index 3cd565794ad7..6cb05dcd19ff 100644
--- a/drivers/usb/gadget/function/tcm.h
+++ b/drivers/usb/gadget/function/tcm.h
@@ -13,7 +13,7 @@
#define USBG_NAMELEN 32
#define fuas_to_gadget(f) (f->function.config->cdev->gadget)
-#define UASP_SS_EP_COMP_LOG_STREAMS 4
+#define UASP_SS_EP_COMP_LOG_STREAMS 5
#define UASP_SS_EP_COMP_NUM_STREAMS (1 << UASP_SS_EP_COMP_LOG_STREAMS)
enum {
--
2.28.0
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]>
---
Changes in v2:
- None
drivers/usb/gadget/function/f_tcm.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/drivers/usb/gadget/function/f_tcm.c b/drivers/usb/gadget/function/f_tcm.c
index 53b178ad4f90..cace5746c0f9 100644
--- a/drivers/usb/gadget/function/f_tcm.c
+++ b/drivers/usb/gadget/function/f_tcm.c
@@ -1045,7 +1045,6 @@ static void usbg_cmd_work(struct work_struct *work)
out:
transport_send_check_condition_and_sense(se_cmd,
TCM_UNSUPPORTED_SCSI_OPCODE, 1);
- transport_generic_free_cmd(&cmd->se_cmd, 0);
}
static struct usbg_cmd *usbg_get_cmd(struct f_uas *fu,
--
2.28.0
If the endpoint is disabled, then free the command. Normally we don't
free a command until it's completed its UASP status (failure or not).
Signed-off-by: Thinh Nguyen <[email protected]>
---
Changes in v2:
- Use goto cleanup for a cleaner look.
drivers/usb/gadget/function/f_tcm.c | 7 +++++--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/drivers/usb/gadget/function/f_tcm.c b/drivers/usb/gadget/function/f_tcm.c
index 8f7eb7045e64..a8a730e5126d 100644
--- a/drivers/usb/gadget/function/f_tcm.c
+++ b/drivers/usb/gadget/function/f_tcm.c
@@ -572,7 +572,7 @@ static void uasp_status_data_cmpl(struct usb_ep *ep, struct usb_request *req)
struct f_uas *fu = cmd->fu;
int ret;
- if (req->status < 0)
+ if (req->status == -ESHUTDOWN)
goto cleanup;
switch (cmd->state) {
@@ -936,7 +936,10 @@ static void usbg_data_write_cmpl(struct usb_ep *ep, struct usb_request *req)
cmd->state = UASP_QUEUE_COMMAND;
- if (req->status < 0) {
+ if (req->status == -ESHUTDOWN)
+ goto cleanup;
+
+ if (req->status) {
pr_err("%s() state %d transfer failed\n", __func__, cmd->state);
goto cleanup;
}
--
2.28.0
Don't just wait for the data write completion and execute the target
command. We need to verify if the request completed successfully and not
just sending invalid data. The verification is done in the write request
completion routine, so we can just run target_execute_cmd() there. The
wait for the data write is not needed.
Signed-off-by: Thinh Nguyen <[email protected]>
---
Changes in v2:
- None
drivers/usb/gadget/function/f_tcm.c | 8 +-------
drivers/usb/gadget/function/tcm.h | 1 -
2 files changed, 1 insertion(+), 8 deletions(-)
diff --git a/drivers/usb/gadget/function/f_tcm.c b/drivers/usb/gadget/function/f_tcm.c
index 6fea80afe2d7..ec83f2f9a858 100644
--- a/drivers/usb/gadget/function/f_tcm.c
+++ b/drivers/usb/gadget/function/f_tcm.c
@@ -248,7 +248,6 @@ static int bot_send_write_request(struct usbg_cmd *cmd)
struct usb_gadget *gadget = fuas_to_gadget(fu);
int ret;
- init_completion(&cmd->write_complete);
cmd->fu = fu;
if (!cmd->data_len) {
@@ -279,8 +278,6 @@ static int bot_send_write_request(struct usbg_cmd *cmd)
if (ret)
pr_err("%s(%d)\n", __func__, __LINE__);
- wait_for_completion(&cmd->write_complete);
- target_execute_cmd(se_cmd);
cleanup:
return ret;
}
@@ -685,7 +682,6 @@ static int uasp_send_write_request(struct usbg_cmd *cmd)
struct sense_iu *iu = &cmd->sense_iu;
int ret;
- init_completion(&cmd->write_complete);
cmd->fu = fu;
iu->tag = cpu_to_be16(cmd->tag);
@@ -717,8 +713,6 @@ static int uasp_send_write_request(struct usbg_cmd *cmd)
pr_err("%s(%d)\n", __func__, __LINE__);
}
- wait_for_completion(&cmd->write_complete);
- target_execute_cmd(se_cmd);
cleanup:
return ret;
}
@@ -955,7 +949,7 @@ static void usbg_data_write_cmpl(struct usb_ep *ep, struct usb_request *req)
se_cmd->data_length);
}
- complete(&cmd->write_complete);
+ target_execute_cmd(se_cmd);
return;
cleanup:
diff --git a/drivers/usb/gadget/function/tcm.h b/drivers/usb/gadget/function/tcm.h
index c7e6d36afd3a..5157af1b166b 100644
--- a/drivers/usb/gadget/function/tcm.h
+++ b/drivers/usb/gadget/function/tcm.h
@@ -74,7 +74,6 @@ struct usbg_cmd {
struct se_cmd se_cmd;
void *data_buf; /* used if no sg support available */
struct f_uas *fu;
- struct completion write_complete;
struct kref ref;
struct usb_request *req;
--
2.28.0
Normally we don't care about the CPU id, but if we ever use
TARGET_SCF_USE_CPUID, then we need to save the cpuid.
Signed-off-by: Thinh Nguyen <[email protected]>
---
Changes in v2:
- None
drivers/usb/gadget/function/f_tcm.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/usb/gadget/function/f_tcm.c b/drivers/usb/gadget/function/f_tcm.c
index 6cf95c9723a3..084143213176 100644
--- a/drivers/usb/gadget/function/f_tcm.c
+++ b/drivers/usb/gadget/function/f_tcm.c
@@ -1069,6 +1069,7 @@ static struct usbg_cmd *usbg_get_cmd(struct f_uas *fu,
memset(cmd, 0, sizeof(*cmd));
cmd->se_cmd.map_tag = tag;
cmd->se_cmd.map_cpu = cpu;
+ cmd->se_cmd.cpuid = cpu;
cmd->se_cmd.tag = cmd->tag = scsi_tag;
cmd->fu = fu;
--
2.28.0
Only allocate up to USBG_NUM_CMDS number of session tags. We should not
be using more than USBG_NUM_CMDS of tags due to the number of commands
limit we imposed. Each command uses a unique tag. Any more than that is
unnecessary. By limitting it, we can detect an issue in our driver
immediately should we run out of session tags.
Signed-off-by: Thinh Nguyen <[email protected]>
---
Changes in v2:
- Add more comments in change log for the change reason.
drivers/usb/gadget/function/tcm.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/usb/gadget/function/tcm.h b/drivers/usb/gadget/function/tcm.h
index bcbe35bb5015..df768559fb60 100644
--- a/drivers/usb/gadget/function/tcm.h
+++ b/drivers/usb/gadget/function/tcm.h
@@ -26,7 +26,7 @@ enum {
#define USB_G_ALT_INT_BBB 0
#define USB_G_ALT_INT_UAS 1
-#define USB_G_DEFAULT_SESSION_TAGS 128
+#define USB_G_DEFAULT_SESSION_TAGS USBG_NUM_CMDS
struct tcm_usbg_nexus {
struct se_session *tvn_se_sess;
--
2.28.0
Hello!
On 7/19/22 4:26 AM, Thinh Nguyen wrote:
> Some old builds of Microsoft Windows 10 UASP class driver reject USAP
UASP?
> device with stream count of 2^4. To keep compatibility with both Linux
> and Windows, let's increase the stream count to 2^5. Also, internal
> tests show that stream count of 2^5 increases performance slightly.
>
> Signed-off-by: Thinh Nguyen <[email protected]>
[...]
MBR, Sergey
On Mon, Jul 18, 2022 at 06:26:01PM -0700, Thinh Nguyen wrote:
> The Linux UASP gadget driver is incomplete and remained broken for a long time.
> It was not implemented for performance either. This series adds some of the
> required features for the UASP driver to work. It also makes some changes to
> the target core.
>
> This is tested against UASP CV and DWC_usb3x controller. It still needs some
> fixes in the target core, which will be separated from this series.
>
> There are still more room for performance improvement and fixes. However, this
> series should be sufficient to bring up a working UASP device.
>
>
> Changes in v2:
> - Remove most target core changes from this series and only keep the must-have
> ones
> - Split the task-management patch to smaller patches
> - Don't send failure Task Management response to target core, reducing
> dependency
> - Add UASP bringup script example in cover page
> - Make various small updates according to feedbacks
I would need a review by the target maintainers before being able to
take any of the USB gadget changes into the USB tree...
thanks,
greg k-h
Hi Martin, Dmitry, and others,
On Fri, Aug 19, 2022 at 10:31:23AM +0200, Greg Kroah-Hartman wrote:
> On Mon, Jul 18, 2022 at 06:26:01PM -0700, Thinh Nguyen wrote:
> > The Linux UASP gadget driver is incomplete and remained broken for a long time.
> > It was not implemented for performance either. This series adds some of the
> > required features for the UASP driver to work. It also makes some changes to
> > the target core.
> >
> > This is tested against UASP CV and DWC_usb3x controller. It still needs some
> > fixes in the target core, which will be separated from this series.
> >
> > There are still more room for performance improvement and fixes. However, this
> > series should be sufficient to bring up a working UASP device.
> >
> >
> > Changes in v2:
> > - Remove most target core changes from this series and only keep the must-have
> > ones
> > - Split the task-management patch to smaller patches
> > - Don't send failure Task Management response to target core, reducing
> > dependency
> > - Add UASP bringup script example in cover page
> > - Make various small updates according to feedbacks
>
> I would need a review by the target maintainers before being able to
> take any of the USB gadget changes into the USB tree...
>
Do you have any comment on this series?
Thanks,
Thinh
On Fri, Aug 19, 2022, Thinh Nguyen wrote:
> Hi Martin, Dmitry, and others,
>
> On Fri, Aug 19, 2022 at 10:31:23AM +0200, Greg Kroah-Hartman wrote:
> > On Mon, Jul 18, 2022 at 06:26:01PM -0700, Thinh Nguyen wrote:
> > > The Linux UASP gadget driver is incomplete and remained broken for a long time.
> > > It was not implemented for performance either. This series adds some of the
> > > required features for the UASP driver to work. It also makes some changes to
> > > the target core.
> > >
> > > This is tested against UASP CV and DWC_usb3x controller. It still needs some
> > > fixes in the target core, which will be separated from this series.
> > >
> > > There are still more room for performance improvement and fixes. However, this
> > > series should be sufficient to bring up a working UASP device.
> > >
> > >
> > > Changes in v2:
> > > - Remove most target core changes from this series and only keep the must-have
> > > ones
> > > - Split the task-management patch to smaller patches
> > > - Don't send failure Task Management response to target core, reducing
> > > dependency
> > > - Add UASP bringup script example in cover page
> > > - Make various small updates according to feedbacks
> >
> > I would need a review by the target maintainers before being able to
> > take any of the USB gadget changes into the USB tree...
> >
>
> Do you have any comment on this series?
>
Hi target maintainers,
Gentle ping...
BR,
Thinh
On 2022-07-18 18:27:12 [-0700], Thinh Nguyen wrote:
> index 6fea80afe2d7..ec83f2f9a858 100644
> --- a/drivers/usb/gadget/function/f_tcm.c
> +++ b/drivers/usb/gadget/function/f_tcm.c
> @@ -955,7 +949,7 @@ static void usbg_data_write_cmpl(struct usb_ep *ep, struct usb_request *req)
> se_cmd->data_length);
> }
>
> - complete(&cmd->write_complete);
> + target_execute_cmd(se_cmd);
usbg_data_write_cmpl() is invoked from interrupt service routing which
may run with disabled interrupts. From looking at target_execute_cmd():
| void target_execute_cmd(struct se_cmd *cmd)
| {
…
| spin_lock_irq(&cmd->t_state_lock);
…
| spin_unlock_irq(&cmd->t_state_lock);
…
| }
which means interrupts will remain open after leaving
target_execute_cmd(). Now, why didn't the WARN_ONCE() in
__handle_irq_event_percpu() trigger? Am I missing something?
> return;
Sebastian
On 2022-07-18 18:28:16 [-0700], Thinh Nguyen wrote:
> diff --git a/drivers/usb/gadget/function/f_tcm.c b/drivers/usb/gadget/function/f_tcm.c
> index 084143213176..a10e74290664 100644
> --- a/drivers/usb/gadget/function/f_tcm.c
> +++ b/drivers/usb/gadget/function/f_tcm.c
> @@ -506,6 +506,22 @@ static void uasp_cleanup_old_alt(struct f_uas *fu)
> uasp_free_cmdreq(fu);
> }
>
> +static struct uas_stream *uasp_get_stream_by_tag(struct f_uas *fu, u16 tag)
> +{
> + /*
> + * For simplicity, we use mod operation to quickly find an in-progress
> + * matching command tag to check for overlapped command. The assumption
> + * is that the UASP class driver will limit to using tag id from 1 to
> + * USBG_NUM_CMDS. This is based on observation from the Windows and
> + * Linux UASP storage class driver behavior. If an unusual UASP class
> + * driver uses a tag greater than USBG_NUM_CMDS, then this method may no
> + * longer work due to possible stream id collision. In that case, we
> + * need to use a proper algorithm to fetch the stream (or simply walk
> + * through all active streams to check for overlap).
> + */
> + return &fu->stream[tag % USBG_NUM_CMDS];
Could you please avoid the assumption what tag actually is?
Please take a look at hashtable.h, hash_add(), hash_del(),
hash_for_each_possible_safe() is probably all you need.
That % looks efficient but gcc will try and remove the div operation
which is something the hash implementation (as of hash_min()) avoids. So
the only additional costs here is the additional hashtable which worth
the price given that you don't assume what tag can be.
Sebastian
On 2022-07-18 18:28:34 [-0700], Thinh Nguyen wrote:
> diff --git a/drivers/usb/gadget/function/f_tcm.c b/drivers/usb/gadget/function/f_tcm.c
> index 8b99ee07df87..dfa3e7c043a3 100644
> --- a/drivers/usb/gadget/function/f_tcm.c
> +++ b/drivers/usb/gadget/function/f_tcm.c
> @@ -1214,6 +1233,51 @@ static void usbg_cmd_work(struct work_struct *work)
> return;
>
> skip:
> + if (cmd->tmr_rsp == RC_OVERLAPPED_TAG) {
> + struct se_session *se_sess;
> + struct uas_stream *stream;
> +
> + se_sess = cmd->fu->tpg->tpg_nexus->tvn_se_sess;
> + stream = uasp_get_stream_by_tag(cmd->fu, cmd->tag);
> +
> + /*
> + * There's no guarantee of a matching completion order between
> + * different endpoints. i.e. The device may receive a new (CDB)
> + * command request completion of the command endpoint before it
> + * gets notified of the previous command status completion from
> + * a status endpoint. The driver still needs to detect
> + * misbehaving host and respond with an overlap command tag. To
> + * prevent false overlapped tag failure, give the active and
> + * matching stream id a short time (1ms) to complete before
> + * respond with overlapped command failure.
> + */
> + msleep(1);
How likely is it for this to happen? Is there maybe some synchronisation
missing? If I see this right, in order to get here, you will already
spill the message "Command tag %d overlapped" which does not look good.
Why should the host re-use a tag which did not yet complete?
Sebastian
On 2022-07-18 18:26:01 [-0700], Thinh Nguyen wrote:
> The Linux UASP gadget driver is incomplete and remained broken for a long time.
> It was not implemented for performance either. This series adds some of the
> required features for the UASP driver to work. It also makes some changes to
> the target core.
Some patches here have fixes: tags and are in the middle of the series.
If they are indeed fixes which are needed for the driver function
regardless of the other changes, which are part of the series, then they
should be moved to the front of series _or_ submitted independently as
in "lets first fix the broken things and then make it pretty".
All in all I am happy to see that somebody is looking into the target
USB gadget.
Sebastian
On Fri, Aug 26, 2022, Sebastian Andrzej Siewior wrote:
> On 2022-07-18 18:27:12 [-0700], Thinh Nguyen wrote:
> > index 6fea80afe2d7..ec83f2f9a858 100644
> > --- a/drivers/usb/gadget/function/f_tcm.c
> > +++ b/drivers/usb/gadget/function/f_tcm.c
> > @@ -955,7 +949,7 @@ static void usbg_data_write_cmpl(struct usb_ep *ep, struct usb_request *req)
> > se_cmd->data_length);
> > }
> >
> > - complete(&cmd->write_complete);
> > + target_execute_cmd(se_cmd);
>
> usbg_data_write_cmpl() is invoked from interrupt service routing which
> may run with disabled interrupts. From looking at target_execute_cmd():
It will always be called with interrupts disabled as documented in
usb_request API.
> | void target_execute_cmd(struct se_cmd *cmd)
> | {
> …
> | spin_lock_irq(&cmd->t_state_lock);
> …
> | spin_unlock_irq(&cmd->t_state_lock);
> …
> | }
>
> which means interrupts will remain open after leaving
> target_execute_cmd(). Now, why didn't the WARN_ONCE() in
> __handle_irq_event_percpu() trigger? Am I missing something?
>
> > return;
>
Since target_execute_cmd() is called in usbg_data_write_cmpl(),
interrupts are still disabled.
Thanks,
Thinh
On Fri, Aug 26, 2022, Sebastian Andrzej Siewior wrote:
> On 2022-07-18 18:28:16 [-0700], Thinh Nguyen wrote:
> > diff --git a/drivers/usb/gadget/function/f_tcm.c b/drivers/usb/gadget/function/f_tcm.c
> > index 084143213176..a10e74290664 100644
> > --- a/drivers/usb/gadget/function/f_tcm.c
> > +++ b/drivers/usb/gadget/function/f_tcm.c
> > @@ -506,6 +506,22 @@ static void uasp_cleanup_old_alt(struct f_uas *fu)
> > uasp_free_cmdreq(fu);
> > }
> >
> > +static struct uas_stream *uasp_get_stream_by_tag(struct f_uas *fu, u16 tag)
> > +{
> > + /*
> > + * For simplicity, we use mod operation to quickly find an in-progress
> > + * matching command tag to check for overlapped command. The assumption
> > + * is that the UASP class driver will limit to using tag id from 1 to
> > + * USBG_NUM_CMDS. This is based on observation from the Windows and
> > + * Linux UASP storage class driver behavior. If an unusual UASP class
> > + * driver uses a tag greater than USBG_NUM_CMDS, then this method may no
> > + * longer work due to possible stream id collision. In that case, we
> > + * need to use a proper algorithm to fetch the stream (or simply walk
> > + * through all active streams to check for overlap).
> > + */
> > + return &fu->stream[tag % USBG_NUM_CMDS];
>
> Could you please avoid the assumption what tag actually is?
> Please take a look at hashtable.h, hash_add(), hash_del(),
> hash_for_each_possible_safe() is probably all you need.
> That % looks efficient but gcc will try and remove the div operation
> which is something the hash implementation (as of hash_min()) avoids. So
> the only additional costs here is the additional hashtable which worth
> the price given that you don't assume what tag can be.
>
Sure, I can look into it.
Thanks,
Thinh
On Fri, Aug 26, 2022, Sebastian Andrzej Siewior wrote:
> On 2022-07-18 18:28:34 [-0700], Thinh Nguyen wrote:
> > diff --git a/drivers/usb/gadget/function/f_tcm.c b/drivers/usb/gadget/function/f_tcm.c
> > index 8b99ee07df87..dfa3e7c043a3 100644
> > --- a/drivers/usb/gadget/function/f_tcm.c
> > +++ b/drivers/usb/gadget/function/f_tcm.c
> > @@ -1214,6 +1233,51 @@ static void usbg_cmd_work(struct work_struct *work)
> > return;
> >
> > skip:
> > + if (cmd->tmr_rsp == RC_OVERLAPPED_TAG) {
> > + struct se_session *se_sess;
> > + struct uas_stream *stream;
> > +
> > + se_sess = cmd->fu->tpg->tpg_nexus->tvn_se_sess;
> > + stream = uasp_get_stream_by_tag(cmd->fu, cmd->tag);
> > +
> > + /*
> > + * There's no guarantee of a matching completion order between
> > + * different endpoints. i.e. The device may receive a new (CDB)
> > + * command request completion of the command endpoint before it
> > + * gets notified of the previous command status completion from
> > + * a status endpoint. The driver still needs to detect
> > + * misbehaving host and respond with an overlap command tag. To
> > + * prevent false overlapped tag failure, give the active and
> > + * matching stream id a short time (1ms) to complete before
> > + * respond with overlapped command failure.
> > + */
> > + msleep(1);
>
> How likely is it for this to happen? Is there maybe some synchronisation
> missing? If I see this right, in order to get here, you will already
> spill the message "Command tag %d overlapped" which does not look good.
> Why should the host re-use a tag which did not yet complete?
>
Not often, but it can happen. For example, even though the device sent a
status on the wire and the host received it. The host may send a new
command with the same tag before the device is notified of the previous
command completion. Since they operate in different endpoints, the
device driver may get notification of the new command from the command
endpoint before the status completion of the status endpoint.
This is an attempt to maintain synchronization and prevent false overlap
check. It's a quick fix. I feel that we can handle this better. If you
can think of a better way to handle this, let me know.
BR,
Thinh
On Fri, Aug 26, 2022, Sebastian Andrzej Siewior wrote:
> On 2022-07-18 18:26:01 [-0700], Thinh Nguyen wrote:
> > The Linux UASP gadget driver is incomplete and remained broken for a long time.
> > It was not implemented for performance either. This series adds some of the
> > required features for the UASP driver to work. It also makes some changes to
> > the target core.
>
> Some patches here have fixes: tags and are in the middle of the series.
> If they are indeed fixes which are needed for the driver function
> regardless of the other changes, which are part of the series, then they
> should be moved to the front of series _or_ submitted independently as
> in "lets first fix the broken things and then make it pretty".
>
> All in all I am happy to see that somebody is looking into the target
> USB gadget.
>
Thanks for the reviews!
I can move the commits with fixes tag around. But for the driver to work
properly against different OSes (and maybe even for Linux), most of the
changes without the fixes tags are also needed and not just to make it
"pretty".
There are still more work for f_tcm, but this series (plus the fixes
series in target) is enough for the driver to work properly.
Thanks,
Thinh
On 2022-08-26 18:37:36 [+0000], Thinh Nguyen wrote:
> On Fri, Aug 26, 2022, Sebastian Andrzej Siewior wrote:
> > On 2022-07-18 18:27:12 [-0700], Thinh Nguyen wrote:
> > > index 6fea80afe2d7..ec83f2f9a858 100644
> > > --- a/drivers/usb/gadget/function/f_tcm.c
> > > +++ b/drivers/usb/gadget/function/f_tcm.c
> > > @@ -955,7 +949,7 @@ static void usbg_data_write_cmpl(struct usb_ep *ep, struct usb_request *req)
> > > se_cmd->data_length);
> > > }
> > >
> > > - complete(&cmd->write_complete);
> > > + target_execute_cmd(se_cmd);
> >
> > usbg_data_write_cmpl() is invoked from interrupt service routing which
> > may run with disabled interrupts. From looking at target_execute_cmd():
>
> It will always be called with interrupts disabled as documented in
> usb_request API.
>
> > | void target_execute_cmd(struct se_cmd *cmd)
> > | {
> > …
> > | spin_lock_irq(&cmd->t_state_lock);
> > …
> > | spin_unlock_irq(&cmd->t_state_lock);
> > …
> > | }
> >
> > which means interrupts will remain open after leaving
> > target_execute_cmd(). Now, why didn't the WARN_ONCE() in
> > __handle_irq_event_percpu() trigger? Am I missing something?
> >
> > > return;
> >
>
> Since target_execute_cmd() is called in usbg_data_write_cmpl(),
> interrupts are still disabled.
but you do realize that target_execute_cmd() will leave with enabled
interrupts and this is not desired? I _think_ this was the reason why I
ended up with the wait+complete construct instead of invoking this
function directly.
An _irqsave() in target_execute_cmd() would probably be all you need
here.
> Thanks,
> Thinh
Sebastian
On Mon, Aug 29, 2022, Sebastian Andrzej Siewior wrote:
> On 2022-08-26 18:37:36 [+0000], Thinh Nguyen wrote:
> > On Fri, Aug 26, 2022, Sebastian Andrzej Siewior wrote:
> > > On 2022-07-18 18:27:12 [-0700], Thinh Nguyen wrote:
> > > > index 6fea80afe2d7..ec83f2f9a858 100644
> > > > --- a/drivers/usb/gadget/function/f_tcm.c
> > > > +++ b/drivers/usb/gadget/function/f_tcm.c
> > > > @@ -955,7 +949,7 @@ static void usbg_data_write_cmpl(struct usb_ep *ep, struct usb_request *req)
> > > > se_cmd->data_length);
> > > > }
> > > >
> > > > - complete(&cmd->write_complete);
> > > > + target_execute_cmd(se_cmd);
> > >
> > > usbg_data_write_cmpl() is invoked from interrupt service routing which
> > > may run with disabled interrupts. From looking at target_execute_cmd():
> >
> > It will always be called with interrupts disabled as documented in
> > usb_request API.
> >
> > > | void target_execute_cmd(struct se_cmd *cmd)
> > > | {
> > > …
> > > | spin_lock_irq(&cmd->t_state_lock);
> > > …
> > > | spin_unlock_irq(&cmd->t_state_lock);
> > > …
> > > | }
> > >
> > > which means interrupts will remain open after leaving
> > > target_execute_cmd(). Now, why didn't the WARN_ONCE() in
> > > __handle_irq_event_percpu() trigger? Am I missing something?
> > >
> > > > return;
> > >
> >
> > Since target_execute_cmd() is called in usbg_data_write_cmpl(),
> > interrupts are still disabled.
>
> but you do realize that target_execute_cmd() will leave with enabled
> interrupts and this is not desired? I _think_ this was the reason why I
> ended up with the wait+complete construct instead of invoking this
> function directly.
> An _irqsave() in target_execute_cmd() would probably be all you need
> here.
>
Ok. Maybe we should make a change in the target_execute_cmd() then. It
seems unreasonable to force the caller to workaround this such as the
wait+complete construct you did (and I don't recall we have changes in
place to know/guarantee that interrupts are enabled before executing
target_execute_cmd() previously either).
For the dwc3, we masked the interrupt at this point, so interrupt won't
be asserted here.
Thanks,
Thinh
On 2022-08-29 21:47:41 [+0000], Thinh Nguyen wrote:
> Ok. Maybe we should make a change in the target_execute_cmd() then. It
> seems unreasonable to force the caller to workaround this such as the
> wait+complete construct you did (and I don't recall we have changes in
> place to know/guarantee that interrupts are enabled before executing
> target_execute_cmd() previously either).
Sounds reasonable. Back then I wasn't sure if I'm putting all the puzzle
pieces correctly together so I preferred this over a target change I
wasn't sure was really needed. Anyway.
> For the dwc3, we masked the interrupt at this point, so interrupt won't
> be asserted here.
dwc3 has a irqrestore() after calling the routine so that will avoid the
splat. But lockdep should yell here.
Anyway, other interrupts on that CPU (timer for instance) could trigger.
> Thanks,
> Thinh
Sebastian