| Message ID | cover.1657149962.git.Thinh.Nguyen@synopsys.com |
|---|---|
| Headers | show |
| Series | usb: gadget: f_tcm: Enhance UASP driver | expand |
You probably want to split this up a bit to make review easier, a natural first series would be target core improvements that can be used as-is. Also please don't just Cc people on individual patches, which makes reviewinging impossible.
On 7/6/2022, Christoph Hellwig wrote: > You probably want to split this up a bit to make review easier, a > natural first series would be target core improvements that can be > used as-is. Also please don't just Cc people on individual patches, > which makes reviewinging impossible. If you haven't noticed already, there are dependencies that the f_tcm needs in the target core to function properly. To fully test the f_tcm, we need everything here. As for the list of people Cc'ed, most are pulled using the get_maintainer.pl. The target related patches also included the USB folks for context. Likewise, the USB patches included the target/scsi list. Please take a look and see how we can split this up while it can still make sense to be able to test it. Thanks, Thinh
On Wed, Jul 06, 2022 at 04:34:20PM -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 fixes to the > target core. So I can't take the USB changes without a change to the target code? Some of these seem like I can, so I do not understand the dependancy here. Can you split this into at least 2 series? One for just target, one for just USB, and maybe one for the remaining bits that require both? thanks, greg k-h
On 07.07.22 01:34, Thinh Nguyen wrote: > Microsoft Windows checks for MI_REPORT_SUPPORTED_OPERATION_CODES. Let's > handle this MAINTENANCE_IN command and report supported commands. > +sense_reason_t > +target_emulate_report_supported_opcodes(struct se_cmd *cmd) > +{ > + unsigned char *cdb = cmd->t_task_cdb; > + unsigned char *buf; > + u8 supported = 0; > + [..] > + case ATA_12: > + case ATA_16: > + case VERIFY: > + case ZBC_IN: > + case ZBC_OUT: > + default: > + break; Why the NOP here? If you want to document something, a comment would be nice. Regards Oliver
On 07.07.22 01:35, Thinh Nguyen wrote: > CHECK CONDITION returns sense data, and sense data is minimum 8 bytes > long plus additional sense data length in the data buffer. Don't just > set the allocated buffer length to the cmd->scsi_sense_length and check > the sense data for that. > > See SPC4-r37 section 4.5.2.1. > > Signed-off-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com> > --- > drivers/target/target_core_transport.c | 10 +++++++++- > 1 file changed, 9 insertions(+), 1 deletion(-) > > diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c > index bc1e4a7c4538..9734952a6228 100644 > --- a/drivers/target/target_core_transport.c > +++ b/drivers/target/target_core_transport.c > @@ -3459,12 +3459,20 @@ 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; > + > + /* > + * CHECK CONDITION returns sense data, and sense data is minimum 8 > + * bytes long. See SPC4-r37 section 4.5.2.1. > + */ > + cmd->scsi_sense_length = 8; > + > 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, > cmd->sense_info) < 0); > + /* Additional sense data length */ > + cmd->scsi_sense_length += buffer[7]; Doesn't this need to check for overflows? Regards Oliver
On Wed, Jul 06, 2022 at 04:35:57PM -0700, Thinh Nguyen wrote: > «Внимание! Данное письмо от внешнего адресата!» > > The INQUIRY data length is minimum 36 bytes plus additional length > jindicated in the descriptor. See SPC4-r37 section 6.6.2. The "len" here > is the total length of the INQUIRY data. Make sure to include the 36 > initial bytes. I think you're wrong, because Standard INQUIRY data format clearly defines ADDITIONAL LENGTH as (n - 4), where n is the total length of the INQUIRY data including the the mandatory bytes. > Signed-off-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com> > --- > drivers/target/target_core_spc.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/target/target_core_spc.c b/drivers/target/target_core_spc.c > index dd799158609d..1801e10cd575 100644 > --- a/drivers/target/target_core_spc.c > +++ b/drivers/target/target_core_spc.c > @@ -756,7 +756,7 @@ spc_emulate_inquiry(struct se_cmd *cmd) > } > > ret = spc_emulate_inquiry_std(cmd, buf); > - len = buf[4] + 5; > + len = buf[4] + 5 + INQUIRY_LEN; > goto out; > } > > -- > 2.28.0 >
On 7/6/2022, Greg Kroah-Hartman wrote: > On Wed, Jul 06, 2022 at 04:34:20PM -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 fixes to the >> target core. > So I can't take the USB changes without a change to the target code? > Some of these seem like I can, so I do not understand the dependancy > here. Without the entire series, UASP Compliant Verification test will fail. The dependency is more for the CV test. > Can you split this into at least 2 series? One for just target, one for > just USB, and maybe one for the remaining bits that require both? > Ok, I can split them base on compilation dependency. Thanks, Thinh
On 7/7/2022, Oliver Neukum wrote: > > On 07.07.22 01:35, Thinh Nguyen wrote: >> CHECK CONDITION returns sense data, and sense data is minimum 8 bytes >> long plus additional sense data length in the data buffer. Don't just >> set the allocated buffer length to the cmd->scsi_sense_length and check >> the sense data for that. >> >> See SPC4-r37 section 4.5.2.1. >> >> Signed-off-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com> >> --- >> drivers/target/target_core_transport.c | 10 +++++++++- >> 1 file changed, 9 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c >> index bc1e4a7c4538..9734952a6228 100644 >> --- a/drivers/target/target_core_transport.c >> +++ b/drivers/target/target_core_transport.c >> @@ -3459,12 +3459,20 @@ 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; >> + >> + /* >> + * CHECK CONDITION returns sense data, and sense data is minimum 8 >> + * bytes long. See SPC4-r37 section 4.5.2.1. >> + */ >> + cmd->scsi_sense_length = 8; >> + >> 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, >> cmd->sense_info) < 0); >> + /* Additional sense data length */ >> + cmd->scsi_sense_length += buffer[7]; > Doesn't this need to check for overflows? I missed that. Will fix. Thanks, Thinh
On Thu, Jul 07, 2022 at 10:15:53AM +0000, Thinh Nguyen wrote: > On 7/6/2022, Greg Kroah-Hartman wrote: > > On Wed, Jul 06, 2022 at 04:34:20PM -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 fixes to the > >> target core. > > So I can't take the USB changes without a change to the target code? > > Some of these seem like I can, so I do not understand the dependancy > > here. > > Without the entire series, UASP Compliant Verification test will fail. It fails today, right? So it's not an issue. > The dependency is more for the CV test. That's independant of getting patches merged, correct? > > Can you split this into at least 2 series? One for just target, one for > > just USB, and maybe one for the remaining bits that require both? > > > > Ok, I can split them base on compilation dependency. You also have to realize there are maintainer and subsystem dependencies that you are crossing. thanks, greg k-h
On Wed, Jul 06, 2022 at 04:34:42PM -0700, Thinh Nguyen wrote: > If the preempt_and_abort_list is NULL, don't attempt to drain it. > Otherwise, it may lead to invalid access. > > Signed-off-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com> > --- > drivers/target/target_core_tmr.c | 8 +++++--- > 1 file changed, 5 insertions(+), 3 deletions(-) > > diff --git a/drivers/target/target_core_tmr.c b/drivers/target/target_core_tmr.c > index bac111456fa1..7a7e24069ba7 100644 > --- a/drivers/target/target_core_tmr.c > +++ b/drivers/target/target_core_tmr.c > @@ -383,9 +383,11 @@ int core_tmr_lun_reset( > (preempt_and_abort_list) ? "Preempt" : "TMR", > dev->transport->name, tas); > > - core_tmr_drain_tmr_list(dev, tmr, preempt_and_abort_list); > - core_tmr_drain_state_list(dev, prout_cmd, tmr_sess, tas, > - preempt_and_abort_list); > + if (preempt_and_abort_list) { > + core_tmr_drain_tmr_list(dev, tmr, preempt_and_abort_list); > + core_tmr_drain_state_list(dev, prout_cmd, tmr_sess, tas, > + preempt_and_abort_list); > + } Those functions are not about to drain preempt_and_abort_list. And there are no invalid access inside. This patch breaks the tmf_abort functionality. NACK > > /* > * Clear any legacy SPC-2 reservation when called during
Hi Thinh, On Wed, Jul 06, 2022 at 04:34:55PM -0700, Thinh Nguyen wrote: > A command completion is asynchronous, regardless if an abort command is > executed. Don't just free the command before its completion. Similarly, > a TMR command is not completed until its response is completed. The > freeing of the command can be done by the target user through > target_generic_free_cmd(). > > Signed-off-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com> > --- > drivers/target/target_core_transport.c | 7 ------- > 1 file changed, 7 deletions(-) > > diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c > index 7838dc20f713..105d3b0e470f 100644 > --- a/drivers/target/target_core_transport.c > +++ b/drivers/target/target_core_transport.c > @@ -836,10 +836,6 @@ static void target_handle_abort(struct se_cmd *cmd) > } > > WARN_ON_ONCE(kref_read(&cmd->cmd_kref) == 0); > - > - transport_lun_remove_cmd(cmd); > - > - transport_cmd_check_stop_to_fabric(cmd); > } > > static void target_abort_work(struct work_struct *work) > @@ -3553,9 +3549,6 @@ static void target_tmr_work(struct work_struct *work) > goto aborted; > > cmd->se_tfo->queue_tm_rsp(cmd); > - > - transport_lun_remove_cmd(cmd); > - transport_cmd_check_stop_to_fabric(cmd); > return; > > aborted: Those functions are not about to free the command. transport_lun_remove_cmd is for remove command from the state/tmr list. transport_cmd_check_stop_to_fabric is for notify a fabric driver to decrease the command kref that it owns. And eventually to wake target_put_cmd_and_wait() in core_tmr_abort_task(). Those functions do always are called after a final response has been sent (STATUS, CHECK_CONDITION,etc). Those functions do not break the abort functionality. But this patch does.
Hi Thinh, On Wed, Jul 06, 2022 at 04:35:07PM -0700, Thinh Nguyen wrote: > If there's no command to abort, just skip doing tmr_notify to an empty > list. AFAIK, that was intentionaly: https://lore.kernel.org/all/20200726153510.13077-3-bstroesser@ts.fujitsu.com/ 'If no commands were aborted, an empty list is supplied.' > Signed-off-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com> > --- > drivers/target/target_core_tmr.c | 3 --- > 1 file changed, 3 deletions(-) > > diff --git a/drivers/target/target_core_tmr.c b/drivers/target/target_core_tmr.c > index 724ddabda488..718d985e4860 100644 > --- a/drivers/target/target_core_tmr.c > +++ b/drivers/target/target_core_tmr.c > @@ -167,9 +167,6 @@ void core_tmr_abort_task( > spin_unlock_irqrestore(&dev->queues[i].lock, flags); > } > > - if (dev->transport->tmr_notify) > - dev->transport->tmr_notify(dev, TMR_ABORT_TASK, &aborted_list); > - > printk("ABORT_TASK: Sending TMR_FUNCTION_COMPLETE for ref_tag: %lld\n", > tmr->ref_task_tag); > tmr->response = TMR_FUNCTION_COMPLETE;
Hi Thinh, On Wed, Jul 06, 2022 at 04:35:20PM -0700, Thinh Nguyen wrote: > Add some standard TMR and match their code id based on UAS-r04 and > SPL4-r13. Note that the non-standard TMR_LUN_RESET_PRO is using the same > id value of QUERY TASK. Change it to 0xf0 instead. > > Signed-off-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com> > --- > drivers/target/target_core_transport.c | 10 ++++++++++ > include/target/target_core_base.h | 8 ++++++-- > 2 files changed, 16 insertions(+), 2 deletions(-) > > diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c > index 105d3b0e470f..cbd876e44cf0 100644 > --- a/drivers/target/target_core_transport.c > +++ b/drivers/target/target_core_transport.c > @@ -3090,6 +3090,10 @@ static const char *target_tmf_name(enum tcm_tmreq_table tmf) > case TMR_TARGET_WARM_RESET: return "TARGET_WARM_RESET"; > case TMR_TARGET_COLD_RESET: return "TARGET_COLD_RESET"; > case TMR_LUN_RESET_PRO: return "LUN_RESET_PRO"; > + case TMR_I_T_NEXUS_RESET: return "I_T_NEXUS_RESET"; > + case TMR_QUERY_TASK: return "QUERY_TASK"; > + case TMR_QUERY_TASK_SET: return "QUERY_TASK_SET"; > + case TMR_QUERY_ASYNC_EVENT: return "QUERY_ASYNC_EVENT"; > case TMR_UNKNOWN: break; > } > return "(?)"; > @@ -3538,6 +3542,12 @@ static void target_tmr_work(struct work_struct *work) > case TMR_TARGET_COLD_RESET: > tmr->response = TMR_FUNCTION_REJECTED; > break; > + case TMR_I_T_NEXUS_RESET: > + case TMR_QUERY_TASK: > + case TMR_QUERY_TASK_SET: > + case TMR_QUERY_ASYNC_EVENT: > + tmr->response = TMR_FUNCTION_REJECTED; > + break; > default: > pr_err("Unknown TMR function: 0x%02x.\n", > tmr->function); > diff --git a/include/target/target_core_base.h b/include/target/target_core_base.h > index 8e3da143a1ce..ccd98604eaf4 100644 > --- a/include/target/target_core_base.h > +++ b/include/target/target_core_base.h > @@ -206,12 +206,16 @@ enum target_sc_flags_table { > enum tcm_tmreq_table { > TMR_ABORT_TASK = 1, > TMR_ABORT_TASK_SET = 2, > - TMR_CLEAR_ACA = 3, > + TMR_CLEAR_ACA = 0x40, There is no need to align that values to some standart. This enum is not standard. That is even stated in the comment for it: /* fabric independent task management function values */ So, just add new values continuing from 8. > TMR_CLEAR_TASK_SET = 4, > TMR_LUN_RESET = 5, > TMR_TARGET_WARM_RESET = 6, > TMR_TARGET_COLD_RESET = 7, > - TMR_LUN_RESET_PRO = 0x80, > + TMR_I_T_NEXUS_RESET = 0x10, > + TMR_QUERY_TASK = 0x80, > + TMR_QUERY_TASK_SET = 0x81, > + TMR_QUERY_ASYNC_EVENT = 0x82, > + TMR_LUN_RESET_PRO = 0xf0, > TMR_UNKNOWN = 0xff, > }; >
Hi Thinh, On Wed, Jul 06, 2022 at 04:35:32PM -0700, Thinh Nguyen wrote: > CHECK CONDITION returns sense data, and sense data is minimum 8 bytes > long plus additional sense data length in the data buffer. Don't just > set the allocated buffer length to the cmd->scsi_sense_length and check > the sense data for that. > > See SPC4-r37 section 4.5.2.1. > > Signed-off-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com> > --- > drivers/target/target_core_transport.c | 10 +++++++++- > 1 file changed, 9 insertions(+), 1 deletion(-) > > diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c > index bc1e4a7c4538..9734952a6228 100644 > --- a/drivers/target/target_core_transport.c > +++ b/drivers/target/target_core_transport.c > @@ -3459,12 +3459,20 @@ 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; > + > + /* > + * CHECK CONDITION returns sense data, and sense data is minimum 8 > + * bytes long. See SPC4-r37 section 4.5.2.1. > + */ > + cmd->scsi_sense_length = 8; > + > 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, scsi_set_sense_information()'s second argument is buffer length; send there TRANSPORT_SENSE_BUFFER and the patch will be correct. > cmd->sense_info) < 0); > + /* Additional sense data length */ > + cmd->scsi_sense_length += buffer[7]; > } > > int
On Wed, Jul 06, 2022 at 04:38:01PM -0700, Thinh Nguyen wrote: > Handle target_core_fabric_ops TASK MANAGEMENT functions and their > response. If a TASK MANAGEMENT command is received, the driver will > interpret the function TMF_*, translate to TMR_*, and fire off a command > work executing target_submit_tmr(). On completion, it will handle the > TASK MANAGEMENT response through uasp_send_tm_response(). > > Signed-off-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com> > --- > NOTE: I appologize for this big patch. I feel that this feature needs to be > viewed in its entirety to see the whole picture and easier review. > > > drivers/usb/gadget/function/f_tcm.c | 260 +++++++++++++++++++++++++--- > drivers/usb/gadget/function/tcm.h | 7 +- > 2 files changed, 241 insertions(+), 26 deletions(-) > > diff --git a/drivers/usb/gadget/function/f_tcm.c b/drivers/usb/gadget/function/f_tcm.c > index fa09999adda7..a68436f97f91 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,53 @@ 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: > + return RC_TMF_COMPLETE; > + case TMR_FUNCTION_REJECTED: > + return RC_TMF_NOT_SUPPORTED; > + case TMR_LUN_DOES_NOT_EXIST: > + return RC_INCORRECT_LUN; > + case TMR_OVERLAPPED_TAG_ATTEMPTED: > + return RC_OVERLAPPED_TAG; > + case TMR_TASK_DOES_NOT_EXIST: > + return RC_INVALID_INFO_UNIT; > + 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 */ > @@ -506,6 +554,11 @@ 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) > +{ > + 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 +566,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 +599,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; > @@ -565,11 +618,36 @@ 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 != TMR_RESPONSE_UNKNOWN) > + rsp_iu->response_code = > + tcm_to_uasp_response(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 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); > struct se_session *se_sess = cmd->se_cmd.se_sess; > int ret; > > @@ -604,6 +682,7 @@ static void uasp_status_data_cmpl(struct usb_ep *ep, struct usb_request *req) > break; > > case UASP_QUEUE_COMMAND: > + stream->cmd = NULL; > > target_free_tag(se_sess, &cmd->se_cmd); > transport_generic_free_cmd(&cmd->se_cmd, 0); > @@ -617,6 +696,7 @@ static void uasp_status_data_cmpl(struct usb_ep *ep, struct usb_request *req) > return; > > cleanup: > + stream->cmd = NULL; > target_free_tag(se_sess, &cmd->se_cmd); > transport_generic_free_cmd(&cmd->se_cmd, 0); > } > @@ -624,7 +704,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); > @@ -633,10 +713,22 @@ 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; > - 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; > > @@ -682,7 +774,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; > > @@ -943,8 +1035,10 @@ 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; > + struct uas_stream *stream = uasp_get_stream_by_tag(cmd->fu, cmd->tag); > > if (req->status == -ESHUTDOWN) { > + stream->cmd = NULL; > target_free_tag(se_cmd->se_sess, se_cmd); > transport_generic_free_cmd(&cmd->se_cmd, 0); > return; > @@ -962,6 +1056,7 @@ static void usbg_data_write_cmpl(struct usb_ep *ep, struct usb_request *req) > se_cmd->data_length); > } > > + cmd->state = UASP_QUEUE_COMMAND; > target_execute_cmd(se_cmd); > return; > > @@ -1042,9 +1137,66 @@ 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_cmd *se_cmd; > + struct tcm_usbg_nexus *tv_nexus; > + struct uas_stream *stream; > + int flags = TARGET_SCF_ACK_KREF; > + > + se_cmd = &cmd->se_cmd; > + tv_nexus = cmd->fu->tpg->tpg_nexus; > + stream = uasp_get_stream_by_tag(cmd->fu, cmd->tag); > + > + /* Failure detected by f_tcm */ > + if (cmd->tmr_rsp != TMR_RESPONSE_UNKNOWN) { > + if (cmd->tmr_rsp == TMR_OVERLAPPED_TAG_ATTEMPTED) { > + /* > + * 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; > + } > + > + /* Overlap command tag detected. Abort command. */ > + cmd->state = UASP_QUEUE_COMMAND; > + stream->cmd->se_cmd.transport_state |= CMD_T_ABORTED; > + target_get_sess_cmd(&stream->cmd->se_cmd, true); > + > + /* This will trigger command abort handler */ > + target_execute_cmd(&stream->cmd->se_cmd); > + transport_generic_free_cmd(&stream->cmd->se_cmd, 1); > + } > + > + > + target_submit_tmr_fail_response(se_cmd, cmd->tmr_rsp, > + tv_nexus->tvn_se_sess, cmd->unpacked_lun, > + GFP_ATOMIC, cmd->tag, flags); I think there is no reason to reject TMR via Core, you may use your uasp_send_tm_response(cmd) directly like other fabric drivers does. That will need some coding to distinguish a completion of the response initiated from Core and from fabric driver. > + return; > + } > + > + target_submit_tmr(se_cmd, tv_nexus->tvn_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 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; > @@ -1073,6 +1225,16 @@ 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); > + > + if (cmd->tmr_func || cmd->tmr_rsp != TMR_RESPONSE_UNKNOWN) > + usbg_submit_tmr(cmd); That looks very strange - in response of received SCSI command you will send a TMR response??? I am about cmd->tmr_rsp != TMR_RESPONSE_UNKNOWN case. > + else > + usbg_submit_cmd(cmd); > +} > + > static struct usbg_cmd *usbg_get_cmd(struct f_uas *fu, > struct tcm_usbg_nexus *tv_nexus, u32 scsi_tag) > { > @@ -1099,37 +1261,84 @@ 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 = TMR_RESPONSE_UNKNOWN; TMR_* constant are fabric agnostic enum. Better use RC_TMF_* values in variables of this driver. > + > + 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 = TMR_TASK_DOES_NOT_EXIST; > + goto skip; > + } > + > + /* > + * For simplicity, we use mod operation to quickly find an in-progress > + * matching command tag and respond with 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). > + */ > + stream = uasp_get_stream_by_tag(fu, scsi_tag); > + if (stream->cmd) { > + WARN_ONCE(stream->cmd->tag != scsi_tag, WARN is used to indicate a non fatal bug in the code. May be you want to use pr_warn/pr_err here? > + "Command tag %d collided with Stream id %d\n", > + scsi_tag, stream->cmd->tag); > + > + cmd->tmr_rsp = TMR_OVERLAPPED_TAG_ATTEMPTED; > + 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: > @@ -1150,9 +1359,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); > > @@ -1298,13 +1505,16 @@ 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) > { > 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) > diff --git a/drivers/usb/gadget/function/tcm.h b/drivers/usb/gadget/function/tcm.h > index 5157af1b166b..f1cd2399fd69 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 TMR_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 {
On 7/7/2022, Dmitry Bogdanov wrote: > Hi Thinh, > > On Wed, Jul 06, 2022 at 04:34:55PM -0700, Thinh Nguyen wrote: >> A command completion is asynchronous, regardless if an abort command is >> executed. Don't just free the command before its completion. Similarly, >> a TMR command is not completed until its response is completed. The >> freeing of the command can be done by the target user through >> target_generic_free_cmd(). >> >> Signed-off-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com> >> --- >> drivers/target/target_core_transport.c | 7 ------- >> 1 file changed, 7 deletions(-) >> >> diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c >> index 7838dc20f713..105d3b0e470f 100644 >> --- a/drivers/target/target_core_transport.c >> +++ b/drivers/target/target_core_transport.c >> @@ -836,10 +836,6 @@ static void target_handle_abort(struct se_cmd *cmd) >> } >> >> WARN_ON_ONCE(kref_read(&cmd->cmd_kref) == 0); >> - >> - transport_lun_remove_cmd(cmd); >> - >> - transport_cmd_check_stop_to_fabric(cmd); >> } >> >> static void target_abort_work(struct work_struct *work) >> @@ -3553,9 +3549,6 @@ static void target_tmr_work(struct work_struct *work) >> goto aborted; >> >> cmd->se_tfo->queue_tm_rsp(cmd); >> - >> - transport_lun_remove_cmd(cmd); >> - transport_cmd_check_stop_to_fabric(cmd); >> return; >> >> aborted: > Those functions are not about to free the command. > transport_lun_remove_cmd is for remove command from the state/tmr list. > transport_cmd_check_stop_to_fabric is for notify a fabric driver to > decrease the command kref that it owns. And eventually to wake > target_put_cmd_and_wait() in core_tmr_abort_task(). > > Those functions do always are called after a final response has been sent > (STATUS, CHECK_CONDITION,etc). > Those functions do not break the abort functionality. But this patch > does. > > Looks like I misunderstood those functions' role. Thanks, Thinh
On 7/7/2022, Dmitry Bogdanov wrote: > Hi Thinh, > > On Wed, Jul 06, 2022 at 04:35:07PM -0700, Thinh Nguyen wrote: >> If there's no command to abort, just skip doing tmr_notify to an empty >> list. > AFAIK, that was intentionaly: > https://urldefense.com/v3/__https://lore.kernel.org/all/20200726153510.13077-3-bstroesser@ts.fujitsu.com/__;!!A4F2R9G_pg!dpvhptHJ68bm5bCAbothekKpcSln7nndX1oqG7PesbrKa3vx4Py68XwKd-SAg9hmYMdmW4AAK9cRe7vOva2wkN5-$ > 'If no commands were aborted, an empty list is supplied.' I see. Thanks for pointing it out. Thanks, Thinh > >> Signed-off-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com> >> --- >> drivers/target/target_core_tmr.c | 3 --- >> 1 file changed, 3 deletions(-) >> >> diff --git a/drivers/target/target_core_tmr.c b/drivers/target/target_core_tmr.c >> index 724ddabda488..718d985e4860 100644 >> --- a/drivers/target/target_core_tmr.c >> +++ b/drivers/target/target_core_tmr.c >> @@ -167,9 +167,6 @@ void core_tmr_abort_task( >> spin_unlock_irqrestore(&dev->queues[i].lock, flags); >> } >> >> - if (dev->transport->tmr_notify) >> - dev->transport->tmr_notify(dev, TMR_ABORT_TASK, &aborted_list); >> - >> printk("ABORT_TASK: Sending TMR_FUNCTION_COMPLETE for ref_tag: %lld\n", >> tmr->ref_task_tag); >> tmr->response = TMR_FUNCTION_COMPLETE;
On 7/7/2022, Dmitry Bogdanov wrote: > Hi Thinh, > > On Wed, Jul 06, 2022 at 04:35:20PM -0700, Thinh Nguyen wrote: >> Add some standard TMR and match their code id based on UAS-r04 and >> SPL4-r13. Note that the non-standard TMR_LUN_RESET_PRO is using the same >> id value of QUERY TASK. Change it to 0xf0 instead. >> >> Signed-off-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com> >> --- >> drivers/target/target_core_transport.c | 10 ++++++++++ >> include/target/target_core_base.h | 8 ++++++-- >> 2 files changed, 16 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c >> index 105d3b0e470f..cbd876e44cf0 100644 >> --- a/drivers/target/target_core_transport.c >> +++ b/drivers/target/target_core_transport.c >> @@ -3090,6 +3090,10 @@ static const char *target_tmf_name(enum tcm_tmreq_table tmf) >> case TMR_TARGET_WARM_RESET: return "TARGET_WARM_RESET"; >> case TMR_TARGET_COLD_RESET: return "TARGET_COLD_RESET"; >> case TMR_LUN_RESET_PRO: return "LUN_RESET_PRO"; >> + case TMR_I_T_NEXUS_RESET: return "I_T_NEXUS_RESET"; >> + case TMR_QUERY_TASK: return "QUERY_TASK"; >> + case TMR_QUERY_TASK_SET: return "QUERY_TASK_SET"; >> + case TMR_QUERY_ASYNC_EVENT: return "QUERY_ASYNC_EVENT"; >> case TMR_UNKNOWN: break; >> } >> return "(?)"; >> @@ -3538,6 +3542,12 @@ static void target_tmr_work(struct work_struct *work) >> case TMR_TARGET_COLD_RESET: >> tmr->response = TMR_FUNCTION_REJECTED; >> break; >> + case TMR_I_T_NEXUS_RESET: >> + case TMR_QUERY_TASK: >> + case TMR_QUERY_TASK_SET: >> + case TMR_QUERY_ASYNC_EVENT: >> + tmr->response = TMR_FUNCTION_REJECTED; >> + break; >> default: >> pr_err("Unknown TMR function: 0x%02x.\n", >> tmr->function); >> diff --git a/include/target/target_core_base.h b/include/target/target_core_base.h >> index 8e3da143a1ce..ccd98604eaf4 100644 >> --- a/include/target/target_core_base.h >> +++ b/include/target/target_core_base.h >> @@ -206,12 +206,16 @@ enum target_sc_flags_table { >> enum tcm_tmreq_table { >> TMR_ABORT_TASK = 1, >> TMR_ABORT_TASK_SET = 2, >> - TMR_CLEAR_ACA = 3, >> + TMR_CLEAR_ACA = 0x40, > There is no need to align that values to some standart. This enum is not > standard. That is even stated in the comment for it: > /* fabric independent task management function values */ > So, just add new values continuing from 8. Sure. I'll do that. Thanks, Thinh >> TMR_CLEAR_TASK_SET = 4, >> TMR_LUN_RESET = 5, >> TMR_TARGET_WARM_RESET = 6, >> TMR_TARGET_COLD_RESET = 7, >> - TMR_LUN_RESET_PRO = 0x80, >> + TMR_I_T_NEXUS_RESET = 0x10, >> + TMR_QUERY_TASK = 0x80, >> + TMR_QUERY_TASK_SET = 0x81, >> + TMR_QUERY_ASYNC_EVENT = 0x82, >> + TMR_LUN_RESET_PRO = 0xf0, >> TMR_UNKNOWN = 0xff, >> }; >>
On 7/7/2022, Dmitry Bogdanov wrote: > Hi Thinh, > > On Wed, Jul 06, 2022 at 04:35:32PM -0700, Thinh Nguyen wrote: >> CHECK CONDITION returns sense data, and sense data is minimum 8 bytes >> long plus additional sense data length in the data buffer. Don't just >> set the allocated buffer length to the cmd->scsi_sense_length and check >> the sense data for that. >> >> See SPC4-r37 section 4.5.2.1. >> >> Signed-off-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com> >> --- >> drivers/target/target_core_transport.c | 10 +++++++++- >> 1 file changed, 9 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c >> index bc1e4a7c4538..9734952a6228 100644 >> --- a/drivers/target/target_core_transport.c >> +++ b/drivers/target/target_core_transport.c >> @@ -3459,12 +3459,20 @@ 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; >> + >> + /* >> + * CHECK CONDITION returns sense data, and sense data is minimum 8 >> + * bytes long. See SPC4-r37 section 4.5.2.1. >> + */ >> + cmd->scsi_sense_length = 8; >> + >> 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, > scsi_set_sense_information()'s second argument is buffer length; send > there TRANSPORT_SENSE_BUFFER and the patch will be correct. Sure, I'll do that. Thanks, Thinh >> cmd->sense_info) < 0); >> + /* Additional sense data length */ >> + cmd->scsi_sense_length += buffer[7]; >> } >> >> int
On 7/8/2022, Dmitry Bogdanov wrote: > On Wed, Jul 06, 2022 at 04:38:01PM -0700, Thinh Nguyen wrote: >> Handle target_core_fabric_ops TASK MANAGEMENT functions and their >> response. If a TASK MANAGEMENT command is received, the driver will >> interpret the function TMF_*, translate to TMR_*, and fire off a command >> work executing target_submit_tmr(). On completion, it will handle the >> TASK MANAGEMENT response through uasp_send_tm_response(). >> >> Signed-off-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com> >> --- >> NOTE: I appologize for this big patch. I feel that this feature needs to be >> viewed in its entirety to see the whole picture and easier review. >> >> >> drivers/usb/gadget/function/f_tcm.c | 260 +++++++++++++++++++++++++--- >> drivers/usb/gadget/function/tcm.h | 7 +- >> 2 files changed, 241 insertions(+), 26 deletions(-) >> >> diff --git a/drivers/usb/gadget/function/f_tcm.c b/drivers/usb/gadget/function/f_tcm.c >> index fa09999adda7..a68436f97f91 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,53 @@ 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: >> + return RC_TMF_COMPLETE; >> + case TMR_FUNCTION_REJECTED: >> + return RC_TMF_NOT_SUPPORTED; >> + case TMR_LUN_DOES_NOT_EXIST: >> + return RC_INCORRECT_LUN; >> + case TMR_OVERLAPPED_TAG_ATTEMPTED: >> + return RC_OVERLAPPED_TAG; >> + case TMR_TASK_DOES_NOT_EXIST: >> + return RC_INVALID_INFO_UNIT; >> + 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 */ >> @@ -506,6 +554,11 @@ 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) >> +{ >> + 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 +566,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 +599,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; >> @@ -565,11 +618,36 @@ 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 != TMR_RESPONSE_UNKNOWN) >> + rsp_iu->response_code = >> + tcm_to_uasp_response(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 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); >> struct se_session *se_sess = cmd->se_cmd.se_sess; >> int ret; >> >> @@ -604,6 +682,7 @@ static void uasp_status_data_cmpl(struct usb_ep *ep, struct usb_request *req) >> break; >> >> case UASP_QUEUE_COMMAND: >> + stream->cmd = NULL; >> >> target_free_tag(se_sess, &cmd->se_cmd); >> transport_generic_free_cmd(&cmd->se_cmd, 0); >> @@ -617,6 +696,7 @@ static void uasp_status_data_cmpl(struct usb_ep *ep, struct usb_request *req) >> return; >> >> cleanup: >> + stream->cmd = NULL; >> target_free_tag(se_sess, &cmd->se_cmd); >> transport_generic_free_cmd(&cmd->se_cmd, 0); >> } >> @@ -624,7 +704,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); >> @@ -633,10 +713,22 @@ 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; >> - 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; >> >> @@ -682,7 +774,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; >> >> @@ -943,8 +1035,10 @@ 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; >> + struct uas_stream *stream = uasp_get_stream_by_tag(cmd->fu, cmd->tag); >> >> if (req->status == -ESHUTDOWN) { >> + stream->cmd = NULL; >> target_free_tag(se_cmd->se_sess, se_cmd); >> transport_generic_free_cmd(&cmd->se_cmd, 0); >> return; >> @@ -962,6 +1056,7 @@ static void usbg_data_write_cmpl(struct usb_ep *ep, struct usb_request *req) >> se_cmd->data_length); >> } >> >> + cmd->state = UASP_QUEUE_COMMAND; >> target_execute_cmd(se_cmd); >> return; >> >> @@ -1042,9 +1137,66 @@ 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_cmd *se_cmd; >> + struct tcm_usbg_nexus *tv_nexus; >> + struct uas_stream *stream; >> + int flags = TARGET_SCF_ACK_KREF; >> + >> + se_cmd = &cmd->se_cmd; >> + tv_nexus = cmd->fu->tpg->tpg_nexus; >> + stream = uasp_get_stream_by_tag(cmd->fu, cmd->tag); >> + >> + /* Failure detected by f_tcm */ >> + if (cmd->tmr_rsp != TMR_RESPONSE_UNKNOWN) { >> + if (cmd->tmr_rsp == TMR_OVERLAPPED_TAG_ATTEMPTED) { >> + /* >> + * 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; >> + } >> + >> + /* Overlap command tag detected. Abort command. */ >> + cmd->state = UASP_QUEUE_COMMAND; >> + stream->cmd->se_cmd.transport_state |= CMD_T_ABORTED; >> + target_get_sess_cmd(&stream->cmd->se_cmd, true); >> + >> + /* This will trigger command abort handler */ >> + target_execute_cmd(&stream->cmd->se_cmd); >> + transport_generic_free_cmd(&stream->cmd->se_cmd, 1); >> + } >> + >> + >> + target_submit_tmr_fail_response(se_cmd, cmd->tmr_rsp, >> + tv_nexus->tvn_se_sess, cmd->unpacked_lun, >> + GFP_ATOMIC, cmd->tag, flags); > I think there is no reason to reject TMR via Core, you may use > your uasp_send_tm_response(cmd) directly like other fabric drivers does. > That will need some coding to distinguish a completion of the response > initiated from Core and from fabric driver. Ok. We can do that. >> + return; >> + } >> + >> + target_submit_tmr(se_cmd, tv_nexus->tvn_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 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; >> @@ -1073,6 +1225,16 @@ 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); >> + >> + if (cmd->tmr_func || cmd->tmr_rsp != TMR_RESPONSE_UNKNOWN) >> + usbg_submit_tmr(cmd); > That looks very strange - in response of received SCSI command you will > send a TMR response??? > I am about cmd->tmr_rsp != TMR_RESPONSE_UNKNOWN case. Sorry if it's unclear here. The condition here is to submit TMR when we have a TASK MANAGEMENT command _or_ we already know that we need to submit a tmr_fail_response. That is, a normal command may need a TMR response such as TMR_TASK_DOES_NOT_EXIST or TMR_OVERLAPPED_TAG_ATTEMPTED. >> + else >> + usbg_submit_cmd(cmd); >> +} >> + >> static struct usbg_cmd *usbg_get_cmd(struct f_uas *fu, >> struct tcm_usbg_nexus *tv_nexus, u32 scsi_tag) >> { >> @@ -1099,37 +1261,84 @@ 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 = TMR_RESPONSE_UNKNOWN; > TMR_* constant are fabric agnostic enum. Better use RC_TMF_* values in > variables of this driver. Sure, we can do that. >> + >> + 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 = TMR_TASK_DOES_NOT_EXIST; >> + goto skip; >> + } >> + >> + /* >> + * For simplicity, we use mod operation to quickly find an in-progress >> + * matching command tag and respond with 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). >> + */ >> + stream = uasp_get_stream_by_tag(fu, scsi_tag); >> + if (stream->cmd) { >> + WARN_ONCE(stream->cmd->tag != scsi_tag, > WARN is used to indicate a non fatal bug in the code. May be you want to > use pr_warn/pr_err here? Ok. Will update. Thanks, Thinh >> + "Command tag %d collided with Stream id %d\n", >> + scsi_tag, stream->cmd->tag); >> + >> + cmd->tmr_rsp = TMR_OVERLAPPED_TAG_ATTEMPTED; >> + 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: >> @@ -1150,9 +1359,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); >> >> @@ -1298,13 +1505,16 @@ 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) >> { >> 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) >> diff --git a/drivers/usb/gadget/function/tcm.h b/drivers/usb/gadget/function/tcm.h >> index 5157af1b166b..f1cd2399fd69 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 TMR_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 {
On 7/7/2022, Konstantin Shelekhin wrote: > On Wed, Jul 06, 2022 at 04:35:57PM -0700, Thinh Nguyen wrote: >> «Внимание! Данное письмо от внешнего адресата!» >> >> The INQUIRY data length is minimum 36 bytes plus additional length >> jindicated in the descriptor. See SPC4-r37 section 6.6.2. The "len" here >> is the total length of the INQUIRY data. Make sure to include the 36 >> initial bytes. > I think you're wrong, because Standard INQUIRY data format clearly > defines ADDITIONAL LENGTH as (n - 4), where n is the total length of the > INQUIRY data including the the mandatory bytes. > Looks like you're right. I mixed it up with Sense data ADDITIONAL LENGTH. Thanks, Thinh >> Signed-off-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com> >> --- >> drivers/target/target_core_spc.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/drivers/target/target_core_spc.c b/drivers/target/target_core_spc.c >> index dd799158609d..1801e10cd575 100644 >> --- a/drivers/target/target_core_spc.c >> +++ b/drivers/target/target_core_spc.c >> @@ -756,7 +756,7 @@ spc_emulate_inquiry(struct se_cmd *cmd) >> } >> >> ret = spc_emulate_inquiry_std(cmd, buf); >> - len = buf[4] + 5; >> + len = buf[4] + 5 + INQUIRY_LEN; >> goto out; >> } >> >> -- >> 2.28.0 >>