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@xxxxxxxxxxxxxxx>
> >
> > 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@xxxxxxxxxxxxxxx>
> > ---
> > 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
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
