tcm_loop: Add DMA_BIDIRECTIONAL CDB support

PATCH In review Nicholas A. Bellinger 2 participants 3 messages Sep 15, 2010

Subsystems (2)

SCSI SCSI target

lore

Share & tools

lore

Nicholas A. Bellinger

author

2010-09-15 12:06 UTC #0 lore raw

PATCH 2/2 tcm_loop: Add DMA_BIDIRECTIONAL CDB support

From: Nicholas Bellinger <[email protected]>

This patch series adds DMA_BIDIRECTIONAL CDB to the TCM_Loop Virtual SCSI LLD module. This includes the following changes:

*) Add call to include/scsi/scsi_cmnd.h:scsi_bidi_cmnd() to set
TCM struct se_cmd internal data_direction in tcm_loop_allocate_core_cmd()
*) Update tcm_loop_new_cmd_map() to handle BIDI READ payload from scsi_in() *) Drop DMA_BIDIRECTIONAL usage check from tcm_loop_queuecommand() *) Add tcm_loop_slave_alloc() and tcm_loop_slave_configure() handlers for
struct scsi_host_template to set QUEUE_FLAG_BIDI in the former.

So far this has been tested together with commit 1f8f3eda132c using XDWRITE_READ_10 via BSG userspace code on a v2.6.36-rc3 kernel.

Signed-off-by Nicholas A. Bellinger <[email protected]>

---
 drivers/target/tcm_loop/tcm_loop_fabric_scsi.c |   48 ++++++++++++++++++------
 1 files changed, 36 insertions(+), 12 deletions(-)

drivers/target/tcm_loop/tcm_loop_fabric_scsi.c +36 ~~-12~~

index 644f978..c4139d8 100644
@@ -64,6 +64,7 @@ static struct se_cmd *tcm_loop_allocate_core_cmd(
 	struct tcm_loop_nexus *tl_nexus = tl_hba->tl_nexus;
 	struct tcm_loop_cmd *tl_cmd;
 	int sam_task_attr;
+	enum dma_data_direction data_direction;
 
 	if (!(tl_nexus)) {
 		scmd_printk(KERN_ERR, sc, "TCM_Loop I_T Nexus"

@@ -81,6 +82,13 @@ static struct se_cmd *tcm_loop_allocate_core_cmd(
 	 * Save the pointer to struct scsi_cmnd *sc
 	 */
 	tl_cmd->sc = sc;
+	/*
+	 * Check for scsi_bidi_cmnd() to signal TCM Core for DMA_BIDIRECTIONAL
+	 */
+	if (scsi_bidi_cmnd(sc))
+		data_direction = DMA_BIDIRECTIONAL;
+	else
+		data_direction = sc->sc_data_direction;
 
 	/*
 	 * Locate the SAM Task Attr from struct scsi_cmnd *

@@ -105,7 +113,7 @@ static struct se_cmd *tcm_loop_allocate_core_cmd(
 	 * Initialize struct se_cmd descriptor from target_core_mod infrastructure
 	 */
 	transport_init_se_cmd(se_cmd, se_tpg->se_tpg_tfo, se_sess,
-			scsi_bufflen(sc), sc->sc_data_direction, sam_task_attr,
+			scsi_bufflen(sc), data_direction, sam_task_attr,
 			&tl_cmd->tl_sense_buf[0]);
 	/*
 	 * Locate the struct se_lun pointer and attach it to struct se_cmd

@@ -128,14 +136,16 @@ static struct se_cmd *tcm_loop_allocate_core_cmd(
 /*
  * Called by struct target_core_fabric_ops->new_cmd_map()
  *
- * Always called in process context
+ * Always called in process context.  A non zero return value
+ * here will signal to handle an exception based on the return code.
  */
 int tcm_loop_new_cmd_map(struct se_cmd *se_cmd)
 {
 	struct tcm_loop_cmd *tl_cmd = container_of(se_cmd,
 				struct tcm_loop_cmd, tl_se_cmd);
 	struct scsi_cmnd *sc = tl_cmd->sc;
-	void *mem_ptr;
+	void *mem_ptr, *mem_bidi_ptr = NULL;
+	u32 sg_no_bidi = 0;
 	int ret;
 	/*
 	 * Allocate the necessary tasks to complete the received CDB+data

@@ -163,6 +173,16 @@ int tcm_loop_new_cmd_map(struct se_cmd *se_cmd)
 	if (scsi_sg_count(sc)) {
 		se_cmd->se_cmd_flags |= SCF_PASSTHROUGH_SG_TO_MEM;
 		mem_ptr = (void *)scsi_sglist(sc);
+		/*
+		 * For BIDI commands, pass in the extra READ buffer
+		 * to transport_generic_map_mem_to_cmd() below..
+		 */
+		if (se_cmd->data_direction == DMA_BIDIRECTIONAL) {
+			struct scsi_data_buffer *sdb = scsi_in(sc);
+
+			mem_bidi_ptr = (void *)sdb->table.sgl;
+			sg_no_bidi = sdb->table.nents;
+		}
 	} else {
 		/*
 		 * Used for DMA_NONE

@@ -322,15 +342,6 @@ static int tcm_loop_queuecommand(
 	}
 	tl_tpg = &tl_hba->tl_hba_tpgs[sc->device->id];
 	se_tpg = &tl_tpg->tl_se_tpg;
-
-	if (sc->sc_data_direction == DMA_BIDIRECTIONAL) {
-		spin_lock_irq(host->host_lock);
-		printk(KERN_ERR "Unsupported BIDI sc->sc_data_direction: %d\n",
-			sc->sc_data_direction);	
-		sc->result = host_byte(DID_ERROR);
-		(*done)(sc);
-		return 0;
-	}
 	/*
 	 * Determine the SAM Task Attribute and allocate tl_cmd and
 	 * tl_cmd->tl_se_cmd from TCM infrastructure

@@ -446,6 +457,17 @@ release:
 	return ret;
 }
 
+static int tcm_loop_slave_alloc(struct scsi_device *sd)
+{
+	set_bit(QUEUE_FLAG_BIDI, &sd->request_queue->queue_flags);
+	return 0;
+}
+
+static int tcm_loop_slave_configure(struct scsi_device *sd)
+{
+	return 0;
+}
+
 static struct scsi_host_template tcm_loop_driver_template = {
 	.proc_info		= tcm_loop_proc_info,
 	.proc_name		= "tcm_loopback",

@@ -468,6 +490,8 @@ static struct scsi_host_template tcm_loop_driver_template = {
 	.cmd_per_lun		= TL_SCSI_CMD_PER_LUN,
 	.max_sectors		= TL_SCSI_MAX_SECTORS,
 	.use_clustering		= DISABLE_CLUSTERING,
+	.slave_alloc		= tcm_loop_slave_alloc,
+	.slave_configure	= tcm_loop_slave_configure,
 	.module			= THIS_MODULE,
 };

Signature · 1 line

1.5.6.5

Boaz Harrosh

2010-09-15 14:08 UTC #1 lore raw

Quoted 12 lines · Nicholas A. Bellinger

From: Nicholas Bellinger <[email protected]>
This patch series adds DMA_BIDIRECTIONAL CDB to the TCM_Loop Virtual SCSI LLD module. This includes the following changes:
*) Add call to include/scsi/scsi_cmnd.h:scsi_bidi_cmnd() to set
TCM struct se_cmd internal data_direction in tcm_loop_allocate_core_cmd()
*) Update tcm_loop_new_cmd_map() to handle BIDI READ payload from scsi_in() *) Drop DMA_BIDIRECTIONAL usage check from tcm_loop_queuecommand() *) Add tcm_loop_slave_alloc() and tcm_loop_slave_configure() handlers for
struct scsi_host_template to set QUEUE_FLAG_BIDI in the former.

Quoted 3 lines

So far this has been tested together with commit 1f8f3eda132c using XDWRITE_READ_10 via BSG userspace code on a v2.6.36-rc3 kernel.

I'll start the review with this patch. Then we'll move on to the bigger harder one.

Quoted 18 lines

Signed-off-by Nicholas A. Bellinger <[email protected]>

---
 drivers/target/tcm_loop/tcm_loop_fabric_scsi.c |   48 ++++++++++++++++++------
 1 files changed, 36 insertions(+), 12 deletions(-)

drivers/target/tcm_loop/tcm_loop_fabric_scsi.c +1 -0

index 644f978..c4139d8 100644
@@ -64,6 +64,7 @@ static struct se_cmd *tcm_loop_allocate_core_cmd(
 	struct tcm_loop_nexus *tl_nexus = tl_hba->tl_nexus;
 	struct tcm_loop_cmd *tl_cmd;
 	int sam_task_attr;
+	enum dma_data_direction data_direction;
 
 	if (!(tl_nexus)) {
 		scmd_printk(KERN_ERR, sc, "TCM_Loop I_T Nexus"

drivers/target/tcm_loop/tcm_loop_fabric_scsi.c static struct se_cmd *tcm_loop_allocate_core_cmd( · line 91

Quoted 10 lines

drivers/target/tcm_loop/tcm_loop_fabric_scsi.c +7 -0

@@ -81,6 +82,13 @@ static struct se_cmd *tcm_loop_allocate_core_cmd(
 	 * Save the pointer to struct scsi_cmnd *sc
 	 */
 	tl_cmd->sc = sc;
+	/*
+	 * Check for scsi_bidi_cmnd() to signal TCM Core for DMA_BIDIRECTIONAL
+	 */
+	if (scsi_bidi_cmnd(sc))
+		data_direction = DMA_BIDIRECTIONAL;
+	else
+		data_direction = sc->sc_data_direction;

So you noticed that scsi-ml has a data_direction = DMA_TO_DEVICE and scsi_bidi_cmnd(sc) == true. I think you could do the same and code will be more simple, see below.

So for start this here is just droped

drivers/target/tcm_loop/tcm_loop_fabric_scsi.c static struct se_cmd *tcm_loop_allocate_core_cmd( · line 116

Quoted 9 lines

drivers/target/tcm_loop/tcm_loop_fabric_scsi.c +1 -1

 	/*
 	 * Locate the SAM Task Attr from struct scsi_cmnd *

@@ -105,7 +113,7 @@ static struct se_cmd *tcm_loop_allocate_core_cmd(
 	 * Initialize struct se_cmd descriptor from target_core_mod infrastructure
 	 */
 	transport_init_se_cmd(se_cmd, se_tpg->se_tpg_tfo, se_sess,
-			scsi_bufflen(sc), sc->sc_data_direction, sam_task_attr,
+			scsi_bufflen(sc), data_direction, sam_task_attr,

And this is dropped

Quoted 23 lines

drivers/target/tcm_loop/tcm_loop_fabric_scsi.c +4 -2

 			&tl_cmd->tl_sense_buf[0]);
 	/*
 	 * Locate the struct se_lun pointer and attach it to struct se_cmd

@@ -128,14 +136,16 @@ static struct se_cmd *tcm_loop_allocate_core_cmd(
 /*
  * Called by struct target_core_fabric_ops->new_cmd_map()
  *
- * Always called in process context
+ * Always called in process context.  A non zero return value
+ * here will signal to handle an exception based on the return code.
  */
 int tcm_loop_new_cmd_map(struct se_cmd *se_cmd)
 {
 	struct tcm_loop_cmd *tl_cmd = container_of(se_cmd,
 				struct tcm_loop_cmd, tl_se_cmd);
 	struct scsi_cmnd *sc = tl_cmd->sc;
-	void *mem_ptr;
+	void *mem_ptr, *mem_bidi_ptr = NULL;
+	u32 sg_no_bidi = 0;
 	int ret;
 	/*
 	 * Allocate the necessary tasks to complete the received CDB+data

drivers/target/tcm_loop/tcm_loop_fabric_scsi.c int tcm_loop_new_cmd_map(struct se_cmd *se_cmd) · line 180

Quoted 8 lines

drivers/target/tcm_loop/tcm_loop_fabric_scsi.c +5 -0

@@ -163,6 +173,16 @@ int tcm_loop_new_cmd_map(struct se_cmd *se_cmd)
 	if (scsi_sg_count(sc)) {
 		se_cmd->se_cmd_flags |= SCF_PASSTHROUGH_SG_TO_MEM;
 		mem_ptr = (void *)scsi_sglist(sc);
+		/*
+		 * For BIDI commands, pass in the extra READ buffer
+		 * to transport_generic_map_mem_to_cmd() below..
+		 */
+		if (se_cmd->data_direction == DMA_BIDIRECTIONAL) {

Just change to if (scsi_bidi_cmnd(sc))
[remember data_direction == DMA_TO_DEVICE]

drivers/target/tcm_loop/tcm_loop_fabric_scsi.c int tcm_loop_new_cmd_map(struct se_cmd *se_cmd) · line 184

Quoted 4 lines

drivers/target/tcm_loop/tcm_loop_fabric_scsi.c +4 -0

+			struct scsi_data_buffer *sdb = scsi_in(sc);
+
+			mem_bidi_ptr = (void *)sdb->table.sgl;
+			sg_no_bidi = sdb->table.nents;

This is the all bidi secret the presence of the scsi_in buffer

Quoted 21 lines

drivers/target/tcm_loop/tcm_loop_fabric_scsi.c +1 -9

+		}
 	} else {
 		/*
 		 * Used for DMA_NONE

@@ -322,15 +342,6 @@ static int tcm_loop_queuecommand(
 	}
 	tl_tpg = &tl_hba->tl_hba_tpgs[sc->device->id];transport_get_task
 	se_tpg = &tl_tpg->tl_se_tpg;
-
-	if (sc->sc_data_direction == DMA_BIDIRECTIONAL) {
-		spin_lock_irq(host->host_lock);
-		printk(KERN_ERR "Unsupported BIDI sc->sc_data_direction: %d\n",
-			sc->sc_data_direction);	
-		sc->result = host_byte(DID_ERROR);
-		(*done)(sc);
-		return 0;
-	}
 	/*
 	 * Determine the SAM Task Attribute and allocate tl_cmd and
 	 * tl_cmd->tl_se_cmd from TCM infrastructure

drivers/target/tcm_loop/tcm_loop_fabric_scsi.c release: · line 462

Quoted 6 lines

drivers/target/tcm_loop/tcm_loop_fabric_scsi.c +3 -0

@@ -446,6 +457,17 @@ release:
 	return ret;
 }
 
+static int tcm_loop_slave_alloc(struct scsi_device *sd)
+{
+	set_bit(QUEUE_FLAG_BIDI, &sd->request_queue->queue_flags);

Thank got, another new one. We are up to 4

Quoted 12 lines

drivers/target/tcm_loop/tcm_loop_fabric_scsi.c +8 -0

+	return 0;
+}
+
+static int tcm_loop_slave_configure(struct scsi_device *sd)
+{
+	return 0;
+}
+
 static struct scsi_host_template tcm_loop_driver_template = {
 	.proc_info		= tcm_loop_proc_info,
 	.proc_name		= "tcm_loopback",

drivers/target/tcm_loop/tcm_loop_fabric_scsi.c static struct scsi_host_template tcm_loop_driver_template = { · line 497

Quoted 8 lines

drivers/target/tcm_loop/tcm_loop_fabric_scsi.c +2 -0

@@ -468,6 +490,8 @@ static struct scsi_host_template tcm_loop_driver_template = {
 	.cmd_per_lun		= TL_SCSI_CMD_PER_LUN,
 	.max_sectors		= TL_SCSI_MAX_SECTORS,
 	.use_clustering		= DISABLE_CLUSTERING,
+	.slave_alloc		= tcm_loop_slave_alloc,
+	.slave_configure	= tcm_loop_slave_configure,
 	.module			= THIS_MODULE,
 };

Boaz

Nicholas A. Bellinger

author

2010-09-15 20:02 UTC #2 lore raw

Quoted 52 lines · Boaz Harrosh

Quoted 12 lines · Nicholas A. Bellinger
From: Nicholas Bellinger <[email protected]>
This patch series adds DMA_BIDIRECTIONAL CDB to the TCM_Loop Virtual SCSI LLD module. This includes the following changes:
*) Add call to include/scsi/scsi_cmnd.h:scsi_bidi_cmnd() to set
TCM struct se_cmd internal data_direction in tcm_loop_allocate_core_cmd()
*) Update tcm_loop_new_cmd_map() to handle BIDI READ payload from scsi_in() *) Drop DMA_BIDIRECTIONAL usage check from tcm_loop_queuecommand() *) Add tcm_loop_slave_alloc() and tcm_loop_slave_configure() handlers for
struct scsi_host_template to set QUEUE_FLAG_BIDI in the former.
Quoted 3 lines
So far this has been tested together with commit 1f8f3eda132c using XDWRITE_READ_10 via BSG userspace code on a v2.6.36-rc3 kernel.
I'll start the review with this patch. Then we'll move on to the bigger harder one.
Quoted 18 lines
Signed-off-by Nicholas A. Bellinger <[email protected]>
---
 drivers/target/tcm_loop/tcm_loop_fabric_scsi.c |   48 ++++++++++++++++++------
 1 files changed, 36 insertions(+), 12 deletions(-)
drivers/target/tcm_loop/tcm_loop_fabric_scsi.c +1 -0
index 644f978..c4139d8 100644
@@ -64,6 +64,7 @@ static struct se_cmd *tcm_loop_allocate_core_cmd(
 	struct tcm_loop_nexus *tl_nexus = tl_hba->tl_nexus;
 	struct tcm_loop_cmd *tl_cmd;
 	int sam_task_attr;
+	enum dma_data_direction data_direction;
 
 	if (!(tl_nexus)) {
 		scmd_printk(KERN_ERR, sc, "TCM_Loop I_T Nexus"
drivers/target/tcm_loop/tcm_loop_fabric_scsi.c static struct se_cmd *tcm_loop_allocate_core_cmd( · line 91
Quoted 10 lines
drivers/target/tcm_loop/tcm_loop_fabric_scsi.c +7 -0
@@ -81,6 +82,13 @@ static struct se_cmd *tcm_loop_allocate_core_cmd(
 	 * Save the pointer to struct scsi_cmnd *sc
 	 */
 	tl_cmd->sc = sc;
+	/*
+	 * Check for scsi_bidi_cmnd() to signal TCM Core for DMA_BIDIRECTIONAL
+	 */
+	if (scsi_bidi_cmnd(sc))
+		data_direction = 
+	else
+		data_direction = sc->sc_data_direction;
So you noticed that scsi-ml has a data_direction = DMA_TO_DEVICE and scsi_bidi_cmnd(sc) == true. I think you could do the same and code will be more simple, see below.

Quoted 1 line

So for start this here is just droped

Hi Boaz,

Thanks for the previous emails wrt to your comments about DMA_BIDIRECTIONAL and actual BIDI-COMMAND usage. This definately clarifies things for me now, and I will go ahead and drop the incorrect DMA_BIDIRECTIONAL here.

Quoted 12 lines

drivers/target/tcm_loop/tcm_loop_fabric_scsi.c static struct se_cmd *tcm_loop_allocate_core_cmd( · line 116
Quoted 10 lines
drivers/target/tcm_loop/tcm_loop_fabric_scsi.c +1 -1
 	/*
 	 * Locate the SAM Task Attr from struct scsi_cmnd *
@@ -105,7 +113,7 @@ static struct se_cmd *tcm_loop_allocate_core_cmd(
 	 * Initialize struct se_cmd descriptor from target_core_mod infrastructure
 	 */
 	transport_init_se_cmd(se_cmd, se_tpg->se_tpg_tfo, se_sess,
-			scsi_bufflen(sc), sc->sc_data_direction, sam_task_attr,
+			scsi_bufflen(sc), data_direction, sam_task_attr,
And this is dropped

Ditto

Quoted 27 lines

drivers/target/tcm_loop/tcm_loop_fabric_scsi.c +4 -2

 			&tl_cmd->tl_sense_buf[0]);
 	/*
 	 * Locate the struct se_lun pointer and attach it to struct se_cmd

@@ -128,14 +136,16 @@ static struct se_cmd *tcm_loop_allocate_core_cmd(
 /*
  * Called by struct target_core_fabric_ops->new_cmd_map()
  *
- * Always called in process context
+ * Always called in process context.  A non zero return value
+ * here will signal to handle an exception based on the return code.
  */
 int tcm_loop_new_cmd_map(struct se_cmd *se_cmd)
 {
 	struct tcm_loop_cmd *tl_cmd = container_of(se_cmd,
 				struct tcm_loop_cmd, tl_se_cmd);
 	struct scsi_cmnd *sc = tl_cmd->sc;
-	void *mem_ptr;
+	void *mem_ptr, *mem_bidi_ptr = NULL;
+	u32 sg_no_bidi = 0;
 	int ret;
 	/*
 	 * Allocate the necessary tasks to complete the received CDB+data

@@ -163,6 +173,16 @@ int tcm_loop_new_cmd_map(struct se_cmd *se_cmd)
 	if (scsi_sg_count(sc)) {
 		se_cmd->se_cmd_flags |= SCF_PASSTHROUGH_SG_TO_MEM;
 		mem_ptr = (void *)scsi_sglist(sc);

Quoted 8 lines

drivers/target/tcm_loop/tcm_loop_fabric_scsi.c int tcm_loop_new_cmd_map(struct se_cmd *se_cmd) · line 180
Quoted 5 lines
drivers/target/tcm_loop/tcm_loop_fabric_scsi.c +5 -0
+		/*
+		 * For BIDI commands, pass in the extra READ buffer
+		 * to transport_generic_map_mem_to_cmd() below..
+		 */
+		if (se_cmd->data_direction == DMA_BIDIRECTIONAL) {
Just change to if (scsi_bidi_cmnd(sc))
[remember data_direction == DMA_TO_DEVICE]

<nod>

Quoted 7 lines

drivers/target/tcm_loop/tcm_loop_fabric_scsi.c int tcm_loop_new_cmd_map(struct se_cmd *se_cmd) · line 184
Quoted 5 lines
drivers/target/tcm_loop/tcm_loop_fabric_scsi.c +4 -0
+			struct scsi_data_buffer *sdb = scsi_in(sc);
+
+			mem_bidi_ptr = (void *)sdb->table.sgl;
+			sg_no_bidi = sdb->table.nents;
This is the all bidi secret the presence of the scsi_in buffer

Makes sense..

Quoted 25 lines

drivers/target/tcm_loop/tcm_loop_fabric_scsi.c +1 -9

+		}
 	} else {
 		/*
 		 * Used for DMA_NONE

@@ -322,15 +342,6 @@ static int tcm_loop_queuecommand(
 	}
 	tl_tpg = &tl_hba->tl_hba_tpgs[sc->device->id];transport_get_task
 	se_tpg = &tl_tpg->tl_se_tpg;
-
-	if (sc->sc_data_direction == DMA_BIDIRECTIONAL) {
-		spin_lock_irq(host->host_lock);
-		printk(KERN_ERR "Unsupported BIDI sc->sc_data_direction: %d\n",
-			sc->sc_data_direction);	
-		sc->result = host_byte(DID_ERROR);
-		(*done)(sc);
-		return 0;
-	}
 	/*
 	 * Determine the SAM Task Attribute and allocate tl_cmd and
 	 * tl_cmd->tl_se_cmd from TCM infrastructure

@@ -446,6 +457,17 @@ release:
 	return ret;
 }

Quoted 5 lines

drivers/target/tcm_loop/tcm_loop_fabric_scsi.c release: · line 462
Quoted 3 lines
drivers/target/tcm_loop/tcm_loop_fabric_scsi.c +3 -0
+static int tcm_loop_slave_alloc(struct scsi_device *sd)
+{
+	set_bit(QUEUE_FLAG_BIDI, &sd->request_queue->queue_flags);
Thank got, another new one. We are up to 4

Perfect, I will make these changes to TCM_Loop and follow up on the larger patch in a moment.

Thanks!

--nab