Skip to contentSkip to conversation

tcm: Add VARIABLE_LENGTH_CMD support w/ XDWRITE_READ_32 emulation

PATCH In review Nicholas A. Bellinger 3 participants 5 messages Sep 17, 2010 – Sep 20, 2010

Subsystems (2)
SCSI SCSI target
lore
Share & tools
lore

Nicholas A. Bellinger

author
#0 lore raw

PATCH 2/3 tcm: Add VARIABLE_LENGTH_CMD support w/ XDWRITE_READ_32 emulation

From: Nicholas Bellinger <[email protected]>

This patch updates transport_generic_cmd_sequencer() to properly support VARIABLE_LENGTH_CMD w/ service action XDWRITE_READ_32 emulation. This patch follows the original XDWRITE_READ_10 patch, and uses the new 32-byte CDB extraction callers from commit 39a347ca2d88. Note this patch uses the same transport_xor_callback() assignment callback as XDWRITE_READ_10.

Also note that this patch enforces the following > TCM_MAX_COMMAND_SIZE check for VARIABLE_LENGTH_CMD CDBs in transport_generic_cmd_sequencer():

	<SNIP>
        /*
         * Check the additional CDB length (+ 8 bytes for header) does
         * not exceed our TCM_MAX_COMMAND_SIZE.
         */
         if ((cdb[7] + 8) > TCM_MAX_COMMAND_SIZE) {
                printk(KERN_INFO "Only %u-byte extended CDBs currently"
                         " supported for VARIABLE_LENGTH_CMD, received:"
                         " %d for service action: 0x%04x\n",
                         TCM_MAX_COMMAND_SIZE, cdb[7], service_action);
                return TGCS_INVALID_CDB_FIELD;
         }
	<SNIP>
Signed-off-by Nicholas A. Bellinger <[email protected]>
---
 drivers/target/target_core_transport.c |   50 +++++++++++++++++++++++++++----
 1 files changed, 43 insertions(+), 7 deletions(-)
drivers/target/target_core_transport.c +43 -7
index a3016f7..a20a4a9 100644
@@ -5401,6 +5401,7 @@ static int transport_generic_cmd_sequencer(
 	struct se_subsystem_dev *su_dev = dev->se_sub_dev;
 	int ret, sector_ret = 0;
 	u32 sectors = 0, size = 0, pr_reg_type = 0;
+	u16 service_action;
 	u8 alua_ascq = 0;
 	/*
 	 * Check for an existing UNIT ATTENTION condition

@@ -5572,6 +5573,48 @@ static int transport_generic_cmd_sequencer(
 		T_TASK(cmd)->t_tasks_fua = (cdb[1] & 0x8);
 		ret = TGCS_DATA_SG_IO_CDB;
 		break;
+	case VARIABLE_LENGTH_CMD:
+		SET_GENERIC_TRANSPORT_FUNCTIONS(cmd);
+		service_action = (cdb[8] << 8) | cdb[9];
+		/*
+		 * Check the additional CDB length (+ 8 bytes for header) does
+		 * not exceed our TCM_MAX_COMMAND_SIZE.
+		 */
+		if ((cdb[7] + 8) > TCM_MAX_COMMAND_SIZE) {
+			printk(KERN_INFO "Only %u-byte extended CDBs currently"
+				" supported for VARIABLE_LENGTH_CMD, received:"
+				" %d for service action: 0x%04x\n",
+				TCM_MAX_COMMAND_SIZE, cdb[7], service_action);
+			return TGCS_INVALID_CDB_FIELD;
+		}
+		switch (service_action) {
+		case 0x0007: /* XDWRITE_READ_32 */
+			sectors = transport_get_sectors_32(cdb, cmd, &sector_ret);
+			if (sector_ret)
+				return TGCS_UNSUPPORTED_CDB;
+			size = transport_get_size(sectors, cdb, cmd);
+			transport_dev_get_mem_SG(cmd->se_orig_obj_ptr, cmd);
+			transport_get_maps(cmd);
+			/*
+			 * Use WRITE_32 and READ_32 opcodes for the emulated
+			 * XDWRITE_READ_32 logic.
+			 */
+			cmd->transport_split_cdb = &split_cdb_XX_32;
+			cmd->transport_get_long_lba = &transport_lba_64_ext;
+			/*
+			 * Setup BIDI XOR callback to be run during
+			 * transport_generic_complete_ok()
+			 */
+			cmd->transport_complete_callback = &transport_xor_callback;
+			T_TASK(cmd)->t_tasks_fua = (cdb[10] & 0x8);
+			ret = TGCS_DATA_SG_IO_CDB;	
+			break;
+		default:
+			printk(KERN_ERR "VARIABLE_LENGTH_CMD service action"
+				" 0x%04x not supported\n", service_action);
+			return TGCS_UNSUPPORTED_CDB;
+		}
+		break;
 	case 0xa3:
 		SET_GENERIC_TRANSPORT_FUNCTIONS(cmd);
 		if (TRANSPORT(dev)->get_device_type(dev) != TYPE_ROM) {

@@ -5747,13 +5790,6 @@ static int transport_generic_cmd_sequencer(
 		transport_get_maps(cmd);
 		ret = TGCS_CONTROL_NONSG_IO_CDB;
 		break;
-	case VARIABLE_LENGTH_CMD:
-		SET_GENERIC_TRANSPORT_FUNCTIONS(cmd);
-		size = (cdb[10] << 8) | cdb[11];
-		transport_dev_get_mem_buf(cmd->se_orig_obj_ptr, cmd);
-		transport_get_maps(cmd);
-		ret = TGCS_CONTROL_NONSG_IO_CDB;
-		break;
 	case RECEIVE_DIAGNOSTIC:
 	case SEND_DIAGNOSTIC:
 		SET_GENERIC_TRANSPORT_FUNCTIONS(cmd);
Signature · 1 line
1.5.6.5
#1 lore raw
Quoted 43 lines · Nicholas A. Bellinger

From: Nicholas Bellinger <[email protected]>

This patch updates transport_generic_cmd_sequencer() to properly support VARIABLE_LENGTH_CMD w/ service action XDWRITE_READ_32 emulation. This patch follows the original XDWRITE_READ_10 patch, and uses the new 32-byte CDB extraction callers from commit 39a347ca2d88. Note this patch uses the same transport_xor_callback() assignment callback as XDWRITE_READ_10.

Also note that this patch enforces the following > TCM_MAX_COMMAND_SIZE check for VARIABLE_LENGTH_CMD CDBs in transport_generic_cmd_sequencer():

	<SNIP>
        /*
         * Check the additional CDB length (+ 8 bytes for header) does
         * not exceed our TCM_MAX_COMMAND_SIZE.
         */
         if ((cdb[7] + 8) > TCM_MAX_COMMAND_SIZE) {
                printk(KERN_INFO "Only %u-byte extended CDBs currently"
                         " supported for VARIABLE_LENGTH_CMD, received:"
                         " %d for service action: 0x%04x\n",
                         TCM_MAX_COMMAND_SIZE, cdb[7], service_action);
                return TGCS_INVALID_CDB_FIELD;
         }
	<SNIP>
Signed-off-by Nicholas A. Bellinger <[email protected]>
---
 drivers/target/target_core_transport.c |   50 +++++++++++++++++++++++++++----
 1 files changed, 43 insertions(+), 7 deletions(-)
drivers/target/target_core_transport.c +1 -0
index a3016f7..a20a4a9 100644
@@ -5401,6 +5401,7 @@ static int transport_generic_cmd_sequencer(
 	struct se_subsystem_dev *su_dev = dev->se_sub_dev;
 	int ret, sector_ret = 0;
 	u32 sectors = 0, size = 0, pr_reg_type = 0;
+	u16 service_action;
 	u8 alua_ascq = 0;
 	/*
 	 * Check for an existing UNIT ATTENTION condition

drivers/target/target_core_transport.c static int transport_generic_cmd_sequencer( · line 5578

Quoted 6 lines
drivers/target/target_core_transport.c +3 -0
@@ -5572,6 +5573,48 @@ static int transport_generic_cmd_sequencer(
 		T_TASK(cmd)->t_tasks_fua = (cdb[1] & 0x8);
 		ret = TGCS_DATA_SG_IO_CDB;
 		break;
+	case VARIABLE_LENGTH_CMD:
+		SET_GENERIC_TRANSPORT_FUNCTIONS(cmd);
+		service_action = (cdb[8] << 8) | cdb[9];

get_unaligned_be16

drivers/target/target_core_transport.c static int transport_generic_cmd_sequencer( · line 5583

Quoted 5 lines
drivers/target/target_core_transport.c +5 -0
+		/*
+		 * Check the additional CDB length (+ 8 bytes for header) does
+		 * not exceed our TCM_MAX_COMMAND_SIZE.
+		 */
+		if ((cdb[7] + 8) > TCM_MAX_COMMAND_SIZE) {

scsi_varlen_cdb_length(cdb)
it's in scsi.h

drivers/target/target_core_transport.c static int transport_generic_cmd_sequencer( · line 5591

Quoted 8 lines
drivers/target/target_core_transport.c +8 -0
+			printk(KERN_INFO "Only %u-byte extended CDBs currently"
+				" supported for VARIABLE_LENGTH_CMD, received:"
+				" %d for service action: 0x%04x\n",
+				TCM_MAX_COMMAND_SIZE, cdb[7], service_action);
+			return TGCS_INVALID_CDB_FIELD;
+		}
+		switch (service_action) {
+		case 0x0007: /* XDWRITE_READ_32 */

Is it about time to define all these service_action codes. I have the OSD defines in an OSD header. So I'm GUILTY just as much.
(but I do have an enum in a public header, at least)

Quoted 33 lines
drivers/target/target_core_transport.c +26 -0
+			sectors = transport_get_sectors_32(cdb, cmd, &sector_ret);
+			if (sector_ret)
+				return TGCS_UNSUPPORTED_CDB;
+			size = transport_get_size(sectors, cdb, cmd);
+			transport_dev_get_mem_SG(cmd->se_orig_obj_ptr, cmd);
+			transport_get_maps(cmd);
+			/*
+			 * Use WRITE_32 and READ_32 opcodes for the emulated
+			 * XDWRITE_READ_32 logic.
+			 */
+			cmd->transport_split_cdb = &split_cdb_XX_32;
+			cmd->transport_get_long_lba = &transport_lba_64_ext;
+			/*
+			 * Setup BIDI XOR callback to be run during
+			 * transport_generic_complete_ok()
+			 */
+			cmd->transport_complete_callback = &transport_xor_callback;
+			T_TASK(cmd)->t_tasks_fua = (cdb[10] & 0x8);
+			ret = TGCS_DATA_SG_IO_CDB;	
+			break;
+		default:
+			printk(KERN_ERR "VARIABLE_LENGTH_CMD service action"
+				" 0x%04x not supported\n", service_action);
+			return TGCS_UNSUPPORTED_CDB;
+		}
+		break;
 	case 0xa3:
 		SET_GENERIC_TRANSPORT_FUNCTIONS(cmd);
 		if (TRANSPORT(dev)->get_device_type(dev) != TYPE_ROM) {

@@ -5747,13 +5790,6 @@ static int transport_generic_cmd_sequencer(
 		transport_get_maps(cmd);
 		ret = TGCS_CONTROL_NONSG_IO_CDB;
 		break;

drivers/target/target_core_transport.c static int transport_generic_cmd_sequencer( · line 5795

Quoted 10 lines
drivers/target/target_core_transport.c +0 -7
-	case VARIABLE_LENGTH_CMD:
-		SET_GENERIC_TRANSPORT_FUNCTIONS(cmd);
-		size = (cdb[10] << 8) | cdb[11];
-		transport_dev_get_mem_buf(cmd->se_orig_obj_ptr, cmd);
-		transport_get_maps(cmd);
-		ret = TGCS_CONTROL_NONSG_IO_CDB;
-		break;
 	case RECEIVE_DIAGNOSTIC:
 	case SEND_DIAGNOSTIC:
 		SET_GENERIC_TRANSPORT_FUNCTIONS(cmd);

Thanks
Boaz

author
#2 lore raw
Quoted 43 lines · Boaz Harrosh
Quoted 42 lines · Nicholas A. Bellinger

From: Nicholas Bellinger <[email protected]>

This patch updates transport_generic_cmd_sequencer() to properly support VARIABLE_LENGTH_CMD w/ service action XDWRITE_READ_32 emulation. This patch follows the original XDWRITE_READ_10 patch, and uses the new 32-byte CDB extraction callers from commit 39a347ca2d88. Note this patch uses the same transport_xor_callback() assignment callback as XDWRITE_READ_10.

Also note that this patch enforces the following > TCM_MAX_COMMAND_SIZE check for VARIABLE_LENGTH_CMD CDBs in transport_generic_cmd_sequencer():

	<SNIP>
        /*
         * Check the additional CDB length (+ 8 bytes for header) does
         * not exceed our TCM_MAX_COMMAND_SIZE.
         */
         if ((cdb[7] + 8) > TCM_MAX_COMMAND_SIZE) {
                printk(KERN_INFO "Only %u-byte extended CDBs currently"
                         " supported for VARIABLE_LENGTH_CMD, received:"
                         " %d for service action: 0x%04x\n",
                         TCM_MAX_COMMAND_SIZE, cdb[7], service_action);
                return TGCS_INVALID_CDB_FIELD;
         }
	<SNIP>
Signed-off-by Nicholas A. Bellinger <[email protected]>
---
 drivers/target/target_core_transport.c |   50 +++++++++++++++++++++++++++----
 1 files changed, 43 insertions(+), 7 deletions(-)
drivers/target/target_core_transport.c +1 -0
index a3016f7..a20a4a9 100644
@@ -5401,6 +5401,7 @@ static int transport_generic_cmd_sequencer(
 	struct se_subsystem_dev *su_dev = dev->se_sub_dev;
 	int ret, sector_ret = 0;
 	u32 sectors = 0, size = 0, pr_reg_type = 0;
+	u16 service_action;
 	u8 alua_ascq = 0;
 	/*
 	 * Check for an existing UNIT ATTENTION condition
Quoted 8 lines

drivers/target/target_core_transport.c static int transport_generic_cmd_sequencer( · line 5578

Quoted 7 lines
drivers/target/target_core_transport.c +3 -0
@@ -5572,6 +5573,48 @@ static int transport_generic_cmd_sequencer(
 		T_TASK(cmd)->t_tasks_fua = (cdb[1] & 0x8);
 		ret = TGCS_DATA_SG_IO_CDB;
 		break;
+	case VARIABLE_LENGTH_CMD:
+		SET_GENERIC_TRANSPORT_FUNCTIONS(cmd);
+		service_action = (cdb[8] << 8) | cdb[9];

get_unaligned_be16

Fixed

Quoted 8 lines

drivers/target/target_core_transport.c static int transport_generic_cmd_sequencer( · line 5583

Quoted 5 lines
drivers/target/target_core_transport.c +5 -0
+		/*
+		 * Check the additional CDB length (+ 8 bytes for header) does
+		 * not exceed our TCM_MAX_COMMAND_SIZE.
+		 */
+		if ((cdb[7] + 8) > TCM_MAX_COMMAND_SIZE) {

scsi_varlen_cdb_length(cdb)
it's in scsi.h

Fixed

Quoted 14 lines

drivers/target/target_core_transport.c static int transport_generic_cmd_sequencer( · line 5591

Quoted 9 lines
drivers/target/target_core_transport.c +8 -0
+			printk(KERN_INFO "Only %u-byte extended CDBs currently"
+				" supported for VARIABLE_LENGTH_CMD, received:"
+				" %d for service action: 0x%04x\n",
+				TCM_MAX_COMMAND_SIZE, cdb[7], service_action);
+			return TGCS_INVALID_CDB_FIELD;
+		}
+		switch (service_action) {
+		case 0x0007: /* XDWRITE_READ_32 */

Is it about time to define all these service_action codes. I have the OSD defines in an OSD header. So I'm GUILTY just as much.
(but I do have an enum in a public header, at least)

Yes, I was being a bit lazy here instead of just adding them to include/scsi/scsi.h, but I will add them into the forth-coming RFCv2 series..

Btw, I have not been able to find a definitive list of the service actions of VARIABLE_LENGTH_CMD for TYPE_DISK in the SBC3 spec that need to be added to scsi.h. Have you come across one in your own readings for these..?

Thanks!

#3 lore raw
Quoted 81 lines · Nicholas A. Bellinger
Quoted 43 lines · Boaz Harrosh
Quoted 42 lines · Nicholas A. Bellinger

From: Nicholas Bellinger <[email protected]>

This patch updates transport_generic_cmd_sequencer() to properly support VARIABLE_LENGTH_CMD w/ service action XDWRITE_READ_32 emulation. This patch follows the original XDWRITE_READ_10 patch, and uses the new 32-byte CDB extraction callers from commit 39a347ca2d88. Note this patch uses the same transport_xor_callback() assignment callback as XDWRITE_READ_10.

Also note that this patch enforces the following > TCM_MAX_COMMAND_SIZE check for VARIABLE_LENGTH_CMD CDBs in transport_generic_cmd_sequencer():

	<SNIP>
        /*
         * Check the additional CDB length (+ 8 bytes for header) does
         * not exceed our TCM_MAX_COMMAND_SIZE.
         */
         if ((cdb[7] + 8) > TCM_MAX_COMMAND_SIZE) {
                printk(KERN_INFO "Only %u-byte extended CDBs currently"
                         " supported for VARIABLE_LENGTH_CMD, received:"
                         " %d for service action: 0x%04x\n",
                         TCM_MAX_COMMAND_SIZE, cdb[7], service_action);
                return TGCS_INVALID_CDB_FIELD;
         }
	<SNIP>
Signed-off-by Nicholas A. Bellinger <[email protected]>
---
 drivers/target/target_core_transport.c |   50 +++++++++++++++++++++++++++----
 1 files changed, 43 insertions(+), 7 deletions(-)
drivers/target/target_core_transport.c +1 -0
index a3016f7..a20a4a9 100644
@@ -5401,6 +5401,7 @@ static int transport_generic_cmd_sequencer(
 	struct se_subsystem_dev *su_dev = dev->se_sub_dev;
 	int ret, sector_ret = 0;
 	u32 sectors = 0, size = 0, pr_reg_type = 0;
+	u16 service_action;
 	u8 alua_ascq = 0;
 	/*
 	 * Check for an existing UNIT ATTENTION condition
Quoted 8 lines

drivers/target/target_core_transport.c static int transport_generic_cmd_sequencer( · line 5578

Quoted 7 lines
drivers/target/target_core_transport.c +3 -0
@@ -5572,6 +5573,48 @@ static int transport_generic_cmd_sequencer(
 		T_TASK(cmd)->t_tasks_fua = (cdb[1] & 0x8);
 		ret = TGCS_DATA_SG_IO_CDB;
 		break;
+	case VARIABLE_LENGTH_CMD:
+		SET_GENERIC_TRANSPORT_FUNCTIONS(cmd);
+		service_action = (cdb[8] << 8) | cdb[9];

get_unaligned_be16

Fixed

Quoted 8 lines

drivers/target/target_core_transport.c static int transport_generic_cmd_sequencer( · line 5583

Quoted 5 lines
drivers/target/target_core_transport.c +5 -0
+		/*
+		 * Check the additional CDB length (+ 8 bytes for header) does
+		 * not exceed our TCM_MAX_COMMAND_SIZE.
+		 */
+		if ((cdb[7] + 8) > TCM_MAX_COMMAND_SIZE) {

scsi_varlen_cdb_length(cdb)
it's in scsi.h

Fixed

Quoted 14 lines

drivers/target/target_core_transport.c static int transport_generic_cmd_sequencer( · line 5591

Quoted 9 lines
drivers/target/target_core_transport.c +8 -0
+			printk(KERN_INFO "Only %u-byte extended CDBs currently"
+				" supported for VARIABLE_LENGTH_CMD, received:"
+				" %d for service action: 0x%04x\n",
+				TCM_MAX_COMMAND_SIZE, cdb[7], service_action);
+			return TGCS_INVALID_CDB_FIELD;
+		}
+		switch (service_action) {
+		case 0x0007: /* XDWRITE_READ_32 */

Is it about time to define all these service_action codes. I have the OSD defines in an OSD header. So I'm GUILTY just as much.
(but I do have an enum in a public header, at least)

Quoted 9 lines

Yes, I was being a bit lazy here instead of just adding them to include/scsi/scsi.h, but I will add them into the forth-coming RFCv2 series..

Btw, I have not been able to find a definitive list of the service actions of VARIABLE_LENGTH_CMD for TYPE_DISK in the SBC3 spec that need to be added to scsi.h. Have you come across one in your own readings for these..?

Sorry, no, I'm only an OSD guy. Perhaps Martin or Matthew

Quoted 2 lines

Thanks!

Boaz

#4 lore raw
Quoted 1 line · Boaz Harrosh

On 09/20/2010 10:58 AM, Nicholas A. Bellinger wrote:

[snip]

Quoted 7 lines
Quoted 5 lines

Btw, I have not been able to find a definitive list of the service actions of VARIABLE_LENGTH_CMD for TYPE_DISK in the SBC3 spec that need to be added to scsi.h. Have you come across one in your own readings for these..?

Sorry, no, I'm only an OSD guy. Perhaps Martin or Matthew

spc4r26.pdf Annex D.3.5
sbc3r24.pdf Annex A.1

Doug Gilbert