Skip to contentSkip to conversation

target: Updates related to UASP

PATCH Superseded Thinh Nguyen 3 participants 11 messages Jul 19, 2022 – Jul 20, 2022

Subsystems (3)
SCSI USB SCSI target
lore
Share & tools
lore

Thinh Nguyen

author
#0 lore raw

v2 cover target: Updates related to UASP

Here are some changes splitted from the series below [*]:
"[PATCH 00/36] usb: gadget: f_tcm: Enhance UASP driver"

Cc [email protected]

Changes in v2:
- From review, remove the following patches either because they are incorrect or not needed:
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: Don't respond TMR_LUN_DOES_NOT_EXIST for all TMR failure
target: Introduce target_submit_tmr_fail_response
target: Include INQUIRY length
- Minor fixes to the remaining patches

[*] https://lore.kernel.org/linux-usb/[email protected]/

Thinh Nguyen (4):
target: Handle MI_REPORT_SUPPORTED_OPERATION_CODES
target: Implement TMR_ABORT_TASK_SET
target: Properly set Sense Data Length of CHECK CONDITION
target: Properly set Sense data length when copy sense

 drivers/target/target_core_alua.c      | 70 ++++++++++++++++++++++++++
 drivers/target/target_core_alua.h      |  2 +
 drivers/target/target_core_spc.c       | 14 +++++-
 drivers/target/target_core_tmr.c       | 16 ++++--
 drivers/target/target_core_transport.c | 21 ++++++--
 5 files changed, 111 insertions(+), 12 deletions(-)

base-commit: 88a15fbb47db483d06b12b1ae69f114b96361a96

Signature · 1 line
2.28.0
author
#1 lore raw

PATCH v2 1/4 target: Handle MI_REPORT_SUPPORTED_OPERATION_CODES

Microsoft Windows checks for MI_REPORT_SUPPORTED_OPERATION_CODES. Let's handle this MAINTENANCE_IN command and report supported commands.

Signed-off-by Thinh Nguyen <[email protected]>

---
Changes in v2:
- Fix spec section number
- Fix potential invalid "buf" access on error

 drivers/target/target_core_alua.c | 70 +++++++++++++++++++++++++++++++
 drivers/target/target_core_alua.h |  2 +
 drivers/target/target_core_spc.c  | 14 ++++++-
 3 files changed, 84 insertions(+), 2 deletions(-)
drivers/target/target_core_alua.c +70 -0
index b56ef8af66e7..881e04ce8544 100644
@@ -259,6 +259,76 @@ target_emulate_report_target_port_groups(struct se_cmd *cmd)
 	return 0;
 }
 
+/*
+ * REPORT_SUPPORTED_OPERATION_CODES
+ *
+ * See spc4r37 section 6.35
+ */
+sense_reason_t
+target_emulate_report_supported_opcodes(struct se_cmd *cmd)
+{
+	unsigned char *cdb = cmd->t_task_cdb;
+	unsigned char *buf;
+
+	/*
+	 * Only supports reporting option 1.
+	 * i.e. one command parameter with no service action.
+	 */
+	if ((cdb[2] & 0x7) != 1) {
+		pr_warn("Unsupported reporting op code %d\n", cdb[2]);
+		return TCM_UNSUPPORTED_SCSI_OPCODE;
+	}
+
+	buf = transport_kmap_data_sg(cmd);
+	if (!buf)
+		return TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE;
+
+	switch (cdb[3]) {
+	case INQUIRY:
+	case MODE_SENSE:
+	case MODE_SENSE_10:
+	case READ_CAPACITY:
+	case SERVICE_ACTION_IN_16:
+	case REPORT_LUNS:
+	case REQUEST_SENSE:
+	case SYNCHRONIZE_CACHE:
+	case REZERO_UNIT:
+	case SEEK_6:
+	case SEEK_10:
+	case TEST_UNIT_READY:
+	case SEND_DIAGNOSTIC:
+	case MAINTENANCE_IN:
+	case READ_6:
+	case READ_10:
+	case READ_16:
+	case WRITE_6:
+	case WRITE_10:
+	case WRITE_16:
+	case VERIFY_16:
+	case MODE_SELECT:
+	case MODE_SELECT_10:
+	case START_STOP:
+	case SECURITY_PROTOCOL_IN:
+	case SECURITY_PROTOCOL_OUT:
+		/* Supported */
+		buf[1] = 3;
+		break;
+	case ATA_12:
+	case ATA_16:
+	case VERIFY:
+	case ZBC_IN:
+	case ZBC_OUT:
+	default:
+		/* Not supported */
+		buf[1] = 1;
+		break;
+	}
+
+	transport_kunmap_data_sg(cmd);
+	target_complete_cmd(cmd, SAM_STAT_GOOD);
+	return 0;
+}
+
 /*
  * SET_TARGET_PORT_GROUPS for explicit ALUA operation.
  *
drivers/target/target_core_alua.h +2 -0
index fc9637cce825..7941e4dd4f97 100644
@@ -82,6 +82,8 @@ extern struct kmem_cache *t10_alua_tg_pt_gp_cache;
 extern struct kmem_cache *t10_alua_lba_map_cache;
 extern struct kmem_cache *t10_alua_lba_map_mem_cache;
 
+extern sense_reason_t
+target_emulate_report_supported_opcodes(struct se_cmd *cmd);
 extern sense_reason_t target_emulate_report_target_port_groups(struct se_cmd *);
 extern sense_reason_t target_emulate_set_target_port_groups(struct se_cmd *);
 extern sense_reason_t target_emulate_report_referrals(struct se_cmd *);
drivers/target/target_core_spc.c +12 -2
index c14441c89bed..dd799158609d 100644
@@ -1425,15 +1425,25 @@ spc_parse_cdb(struct se_cmd *cmd, unsigned int *size)
 		break;
 	case MAINTENANCE_IN:
 		if (dev->transport->get_device_type(dev) != TYPE_ROM) {
+			u8 action = cdb[1] & 0x1f;
+
 			/*
 			 * MAINTENANCE_IN from SCC-2
 			 * Check for emulated MI_REPORT_TARGET_PGS
 			 */
-			if ((cdb[1] & 0x1f) == MI_REPORT_TARGET_PGS) {
+			if (action == MI_REPORT_TARGET_PGS) {
 				cmd->execute_cmd =
 					target_emulate_report_target_port_groups;
+
+				*size = get_unaligned_be32(&cdb[6]);
+			}
+
+			if (action == MI_REPORT_SUPPORTED_OPERATION_CODES) {
+				cmd->execute_cmd =
+					target_emulate_report_supported_opcodes;
+
+				*size = get_unaligned_be16(&cdb[2]);
 			}
-			*size = get_unaligned_be32(&cdb[6]);
 		} else {
 			/*
 			 * GPCMD_SEND_KEY from multi media commands
Signature · 1 line
2.28.0
author
#3 lore raw

Re: [PATCH v2 1/4] target: Handle MI_REPORT_SUPPORTED_OPERATION_CODES

Quoted 6 lines · Konstantin Shelekhin
Quoted 2 lines · Thinh Nguyen

Microsoft Windows checks for MI_REPORT_SUPPORTED_OPERATION_CODES. Let's handle this MAINTENANCE_IN command and report supported commands.

Please see the RSOC patch series[1].

[1]: https://urldefense.com/v3/__https://lore.kernel.org/target-devel/[email protected]/T/*t__;Iw!!A4F2R9G_pg!aSU50xkFUuW-1Obo9oSgdrgMVpjxb7idbtKhNzxCaY4UlbYxcNi1LFozr0U8iq9JdjyZWGP0wLCJLq3qgwHHKoEiQQ$

Great!

Thanks,
Thinh

author
#5 lore raw

Re: [PATCH v2 1/4] target: Handle MI_REPORT_SUPPORTED_OPERATION_CODES

Quoted 9 lines · Mike Christie
Quoted 3 lines · Thinh Nguyen

Microsoft Windows checks for MI_REPORT_SUPPORTED_OPERATION_CODES. Let's handle this MAINTENANCE_IN command and report supported commands.

Let's concentrate on Dimitry's patchset for this:

https://urldefense.com/v3/__https://lore.kernel.org/linux-scsi/[email protected]/T/*t__;Iw!!A4F2R9G_pg!adGsdmnAWvf_URZOmsz8HTGyKk_UyFe9urhtF3SvIigtdGvV8NwGQ9TE46cwDPN1SRJCIyszaqvV1cDF3wuHsyc9bfCmunY$

It's complete and supports when users can enable/disable commands.

Right, I wasn't aware. It's great that someone's adding full support for it. I'm dropping this patch.

Thanks,
Thinh

author
#6 lore raw

PATCH v2 2/4 target: Implement TMR_ABORT_TASK_SET

In reply to Thinh Nguyen

Task ABORT TASK SET function is required by SCSI transport protocol standards (SAM-4 r14 section 7.3). It is similar to ABORT TASK function, but it applies to all commands received on a specified I_T nexus rather than a specific referenced command. Modify core_tmr_abort_task() to support TMR_ABORT_TASK_SET.

Signed-off-by Thinh Nguyen <[email protected]>

---
Changes in v2:
- None

 drivers/target/target_core_tmr.c       | 16 +++++++++++-----
 drivers/target/target_core_transport.c |  2 +-
 2 files changed, 12 insertions(+), 6 deletions(-)
drivers/target/target_core_tmr.c +11 -5
index bac111456fa1..1ea72e15f872 100644
@@ -131,11 +131,13 @@ void core_tmr_abort_task(
 				continue;
 
 			ref_tag = se_cmd->tag;
-			if (tmr->ref_task_tag != ref_tag)
-				continue;
+			if (tmr->function == TMR_ABORT_TASK) {
+				if (tmr->ref_task_tag != ref_tag)
+					continue;
 
-			pr_err("ABORT_TASK: Found referenced %s task_tag: %llu\n",
-			       se_cmd->se_tfo->fabric_name, ref_tag);
+				pr_err("ABORT_TASK: Found referenced %s task_tag: %llu\n",
+				       se_cmd->se_tfo->fabric_name, ref_tag);
+			}
 
 			spin_lock(&se_sess->sess_cmd_lock);
 			rc = __target_check_io_state(se_cmd, se_sess, 0);

@@ -158,7 +160,11 @@ void core_tmr_abort_task(
 			       ref_tag);
 			tmr->response = TMR_FUNCTION_COMPLETE;
 			atomic_long_inc(&dev->aborts_complete);
-			return;
+
+			if (tmr->function == TMR_ABORT_TASK)
+				return;
+
+			spin_lock_irqsave(&dev->queues[i].lock, flags);
 		}
 		spin_unlock_irqrestore(&dev->queues[i].lock, flags);
 	}
drivers/target/target_core_transport.c +1 -1
index 7838dc20f713..8c386142ef90 100644
@@ -3519,9 +3519,9 @@ static void target_tmr_work(struct work_struct *work)
 
 	switch (tmr->function) {
 	case TMR_ABORT_TASK:
+	case TMR_ABORT_TASK_SET:
 		core_tmr_abort_task(dev, tmr, cmd->se_sess);
 		break;
-	case TMR_ABORT_TASK_SET:
 	case TMR_CLEAR_ACA:
 	case TMR_CLEAR_TASK_SET:
 		tmr->response = TMR_TASK_MGMT_FUNCTION_NOT_SUPPORTED;
Signature · 1 line
2.28.0
#7 lore raw

Re: [PATCH v2 2/4] target: Implement TMR_ABORT_TASK_SET

Quoted 1 line · Thinh Nguyen

Task ABORT TASK SET function is required by SCSI transport protocol

What OS is using this and how do they use it? For the latter, does the OS try an abort for each cmd first, then try an abort task set if the aborts fail (does fail mean get a response that indicates failure and also does a timeout count)? Or does it start with the abort task set?

I'm asking because it looks like if it does an abort first, then the abort task set will always return TMR_TASK_DOES_NOT_EXIST. For the abort we will remove the cmds from the state_list so if the abort task set runs after the initiator has tried to abort all the commands it will never find any.

Quoted 5 lines

standards (SAM-4 r14 section 7.3). It is similar to ABORT TASK function, but it applies to all commands received on a specified I_T nexus rather than a specific referenced command. Modify core_tmr_abort_task() to support TMR_ABORT_TASK_SET.

author
#8 lore raw

Re: [PATCH v2 2/4] target: Implement TMR_ABORT_TASK_SET

Quoted 6 lines · Mike Christie
Quoted 1 line · Thinh Nguyen

Task ABORT TASK SET function is required by SCSI transport protocol

What OS is using this and how do they use it? For the latter, does the OS try an abort for each cmd first, then try an abort task set if the aborts fail (does fail mean get a response that indicates failure and also does a timeout count)? Or does it start with the abort task set?

It's not from any real driver. It's from the USB Compliant Verification
(https://www.usb.org/document-library/usb3cv). It uses the command for UASP compliant test.

The test only ever aborts a single command at a time, so I can't confirm your following questions. The SAM4-r14 wasn't clear on those questions either.

Quoted 5 lines

I'm asking because it looks like if it does an abort first, then the abort task set will always return TMR_TASK_DOES_NOT_EXIST. For the abort we will remove the cmds from the state_list so if the abort task set runs after the initiator has tried to abort all the commands it will never find any.

I didn't notice since I dropped a patch where I removed the TMR_TASK_DOES_NOT_EXIST and UASP converts this to RC_TMF_COMPLETE. UASP respond to FUNCTION COMPLETE with RC_TMF_COMPLETE. I'll can make a fix to that.

If there's any suggestion to implement this, please advise.

Thanks,
Thinh

Quoted 5 lines
Quoted 5 lines

standards (SAM-4 r14 section 7.3). It is similar to ABORT TASK function, but it applies to all commands received on a specified I_T nexus rather than a specific referenced command. Modify core_tmr_abort_task() to support TMR_ABORT_TASK_SET.

#9 lore raw

Re: [PATCH v2 2/4] target: Implement TMR_ABORT_TASK_SET

In reply to Mike Christie
Quoted 9 lines · Mike Christie

«Внимание! Данное письмо от внешнего адресата!»

Quoted 1 line · Thinh Nguyen

Task ABORT TASK SET function is required by SCSI transport protocol

What OS is using this and how do they use it? For the latter, does the OS try an abort for each cmd first, then try an abort task set if the aborts fail (does fail mean get a response that indicates failure and also does a timeout count)? Or does it start with the abort task set?

AIX IIRC. However, this feature also requires valid bits in one of the VPD pages.

author
#10 lore raw

PATCH v2 3/4 target: Properly set Sense Data Length of CHECK CONDITION

In reply to Thinh Nguyen

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 <[email protected]>

---
Changes in v2:
- Make sure it doesn't overflow and properly pass TRANSPORT_SENSE_BUFFER to scsi_set_sense_information()

 drivers/target/target_core_transport.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)
drivers/target/target_core_transport.c +9 -2
index 8c386142ef90..db53b326048d 100644
@@ -3459,12 +3459,19 @@ 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;
+
 	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,
+							TRANSPORT_SENSE_BUFFER,
 							cmd->sense_info) < 0);
+	/*
+	 * CHECK CONDITION returns sense data, and sense data is minimum 8
+	 * bytes long plus additional Sense Data Length.
+	 * See SPC4-r37 section 4.5.2.1.
+	 */
+	cmd->scsi_sense_length = min_t(u16, buffer[7] + 8,
+				       TRANSPORT_SENSE_BUFFER);
 }
 
 int
Signature · 1 line
2.28.0