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 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
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 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