mbox series

[00/36] usb: gadget: f_tcm: Enhance UASP driver

Message ID cover.1657149962.git.Thinh.Nguyen@synopsys.com
Headers show
Series usb: gadget: f_tcm: Enhance UASP driver | expand

Message

Thinh Nguyen July 6, 2022, 11:34 p.m. UTC 
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.

Please note that the f_tcm is far from a good state. It needs better error
recovery, error reports, more cleanup, and the ability to handle various
required commands.

Also please note that I try to juggle between checkpatch warnings and code
style consistency. As a result, there maybe some minor checkpatch warnings.

Hopefully this can help jumpstart the UASP driver. Please test it out.

This was tested against UASP CV and DWC_usb3x controller.

Thanks!


Thinh Nguyen (36):
  target: Handle MI_REPORT_SUPPORTED_OPERATION_CODES
  target: Add overlapped response to tmrsp_table
  target: Don't drain empty list
  target: Does tmr notify on aborted command
  target: Don't remove command too early
  target: Return Function Complete
  target: Don't do tmr_notify on empty aborted list
  target: Refactor core_tmr_abort_task
  target: Add common Task Management values
  target: Implement TMR_ABORT_TASK_SET
  target: Properly set Sense Data Length of CHECK CONDITION
  target: Properly set Sense data length when copy sense
  target: Don't respond TMR_LUN_DOES_NOT_EXIST for all TMR failure
  target: Introduce target_submit_tmr_fail_response
  target: Include INQUIRY length
  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: Send sense reason
  usb: gadget: f_tcm: Save CPU ID per command
  usb: gadget: f_tcm: Free tags earlier
  usb: gadget: f_tcm: Handle TASK_MANAGEMENT commands
  usb: gadget: f_tcm: Comply with UAS Task Management requirement

 drivers/target/target_core_alua.c      |  66 ++++
 drivers/target/target_core_alua.h      |   2 +
 drivers/target/target_core_spc.c       |  16 +-
 drivers/target/target_core_tmr.c       |  39 +-
 drivers/target/target_core_transport.c |  73 +++-
 drivers/usb/gadget/function/f_tcm.c    | 502 ++++++++++++++++++-------
 drivers/usb/gadget/function/tcm.h      |  20 +-
 include/target/target_core_base.h      |   9 +-
 include/target/target_core_fabric.h    |   3 +
 9 files changed, 562 insertions(+), 168 deletions(-)


base-commit: 90557fa89d3e99286506593fd5180f699c41b152

Comments

Christoph Hellwig July 7, 2022, 4:38 a.m. UTC | #1 
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.
Thinh Nguyen July 7, 2022, 4:50 a.m. UTC | #2 
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
Greg Kroah-Hartman July 7, 2022, 6:59 a.m. UTC | #3 
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
Oliver Neukum July 7, 2022, 8:01 a.m. UTC | #4 
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
Oliver Neukum July 7, 2022, 8:24 a.m. UTC | #5 
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
Thinh Nguyen July 7, 2022, 10:15 a.m. UTC | #6 
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
Thinh Nguyen July 7, 2022, 10:21 a.m. UTC | #7 
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
Greg Kroah-Hartman July 7, 2022, 11:15 a.m. UTC | #8 
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
Dmitry Bogdanov July 7, 2022, 7:36 p.m. UTC | #9 
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,
>  };
>
Dmitry Bogdanov July 7, 2022, 8:27 p.m. UTC | #10 
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
Thinh Nguyen July 9, 2022, 12:04 a.m. UTC | #11 
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,
>>   };
>>
Thinh Nguyen July 9, 2022, 12:11 a.m. UTC | #12 
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