| Message ID | 0150d7b669ad80e94596b371cbce0460a327ce7c.1658192351.git.Thinh.Nguyen@synopsys.com |
|---|---|
| State | New |
| Headers | show |
| Series | usb: gadget: f_tcm: Enhance UASP driver | expand |
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 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
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) {
If there's an overlapped command tag, cancel the command and respond with RC_OVERLAPPED_TAG to host. Signed-off-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com> --- Changes in v2: - New patch. It was part of TASK MANAGEMENT change. - Directly respond to host on overlapped tag instead of reporting to target core. drivers/usb/gadget/function/f_tcm.c | 75 ++++++++++++++++++++++++++++- 1 file changed, 73 insertions(+), 2 deletions(-)