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 KH 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
Konstantin Shelekhin July 7, 2022, 10:11 a.m. UTC | #6
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
>
Thinh Nguyen July 7, 2022, 10:15 a.m. UTC | #7
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 | #8
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 KH July 7, 2022, 11:15 a.m. UTC | #9
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, 12:42 p.m. UTC | #10
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
Dmitry Bogdanov July 7, 2022, 1:16 p.m. UTC | #11
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.
Dmitry Bogdanov July 7, 2022, 1:36 p.m. UTC | #12
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;
Dmitry Bogdanov July 7, 2022, 7:36 p.m. UTC | #13
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 | #14
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
Dmitry Bogdanov July 8, 2022, 9:07 a.m. UTC | #15
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 {
Thinh Nguyen July 8, 2022, 11:40 p.m. UTC | #16
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
Thinh Nguyen July 8, 2022, 11:53 p.m. UTC | #17
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;
Thinh Nguyen July 9, 2022, 12:04 a.m. UTC | #18
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 | #19
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
Thinh Nguyen July 9, 2022, 12:41 a.m. UTC | #20
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 {
Thinh Nguyen July 14, 2022, 1:42 a.m. UTC | #21
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
>>