* [PATCH 2/2] tcm_loop: Add DMA_BIDIRECTIONAL CDB support
@ 2010-09-15 12:06 Nicholas A. Bellinger
2010-09-15 14:08 ` Boaz Harrosh
0 siblings, 1 reply; 3+ messages in thread
From: Nicholas A. Bellinger @ 2010-09-15 12:06 UTC (permalink / raw)
To: linux-scsi, linux-kernel, Boaz Harrosh, FUJITA Tomonori
Cc: Mike Christie, Hannes Reinecke, James Bottomley,
Konrad Rzeszutek Wilk, Douglas Gilbert, Joe Eykholt,
Nicholas Bellinger
From: Nicholas Bellinger <nab@linux-iscsi.org>
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 <nab@linux-iscsi.org>
---
drivers/target/tcm_loop/tcm_loop_fabric_scsi.c | 48 ++++++++++++++++++------
1 files changed, 36 insertions(+), 12 deletions(-)
diff --git a/drivers/target/tcm_loop/tcm_loop_fabric_scsi.c b/drivers/target/tcm_loop/tcm_loop_fabric_scsi.c
index 644f978..c4139d8 100644
--- a/drivers/target/tcm_loop/tcm_loop_fabric_scsi.c
+++ b/drivers/target/tcm_loop/tcm_loop_fabric_scsi.c
@@ -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,
};
--
1.5.6.5
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH 2/2] tcm_loop: Add DMA_BIDIRECTIONAL CDB support
2010-09-15 12:06 [PATCH 2/2] tcm_loop: Add DMA_BIDIRECTIONAL CDB support Nicholas A. Bellinger
@ 2010-09-15 14:08 ` Boaz Harrosh
2010-09-15 20:02 ` Nicholas A. Bellinger
0 siblings, 1 reply; 3+ messages in thread
From: Boaz Harrosh @ 2010-09-15 14:08 UTC (permalink / raw)
To: Nicholas A. Bellinger
Cc: linux-scsi, linux-kernel, FUJITA Tomonori, Mike Christie,
Hannes Reinecke, James Bottomley, Konrad Rzeszutek Wilk,
Douglas Gilbert, Joe Eykholt
On 09/15/2010 02:06 PM, Nicholas A. Bellinger wrote:
> From: Nicholas Bellinger <nab@linux-iscsi.org>
>
> 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.
>
I'll start the review with this patch. Then we'll move on to the bigger harder one.
> Signed-off-by: Nicholas A. Bellinger <nab@linux-iscsi.org>
> ---
> drivers/target/tcm_loop/tcm_loop_fabric_scsi.c | 48 ++++++++++++++++++------
> 1 files changed, 36 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/target/tcm_loop/tcm_loop_fabric_scsi.c b/drivers/target/tcm_loop/tcm_loop_fabric_scsi.c
> index 644f978..c4139d8 100644
> --- a/drivers/target/tcm_loop/tcm_loop_fabric_scsi.c
> +++ b/drivers/target/tcm_loop/tcm_loop_fabric_scsi.c
> @@ -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;
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
>
> /*
> * 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
> &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) {
Just change to if (scsi_bidi_cmnd(sc))
[remember data_direction == DMA_TO_DEVICE]
> + 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
> + }
> } 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;
> }
>
> +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
> + 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,
> };
>
Boaz
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH 2/2] tcm_loop: Add DMA_BIDIRECTIONAL CDB support
2010-09-15 14:08 ` Boaz Harrosh
@ 2010-09-15 20:02 ` Nicholas A. Bellinger
0 siblings, 0 replies; 3+ messages in thread
From: Nicholas A. Bellinger @ 2010-09-15 20:02 UTC (permalink / raw)
To: Boaz Harrosh
Cc: linux-scsi, linux-kernel, FUJITA Tomonori, Mike Christie,
Hannes Reinecke, James Bottomley, Konrad Rzeszutek Wilk,
Douglas Gilbert, Joe Eykholt
On Wed, 2010-09-15 at 16:08 +0200, Boaz Harrosh wrote:
> On 09/15/2010 02:06 PM, Nicholas A. Bellinger wrote:
> > From: Nicholas Bellinger <nab@linux-iscsi.org>
> >
> > 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.
> >
>
> I'll start the review with this patch. Then we'll move on to the bigger harder one.
>
> > Signed-off-by: Nicholas A. Bellinger <nab@linux-iscsi.org>
> > ---
> > drivers/target/tcm_loop/tcm_loop_fabric_scsi.c | 48 ++++++++++++++++++------
> > 1 files changed, 36 insertions(+), 12 deletions(-)
> >
> > diff --git a/drivers/target/tcm_loop/tcm_loop_fabric_scsi.c b/drivers/target/tcm_loop/tcm_loop_fabric_scsi.c
> > index 644f978..c4139d8 100644
> > --- a/drivers/target/tcm_loop/tcm_loop_fabric_scsi.c
> > +++ b/drivers/target/tcm_loop/tcm_loop_fabric_scsi.c
> > @@ -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 =
> > + 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
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.
>
> >
> > /*
> > * 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
>
> > &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) {
>
> Just change to if (scsi_bidi_cmnd(sc))
> [remember data_direction == DMA_TO_DEVICE]
<nod>
>
> > + 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..
>
> > + }
> > } 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;
> > }
> >
> > +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
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2010-09-15 20:02 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2010-09-15 12:06 [PATCH 2/2] tcm_loop: Add DMA_BIDIRECTIONAL CDB support Nicholas A. Bellinger
2010-09-15 14:08 ` Boaz Harrosh
2010-09-15 20:02 ` Nicholas A. Bellinger
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).