[PATCH 0/17] Clear up bidi command confusion

[PATCH 0/17] Clear up bidi command confusion

[Bart Van Assche]

There is some confusion in the SCSI core and in SCSI LLDs around the
meaning of sc_data_direction and whether or not this member can have the
value DMA_BIDIRECTIONAL. Clear up this confusion. The patches in this
series are:

0001-Clear-up-sc_data_direction-documentation.patch
0002-Clean-up-scsi_ioctl_reset.patch
0003-sg-Remove-an-unused-variable.patch
0004-dpt_i2o-Fix-bidi-command-test.patch
0005-aachba-Fix-bidi-command-test.patch
0006-eata-Remove-dead-code.patch
0007-sbp2-Fix-bidi-command-test.patch
0008-ibmvscsi-Fix-bidi-command-test.patch
0009-cciss-Remove-dead-code.patch
0010-hpsa-Remove-dead-code.patch
0011-3w-9xxx-Remove-dead-code.patch
0012-ncr53c8xx-Fix-bidi-command-support.patch
0013-qla1280-Fix-bidi-command-support.patch
0014-53c700-Remove-dead-code.patch
0015-mvumi-Remove-dead-code.patch
0016-sym_glue-Fix-bidi-command-test.patch
0017-IB-srp-Detect-bidi-commands-properly.patch

[PATCH 01/17] Clear up sc_data_direction documentation

[Bart Van Assche]

Signed-off-by: Bart Van Assche <bart.vanassche@sandisk.com>
---
 Documentation/DMA-API-HOWTO.txt         | 6 +++---
 Documentation/scsi/scsi_mid_low_api.txt | 5 +++--
 2 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/Documentation/DMA-API-HOWTO.txt b/Documentation/DMA-API-HOWTO.txt
index 0f7afb2..3999ec3 100644
--- a/Documentation/DMA-API-HOWTO.txt
+++ b/Documentation/DMA-API-HOWTO.txt
@@ -507,9 +507,9 @@ Only streaming mappings specify a direction, consistent mappings
 implicitly have a direction attribute setting of
 DMA_BIDIRECTIONAL.
 
-The SCSI subsystem tells you the direction to use in the
-'sc_data_direction' member of the SCSI command your driver is
-working on.
+Except for bidi commands, the SCSI subsystem tells you the direction to
+use in the 'sc_data_direction' member of the SCSI command your driver
+is working on.
 
 For Networking drivers, it's a rather simple affair.  For transmit
 packets, map/unmap them with the DMA_TO_DEVICE direction
diff --git a/Documentation/scsi/scsi_mid_low_api.txt b/Documentation/scsi/scsi_mid_low_api.txt
index 731bc4f..39cbbca 100644
--- a/Documentation/scsi/scsi_mid_low_api.txt
+++ b/Documentation/scsi/scsi_mid_low_api.txt
@@ -1259,8 +1259,9 @@ be at least one instance of struct scsi_cmnd available for each SCSI device.
 Members of interest:
     cmnd         - array containing SCSI command
     cmnd_len     - length (in bytes) of SCSI command
-    sc_data_direction - direction of data transfer in data phase. See
-                "enum dma_data_direction" in include/linux/dma-mapping.h
+    sc_data_direction - except for bidi commands, direction of data transfer
+                in data phase. See "enum dma_data_direction" in
+                include/linux/dma-mapping.h
     request_bufflen - number of data bytes to transfer (0 if no data phase)
     use_sg       - ==0 -> no scatter gather list, hence transfer data
                           to/from request_buffer
-- 
2.1.2

[PATCH 02/17] Clean up scsi_ioctl_reset()

[Bart Van Assche]

Since SG_SCSI_RESET causes a task management function to be sent
and since LLDs only check the sc_data_direction field inside the
queueucommand callback function, the sc_data_direction member
does not have to be set here. Hence remove the sc_data_direction
assignment.

Signed-off-by: Bart Van Assche <bart.vanassche@sandisk.com>
---
 drivers/scsi/scsi_error.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
index 8afb016..9ee4830 100644
--- a/drivers/scsi/scsi_error.c
+++ b/drivers/scsi/scsi_error.c
@@ -2346,8 +2346,6 @@ scsi_ioctl_reset(struct scsi_device *dev, int __user *arg)
 
 	scmd->cmd_len			= 0;
 
-	scmd->sc_data_direction		= DMA_BIDIRECTIONAL;
-
 	spin_lock_irqsave(shost->host_lock, flags);
 	shost->tmf_in_progress = 1;
 	spin_unlock_irqrestore(shost->host_lock, flags);
-- 
2.1.2

[PATCH 03/17] sg: Remove an unused variable

[Bart Van Assche]

Signed-off-by: Bart Van Assche <bart.vanassche@sandisk.com>
---
 drivers/scsi/sg.c | 17 +----------------
 1 file changed, 1 insertion(+), 16 deletions(-)

diff --git a/drivers/scsi/sg.c b/drivers/scsi/sg.c
index b14f64c..a668c88 100644
--- a/drivers/scsi/sg.c
+++ b/drivers/scsi/sg.c
@@ -763,7 +763,7 @@ static int
 sg_common_write(Sg_fd * sfp, Sg_request * srp,
 		unsigned char *cmnd, int timeout, int blocking)
 {
-	int k, data_dir, at_head;
+	int k, at_head;
 	Sg_device *sdp = sfp->parentdp;
 	sg_io_hdr_t *hp = &srp->header;
 
@@ -793,21 +793,6 @@ sg_common_write(Sg_fd * sfp, Sg_request * srp,
 		return -ENODEV;
 	}
 
-	switch (hp->dxfer_direction) {
-	case SG_DXFER_TO_FROM_DEV:
-	case SG_DXFER_FROM_DEV:
-		data_dir = DMA_FROM_DEVICE;
-		break;
-	case SG_DXFER_TO_DEV:
-		data_dir = DMA_TO_DEVICE;
-		break;
-	case SG_DXFER_UNKNOWN:
-		data_dir = DMA_BIDIRECTIONAL;
-		break;
-	default:
-		data_dir = DMA_NONE;
-		break;
-	}
 	hp->duration = jiffies_to_msecs(jiffies);
 	if (hp->interface_id != '\0' &&	/* v3 (or later) interface */
 	    (SG_FLAG_Q_AT_TAIL & hp->flags))
-- 
2.1.2

Re: [PATCH 03/17] sg: Remove an unused variable

[Douglas Gilbert]

On 15-01-23 01:07 PM, Bart Van Assche wrote:
> Signed-off-by: Bart Van Assche <bart.vanassche@sandisk.com>

Acked-by: Douglas Gilbert <dgilbert@interlog.com>

> ---
>   drivers/scsi/sg.c | 17 +----------------
>   1 file changed, 1 insertion(+), 16 deletions(-)
>
> diff --git a/drivers/scsi/sg.c b/drivers/scsi/sg.c
> index b14f64c..a668c88 100644
> --- a/drivers/scsi/sg.c
> +++ b/drivers/scsi/sg.c
> @@ -763,7 +763,7 @@ static int
>   sg_common_write(Sg_fd * sfp, Sg_request * srp,
>   		unsigned char *cmnd, int timeout, int blocking)
>   {
> -	int k, data_dir, at_head;
> +	int k, at_head;
>   	Sg_device *sdp = sfp->parentdp;
>   	sg_io_hdr_t *hp = &srp->header;
>
> @@ -793,21 +793,6 @@ sg_common_write(Sg_fd * sfp, Sg_request * srp,
>   		return -ENODEV;
>   	}
>
> -	switch (hp->dxfer_direction) {
> -	case SG_DXFER_TO_FROM_DEV:
> -	case SG_DXFER_FROM_DEV:
> -		data_dir = DMA_FROM_DEVICE;
> -		break;
> -	case SG_DXFER_TO_DEV:
> -		data_dir = DMA_TO_DEVICE;
> -		break;
> -	case SG_DXFER_UNKNOWN:
> -		data_dir = DMA_BIDIRECTIONAL;
> -		break;
> -	default:
> -		data_dir = DMA_NONE;
> -		break;
> -	}
>   	hp->duration = jiffies_to_msecs(jiffies);
>   	if (hp->interface_id != '\0' &&	/* v3 (or later) interface */
>   	    (SG_FLAG_Q_AT_TAIL & hp->flags))
>

[PATCH 04/17] dpt_i2o: Fix bidi command test

[Bart Van Assche]

Signed-off-by: Bart Van Assche <bart.vanassche@sandisk.com>
Cc: Adaptec OEM Raid Solutions <aacraid@adaptec.com>
---
 drivers/scsi/dpt_i2o.c | 21 ++++++---------------
 1 file changed, 6 insertions(+), 15 deletions(-)

diff --git a/drivers/scsi/dpt_i2o.c b/drivers/scsi/dpt_i2o.c
index 0bf9769..9bab86c 100644
--- a/drivers/scsi/dpt_i2o.c
+++ b/drivers/scsi/dpt_i2o.c
@@ -2240,26 +2240,17 @@ static s32 adpt_scsi_to_i2o(adpt_hba* pHba, struct scsi_cmnd* cmd, struct adpt_d
 		 * Note:  Do not have to verify index is less than 0 since
 		 * cmd->cmnd[0] is an unsigned char
 		 */
-		switch(cmd->sc_data_direction){
-		case DMA_FROM_DEVICE:
-			scsidir  =0x40000000;	// DATA IN  (iop<--dev)
-			break;
-		case DMA_TO_DEVICE:
-			direction=0x04000000;	// SGL OUT
-			scsidir  =0x80000000;	// DATA OUT (iop-->dev)
-			break;
-		case DMA_NONE:
-			break;
-		case DMA_BIDIRECTIONAL:
-			scsidir  =0x40000000;	// DATA IN  (iop<--dev)
-			// Assume In - and continue;
-			break;
-		default:
+		if (scsi_bidi_cmnd(cmd)) {
 			printk(KERN_WARNING"%s: scsi opcode 0x%x not supported.\n",
 			     pHba->name, cmd->cmnd[0]);
 			cmd->result = (DID_OK <<16) | (INITIATOR_ERROR << 8);
 			cmd->scsi_done(cmd);
 			return 	0;
+		} else if (cmd->sc_data_direction == DMA_FROM_DEVICE) {
+			scsidir  =0x40000000;	// DATA IN  (iop<--dev)
+		} else if (cmd->sc_data_direction == DMA_TO_DEVICE) {
+			direction=0x04000000;	// SGL OUT
+			scsidir  =0x80000000;	// DATA OUT (iop-->dev)
 		}
 	}
 	// msg[0] is set later
-- 
2.1.2

[PATCH 05/17] aachba: Fix bidi command test

[Bart Van Assche]

Signed-off-by: Bart Van Assche <bart.vanassche@sandisk.com>
---
 drivers/scsi/aacraid/aachba.c | 17 +++++------------
 1 file changed, 5 insertions(+), 12 deletions(-)

diff --git a/drivers/scsi/aacraid/aachba.c b/drivers/scsi/aacraid/aachba.c
index b32e77d..5ade84a 100644
--- a/drivers/scsi/aacraid/aachba.c
+++ b/drivers/scsi/aacraid/aachba.c
@@ -1228,21 +1228,14 @@ static struct aac_srb * aac_scsi_common(struct fib * fib, struct scsi_cmnd * cmd
 	u32 timeout;
 
 	aac_fib_init(fib);
-	switch(cmd->sc_data_direction){
-	case DMA_TO_DEVICE:
-		flag = SRB_DataOut;
-		break;
-	case DMA_BIDIRECTIONAL:
+	if (scsi_bidi_cmnd(cmd))
 		flag = SRB_DataIn | SRB_DataOut;
-		break;
-	case DMA_FROM_DEVICE:
+	else if (cmd->sc_data_direction == DMA_TO_DEVICE)
+		flag = SRB_DataOut;
+	else if (cmd->sc_data_direction == DMA_FROM_DEVICE)
 		flag = SRB_DataIn;
-		break;
-	case DMA_NONE:
-	default:	/* shuts up some versions of gcc */
+	else
 		flag = SRB_NoDataXfer;
-		break;
-	}
 
 	srbcmd = (struct aac_srb*) fib_data(fib);
 	srbcmd->function = cpu_to_le32(SRBF_ExecuteScsi);
-- 
2.1.2

[PATCH 06/17] eata: Remove dead code

[Bart Van Assche]

The SCSI core never sets sc_data_direction to DMA_BIDIRECTIONAL.
Hence remove the code that tests for this value.

Signed-off-by: Bart Van Assche <bart.vanassche@sandisk.com>
Cc: Dario Ballabio <ballabio_dario@emc.com>
---
 drivers/scsi/eata.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/scsi/eata.c b/drivers/scsi/eata.c
index 227dd2c..c1fa5ba 100644
--- a/drivers/scsi/eata.c
+++ b/drivers/scsi/eata.c
@@ -1726,9 +1726,8 @@ static void scsi_to_dev_dir(unsigned int i, struct hostdata *ha)
 		return;
 	}
 
-	if (SCpnt->sc_data_direction != DMA_BIDIRECTIONAL)
-		panic("%s: qcomm, invalid SCpnt->sc_data_direction.\n",
-				ha->board_name);
+	panic("%s: qcomm, invalid SCpnt->sc_data_direction %d.\n",
+	      ha->board_name, SCpnt->sc_data_direction);
 
 	for (k = 0; k < ARRAY_SIZE(data_out_cmds); k++)
 		if (SCpnt->cmnd[0] == data_out_cmds[k]) {
-- 
2.1.2

[PATCH 07/17] sbp2: Fix bidi command test

[Bart Van Assche]

The SCSI core never sets sc_data_direction to DMA_BIDIRECTIONAL.

Signed-off-by: Bart Van Assche <bart.vanassche@sandisk.com>
Cc: Stefan Richter <stefanr@s5r6.in-berlin.de>
---
 drivers/firewire/sbp2.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/firewire/sbp2.c b/drivers/firewire/sbp2.c
index 64ac8f8..ab1b114 100644
--- a/drivers/firewire/sbp2.c
+++ b/drivers/firewire/sbp2.c
@@ -1467,7 +1467,7 @@ static int sbp2_scsi_queuecommand(struct Scsi_Host *shost,
 	 * Bidirectional commands are not yet implemented, and unknown
 	 * transfer direction not handled.
 	 */
-	if (cmd->sc_data_direction == DMA_BIDIRECTIONAL) {
+	if (scsi_bidi_cmnd(cmd)) {
 		dev_err(lu_dev(lu), "cannot handle bidirectional command\n");
 		cmd->result = DID_ERROR << 16;
 		cmd->scsi_done(cmd);
-- 
2.1.2

Re: [PATCH 07/17] sbp2: Fix bidi command test

[Stefan Richter]

On Jan 23 Bart Van Assche wrote:
> The SCSI core never sets sc_data_direction to DMA_BIDIRECTIONAL.
> 
> Signed-off-by: Bart Van Assche <bart.vanassche@sandisk.com>
> Cc: Stefan Richter <stefanr@s5r6.in-berlin.de>
> ---
>  drivers/firewire/sbp2.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/firewire/sbp2.c b/drivers/firewire/sbp2.c
> index 64ac8f8..ab1b114 100644
> --- a/drivers/firewire/sbp2.c
> +++ b/drivers/firewire/sbp2.c
> @@ -1467,7 +1467,7 @@ static int sbp2_scsi_queuecommand(struct Scsi_Host *shost,
>  	 * Bidirectional commands are not yet implemented, and unknown
>  	 * transfer direction not handled.
>  	 */
> -	if (cmd->sc_data_direction == DMA_BIDIRECTIONAL) {
> +	if (scsi_bidi_cmnd(cmd)) {
>  		dev_err(lu_dev(lu), "cannot handle bidirectional command\n");
>  		cmd->result = DID_ERROR << 16;
>  		cmd->scsi_done(cmd);

---- 8< ----

Subject: [PATCH] firewire: sbp2: remove redundant check for bidi command

[Bart van Asche:]  SCSI core never sets cmd->sc_data_direction to
DMA_BIDIRECTIONAL, scsi_bidi_cmnd(cmd) should be used instead.

[Christoph Hellwig:]  Bidirectional commands won't ever be queued
anyway, unless a LLD or transport driver sets QUEUE_FLAG_BIDI.

So, simply remove the respective queuecommand check in the SBP-2
transport driver.

Signed-off-by: Stefan Richter <stefanr@s5r6.in-berlin.de>
---
 drivers/firewire/sbp2.c |   11 -----------
 1 file changed, 11 deletions(-)

--- a/drivers/firewire/sbp2.c
+++ b/drivers/firewire/sbp2.c
@@ -1463,17 +1463,6 @@ static int sbp2_scsi_queuecommand(struct
 	struct sbp2_command_orb *orb;
 	int generation, retval = SCSI_MLQUEUE_HOST_BUSY;
 
-	/*
-	 * Bidirectional commands are not yet implemented, and unknown
-	 * transfer direction not handled.
-	 */
-	if (cmd->sc_data_direction == DMA_BIDIRECTIONAL) {
-		dev_err(lu_dev(lu), "cannot handle bidirectional command\n");
-		cmd->result = DID_ERROR << 16;
-		cmd->scsi_done(cmd);
-		return 0;
-	}
-
 	orb = kzalloc(sizeof(*orb), GFP_ATOMIC);
 	if (orb == NULL)
 		return SCSI_MLQUEUE_HOST_BUSY;


-- 
Stefan Richter
-=====-===== ---= =-===
http://arcgraph.de/sr/

------------------------------------------------------------------------------
New Year. New Location. New Benefits. New Data Center in Ashburn, VA.
GigeNET is offering a free month of service with a new server in Ashburn.
Choose from 2 high performing configs, both with 100TB of bandwidth.
Higher redundancy.Lower latency.Increased capacity.Completely compliant.
http://p.sf.net/sfu/gigenet

[PATCH 08/17] ibmvscsi: Fix bidi command test

[Bart Van Assche]

Signed-off-by: Bart Van Assche <bart.vanassche@sandisk.com>
Cc: Brian King <brking@linux.vnet.ibm.com>
Cc: Nathan Fontenot <nfont@linux.vnet.ibm.com>
---
 drivers/scsi/ibmvscsi/ibmvscsi.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/scsi/ibmvscsi/ibmvscsi.c b/drivers/scsi/ibmvscsi/ibmvscsi.c
index acea5d6..cf26b33 100644
--- a/drivers/scsi/ibmvscsi/ibmvscsi.c
+++ b/drivers/scsi/ibmvscsi/ibmvscsi.c
@@ -765,16 +765,18 @@ static int map_data_for_srp_cmd(struct scsi_cmnd *cmd,
 				struct srp_event_struct *evt_struct,
 				struct srp_cmd *srp_cmd, struct device *dev)
 {
+	if (scsi_bidi_cmnd(cmd)) {
+		sdev_printk(KERN_ERR, cmd->device,
+			    "Bidirectional commands are not yet supported\n");
+		return 0;
+	}
+
 	switch (cmd->sc_data_direction) {
 	case DMA_FROM_DEVICE:
 	case DMA_TO_DEVICE:
 		break;
 	case DMA_NONE:
 		return 1;
-	case DMA_BIDIRECTIONAL:
-		sdev_printk(KERN_ERR, cmd->device,
-			    "Can't map DMA_BIDIRECTIONAL to read/write\n");
-		return 0;
 	default:
 		sdev_printk(KERN_ERR, cmd->device,
 			    "Unknown data direction 0x%02x; can't map!\n",
-- 
2.1.2

Re: [PATCH 08/17] ibmvscsi: Fix bidi command test

[Tyrel Datwyler]

On 01/23/2015 04:10 AM, Bart Van Assche wrote:
> Signed-off-by: Bart Van Assche <bart.vanassche@sandisk.com>
> Cc: Brian King <brking@linux.vnet.ibm.com>
> Cc: Nathan Fontenot <nfont@linux.vnet.ibm.com>
> ---
>  drivers/scsi/ibmvscsi/ibmvscsi.c | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/scsi/ibmvscsi/ibmvscsi.c b/drivers/scsi/ibmvscsi/ibmvscsi.c
> index acea5d6..cf26b33 100644
> --- a/drivers/scsi/ibmvscsi/ibmvscsi.c
> +++ b/drivers/scsi/ibmvscsi/ibmvscsi.c
> @@ -765,16 +765,18 @@ static int map_data_for_srp_cmd(struct scsi_cmnd *cmd,
>  				struct srp_event_struct *evt_struct,
>  				struct srp_cmd *srp_cmd, struct device *dev)
>  {
> +	if (scsi_bidi_cmnd(cmd)) {
> +		sdev_printk(KERN_ERR, cmd->device,
> +			    "Bidirectional commands are not yet supported\n");
> +		return 0;
> +	}
> +

Is there a particular problem this solves, or is this simply a change to
use the bidi API in place of checking sc_data_direction for
DMA_BIDIRECTIONAL?

-Tyrel

>  	switch (cmd->sc_data_direction) {
>  	case DMA_FROM_DEVICE:
>  	case DMA_TO_DEVICE:
>  		break;
>  	case DMA_NONE:
>  		return 1;
> -	case DMA_BIDIRECTIONAL:
> -		sdev_printk(KERN_ERR, cmd->device,
> -			    "Can't map DMA_BIDIRECTIONAL to read/write\n");
> -		return 0;
>  	default:
>  		sdev_printk(KERN_ERR, cmd->device,
>  			    "Unknown data direction 0x%02x; can't map!\n",
> 

Re: [PATCH 08/17] ibmvscsi: Fix bidi command test

[Bart Van Assche]

On 01/27/15 23:11, Tyrel Datwyler wrote:
> On 01/23/2015 04:10 AM, Bart Van Assche wrote:
>> Signed-off-by: Bart Van Assche <bart.vanassche@sandisk.com>
>> Cc: Brian King <brking@linux.vnet.ibm.com>
>> Cc: Nathan Fontenot <nfont@linux.vnet.ibm.com>
>> ---
>>  drivers/scsi/ibmvscsi/ibmvscsi.c | 10 ++++++----
>>  1 file changed, 6 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/scsi/ibmvscsi/ibmvscsi.c b/drivers/scsi/ibmvscsi/ibmvscsi.c
>> index acea5d6..cf26b33 100644
>> --- a/drivers/scsi/ibmvscsi/ibmvscsi.c
>> +++ b/drivers/scsi/ibmvscsi/ibmvscsi.c
>> @@ -765,16 +765,18 @@ static int map_data_for_srp_cmd(struct scsi_cmnd *cmd,
>>  				struct srp_event_struct *evt_struct,
>>  				struct srp_cmd *srp_cmd, struct device *dev)
>>  {
>> +	if (scsi_bidi_cmnd(cmd)) {
>> +		sdev_printk(KERN_ERR, cmd->device,
>> +			    "Bidirectional commands are not yet supported\n");
>> +		return 0;
>> +	}
>> +
> 
> Is there a particular problem this solves, or is this simply a change to
> use the bidi API in place of checking sc_data_direction for
> DMA_BIDIRECTIONAL?
> 
> -Tyrel
> 
>>  	switch (cmd->sc_data_direction) {
>>  	case DMA_FROM_DEVICE:
>>  	case DMA_TO_DEVICE:
>>  		break;
>>  	case DMA_NONE:
>>  		return 1;
>> -	case DMA_BIDIRECTIONAL:
>> -		sdev_printk(KERN_ERR, cmd->device,
>> -			    "Can't map DMA_BIDIRECTIONAL to read/write\n");
>> -		return 0;
>>  	default:
>>  		sdev_printk(KERN_ERR, cmd->device,
>>  			    "Unknown data direction 0x%02x; can't map!\n",
>>

Hello Tyrel,

Since the ibmvscsi driver does not set QUEUE_FLAG_BIDI neither the code
under "case DMA_BIDIRECTIONAL" nor the code under "if
(scsi_bidi_cmnd(cmd))" is ever executed. The only reason why I proposed
this change is to avoid that an author of another SCSI LLD driver would
use the code that compares sc_data_direction with DMA_BIDIRECTIONAL as
an example.

BTW, had you already noticed that Christoph Hellwig proposed to modify
the SCSI core such that sc_data_direction is set to DMA_BIDIRECTIONAL
for bidi commands ? I'd like to rework this patch series accordingly.
With that approach no changes are necessary in the ibmvscsi driver.

Bart.

Re: [PATCH 08/17] ibmvscsi: Fix bidi command test

[Tyrel Datwyler]

On 01/27/2015 11:57 PM, Bart Van Assche wrote:

[snip]

> 
> Hello Tyrel,
> 
> Since the ibmvscsi driver does not set QUEUE_FLAG_BIDI neither the code
> under "case DMA_BIDIRECTIONAL" nor the code under "if
> (scsi_bidi_cmnd(cmd))" is ever executed. The only reason why I proposed
> this change is to avoid that an author of another SCSI LLD driver would
> use the code that compares sc_data_direction with DMA_BIDIRECTIONAL as
> an example.
> 
> BTW, had you already noticed that Christoph Hellwig proposed to modify
> the SCSI core such that sc_data_direction is set to DMA_BIDIRECTIONAL
> for bidi commands ? I'd like to rework this patch series accordingly.
> With that approach no changes are necessary in the ibmvscsi driver.
> 
> Bart.
> 
> 

I only saw the ibmvscsi patch on my way out of the office yesterday
because Brian King forwarded it to me. After looking at the full
patchset I now understand the intent. I did see Christoph's proposal and
agree with the logic of setting DMA_BIDIRECTIONAL for bidi commands.

-Tyrel

[PATCH 09/17] cciss: Remove dead code

[Bart Van Assche]

The code that could set sc_data_direction to DMA_BIDIRECTIONAL has
been removed eight years ago. Hence remove the code that handles
this case. See also patch "unify SCSI_IOCTL_SEND_COMMAND implementations"
(commit 21b2f0c803ad).

Signed-off-by: Bart Van Assche <bart.vanassche@sandisk.com>
Cc: Don Brace <don.brace@pmcs.com>
---
 drivers/block/cciss_scsi.c | 15 ---------------
 1 file changed, 15 deletions(-)

diff --git a/drivers/block/cciss_scsi.c b/drivers/block/cciss_scsi.c
index ecd845c..8f60a64 100644
--- a/drivers/block/cciss_scsi.c
+++ b/drivers/block/cciss_scsi.c
@@ -1478,21 +1478,6 @@ cciss_scsi_queue_command_lck(struct scsi_cmnd *cmd, void (*done)(struct scsi_cmn
 	  case DMA_NONE:
 		c->Request.Type.Direction = XFER_NONE;
 		break;
-	  case DMA_BIDIRECTIONAL:
-		// This can happen if a buggy application does a scsi passthru
-		// and sets both inlen and outlen to non-zero. ( see
-		// ../scsi/scsi_ioctl.c:scsi_ioctl_send_command() )
-
-		c->Request.Type.Direction = XFER_RSVD;
-		// This is technically wrong, and cciss controllers should
-		// reject it with CMD_INVALID, which is the most correct 
-		// response, but non-fibre backends appear to let it 
-		// slide by, and give the same results as if this field
-		// were set correctly.  Either way is acceptable for
-		// our purposes here.
-
-		break;
-
 	  default: 
 		dev_warn(&h->pdev->dev, "unknown data direction: %d\n",
 			cmd->sc_data_direction);
-- 
2.1.2

[PATCH 10/17] hpsa: Remove dead code

[Bart Van Assche]

The SCSI core never sets sc_data_direction to DMA_BIDIRECTIONAL.
Hence remove the code that tests for this value.

Signed-off-by: Bart Van Assche <bart.vanassche@sandisk.com>
Cc: Don Brace <don.brace@pmcs.com>
---
 drivers/scsi/hpsa.c | 18 ------------------
 1 file changed, 18 deletions(-)

diff --git a/drivers/scsi/hpsa.c b/drivers/scsi/hpsa.c
index 6bb4611..9d16a06 100644
--- a/drivers/scsi/hpsa.c
+++ b/drivers/scsi/hpsa.c
@@ -3969,24 +3969,6 @@ static int hpsa_scsi_queue_command(struct Scsi_Host *sh, struct scsi_cmnd *cmd)
 		c->Request.type_attr_dir =
 			TYPE_ATTR_DIR(TYPE_CMD, ATTR_SIMPLE, XFER_NONE);
 		break;
-	case DMA_BIDIRECTIONAL:
-		/* This can happen if a buggy application does a scsi passthru
-		 * and sets both inlen and outlen to non-zero. ( see
-		 * ../scsi/scsi_ioctl.c:scsi_ioctl_send_command() )
-		 */
-
-		c->Request.type_attr_dir =
-			TYPE_ATTR_DIR(TYPE_CMD, ATTR_SIMPLE, XFER_RSVD);
-		/* This is technically wrong, and hpsa controllers should
-		 * reject it with CMD_INVALID, which is the most correct
-		 * response, but non-fibre backends appear to let it
-		 * slide by, and give the same results as if this field
-		 * were set correctly.  Either way is acceptable for
-		 * our purposes here.
-		 */
-
-		break;
-
 	default:
 		dev_err(&h->pdev->dev, "unknown data direction: %d\n",
 			cmd->sc_data_direction);
-- 
2.1.2

[PATCH 11/17] 3w-9xxx: Remove dead code

[Bart Van Assche]

The SCSI core never sets sc_data_direction to DMA_BIDIRECTIONAL.
Hence remove the code that tests for this value.

Signed-off-by: Bart Van Assche <bart.vanassche@sandisk.com>
Cc: Adam Radford <linuxraid@lsi.com>
---
 drivers/scsi/3w-9xxx.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/scsi/3w-9xxx.c b/drivers/scsi/3w-9xxx.c
index cd4129f..e83e3b7 100644
--- a/drivers/scsi/3w-9xxx.c
+++ b/drivers/scsi/3w-9xxx.c
@@ -1854,8 +1854,7 @@ static int twa_scsiop_execute_scsi(TW_Device_Extension *tw_dev, int request_id,
 		if (scsi_sg_count(srb)) {
 			if ((scsi_sg_count(srb) == 1) &&
 			    (scsi_bufflen(srb) < TW_MIN_SGL_LENGTH)) {
-				if (srb->sc_data_direction == DMA_TO_DEVICE ||
-				    srb->sc_data_direction == DMA_BIDIRECTIONAL)
+				if (srb->sc_data_direction == DMA_TO_DEVICE)
 					scsi_sg_copy_to_buffer(srb,
 							       tw_dev->generic_buffer_virt[request_id],
 							       TW_SECTOR_SIZE);
@@ -1927,8 +1926,7 @@ static void twa_scsiop_execute_scsi_complete(TW_Device_Extension *tw_dev, int re
 	struct scsi_cmnd *cmd = tw_dev->srb[request_id];
 
 	if (scsi_bufflen(cmd) < TW_MIN_SGL_LENGTH &&
-	    (cmd->sc_data_direction == DMA_FROM_DEVICE ||
-	     cmd->sc_data_direction == DMA_BIDIRECTIONAL)) {
+	    cmd->sc_data_direction == DMA_FROM_DEVICE) {
 		if (scsi_sg_count(cmd) == 1) {
 			void *buf = tw_dev->generic_buffer_virt[request_id];
 
-- 
2.1.2

[PATCH 12/17] ncr53c8xx: Fix bidi command support

[Bart Van Assche]

Signed-off-by: Bart Van Assche <bart.vanassche@sandisk.com>
Cc: Finn Thain <fthain@telegraphics.com.au>
Cc: Michael Schmitz <schmitzmic@gmail.com>
---
 drivers/scsi/ncr53c8xx.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/scsi/ncr53c8xx.c b/drivers/scsi/ncr53c8xx.c
index 5b93ed8..b9c489a 100644
--- a/drivers/scsi/ncr53c8xx.c
+++ b/drivers/scsi/ncr53c8xx.c
@@ -4286,13 +4286,12 @@ static int ncr_queue_command (struct ncb *np, struct scsi_cmnd *cmd)
 		direction = DMA_NONE;
 
 	/*
-	**	If data direction is BIDIRECTIONAL, speculate FROM_DEVICE
+	**	For bidi commands, speculate FROM_DEVICE
 	**	but prepare alternate pointers for TO_DEVICE in case 
 	**	of our speculation will be just wrong.
 	**	SCRIPTS will swap values if needed.
 	*/
 	switch(direction) {
-	case DMA_BIDIRECTIONAL:
 	case DMA_TO_DEVICE:
 		goalp = NCB_SCRIPT_PHYS (np, data_out2) + 8;
 		if (segments <= MAX_SCATTERL)
@@ -4301,7 +4300,7 @@ static int ncr_queue_command (struct ncb *np, struct scsi_cmnd *cmd)
 			lastp = NCB_SCRIPTH_PHYS (np, hdata_out2);
 			lastp -= (segments - MAX_SCATTERL) * 16;
 		}
-		if (direction != DMA_BIDIRECTIONAL)
+		if (!scsi_bidi_cmd(cmd))
 			break;
 		cp->phys.header.wgoalp	= cpu_to_scr(goalp);
 		cp->phys.header.wlastp	= cpu_to_scr(lastp);
@@ -4328,7 +4327,7 @@ static int ncr_queue_command (struct ncb *np, struct scsi_cmnd *cmd)
 	cp->phys.header.lastp = cpu_to_scr(lastp);
 	cp->phys.header.goalp = cpu_to_scr(goalp);
 
-	if (direction == DMA_BIDIRECTIONAL)
+	if (scsi_bidi_cmd(cmd))
 		cp->phys.header.savep = 
 			cpu_to_scr(NCB_SCRIPTH_PHYS (np, data_io));
 	else
-- 
2.1.2

[PATCH 13/17] qla1280: Fix bidi command support

[Bart Van Assche]

The SCSI core never sets sc_data_direction to DMA_BIDIRECTIONAL.

Signed-off-by: Bart Van Assche <bart.vanassche@sandisk.com>
Cc: Michael Reed <mdr@sgi.com>
---
 drivers/scsi/qla1280.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/scsi/qla1280.c b/drivers/scsi/qla1280.c
index c68a66e..ede9563 100644
--- a/drivers/scsi/qla1280.c
+++ b/drivers/scsi/qla1280.c
@@ -468,13 +468,14 @@ static struct qla_driver_setup driver_setup;
 static inline uint16_t
 qla1280_data_direction(struct scsi_cmnd *cmnd)
 {
+	if (scsi_bidi_cmnd(cmnd))
+		return BIT_5 | BIT_6;
+
 	switch(cmnd->sc_data_direction) {
 	case DMA_FROM_DEVICE:
 		return BIT_5;
 	case DMA_TO_DEVICE:
 		return BIT_6;
-	case DMA_BIDIRECTIONAL:
-		return BIT_5 | BIT_6;
 	/*
 	 * We could BUG() on default here if one of the four cases aren't
 	 * met, but then again if we receive something like that from the
-- 
2.1.2

[PATCH 14/17] 53c700: Remove dead code

[Bart Van Assche]

The SCSI core never sets sc_data_direction to DMA_BIDIRECTIONAL.
Hence remove the code that tests for this value.

Signed-off-by: Bart Van Assche <bart.vanassche@sandisk.com>
---
 drivers/scsi/53c700.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/scsi/53c700.c b/drivers/scsi/53c700.c
index 82abfce..78f4b57 100644
--- a/drivers/scsi/53c700.c
+++ b/drivers/scsi/53c700.c
@@ -578,8 +578,7 @@ STATIC inline void
 NCR_700_unmap(struct NCR_700_Host_Parameters *hostdata, struct scsi_cmnd *SCp,
 	      struct NCR_700_command_slot *slot)
 {
-	if(SCp->sc_data_direction != DMA_NONE &&
-	   SCp->sc_data_direction != DMA_BIDIRECTIONAL)
+	if (SCp->sc_data_direction != DMA_NONE)
 		scsi_dma_unmap(SCp);
 }
 
@@ -1846,7 +1845,6 @@ NCR_700_queuecommand_lck(struct scsi_cmnd *SCp, void (*done)(struct scsi_cmnd *)
 	default:
 		/* OK, get it from the command */
 		switch(SCp->sc_data_direction) {
-		case DMA_BIDIRECTIONAL:
 		default:
 			printk(KERN_ERR "53c700: Unknown command for data direction ");
 			scsi_print_command(SCp);
-- 
2.1.2

[PATCH 15/17] mvumi: Remove dead code

[Bart Van Assche]

The SCSI core never sets sc_data_direction to DMA_BIDIRECTIONAL.
Hence remove the code that tests for this value.

Signed-off-by: Bart Van Assche <bart.vanassche@sandisk.com>
---
 drivers/scsi/mvumi.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/scsi/mvumi.c b/drivers/scsi/mvumi.c
index 3e6b866..eae3cb2 100644
--- a/drivers/scsi/mvumi.c
+++ b/drivers/scsi/mvumi.c
@@ -2081,7 +2081,6 @@ static unsigned char mvumi_build_frame(struct mvumi_hba *mhba,
 	case DMA_TO_DEVICE:
 		pframe->cmd_flag |= CMD_FLAG_DATA_OUT;
 		break;
-	case DMA_BIDIRECTIONAL:
 	default:
 		dev_warn(&mhba->pdev->dev, "unexpected data direction[%d] "
 			"cmd[0x%x]\n", scmd->sc_data_direction, scmd->cmnd[0]);
-- 
2.1.2

[PATCH 16/17] sym_glue: Fix bidi command test

[Bart Van Assche]

Signed-off-by: Bart Van Assche <bart.vanassche@sandisk.com>
---
 drivers/scsi/sym53c8xx_2/sym_glue.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/scsi/sym53c8xx_2/sym_glue.c b/drivers/scsi/sym53c8xx_2/sym_glue.c
index 5d00e51..c33c697 100644
--- a/drivers/scsi/sym53c8xx_2/sym_glue.c
+++ b/drivers/scsi/sym53c8xx_2/sym_glue.c
@@ -374,14 +374,16 @@ int sym_setup_data_and_start(struct sym_hcb *np, struct scsi_cmnd *cmd, struct s
 		cp->segments = 0;
 	}
 
+	if (scsi_bidi_cmnd(cmd)) {
+		scmd_printk(KERN_INFO, cmd, "got bidi command");
+		sym_set_cam_status(cmd, DID_ERROR);
+		goto out_abort;
+	}
+
 	/*
 	 *  Set the data pointer.
 	 */
 	switch (dir) {
-	case DMA_BIDIRECTIONAL:
-		scmd_printk(KERN_INFO, cmd, "got DMA_BIDIRECTIONAL command");
-		sym_set_cam_status(cmd, DID_ERROR);
-		goto out_abort;
 	case DMA_TO_DEVICE:
 		goalp = SCRIPTA_BA(np, data_out2) + 8;
 		lastp = goalp - 8 - (cp->segments * (2*4));
-- 
2.1.2

[PATCH 17/17] IB/srp: Detect bidi commands properly

[Bart Van Assche]

The SCSI core never sets sc_data_direction to DMA_BIDIRECTIONAL. This allows
to simplify srp_unmap_data(). Fix the bidi test in srp_map_data().

Signed-off-by: Bart Van Assche <bart.vanassche@sandisk.com>
Cc: Roland Dreier <roland@kernel.org>
---
 drivers/infiniband/ulp/srp/ib_srp.c | 10 +++-------
 1 file changed, 3 insertions(+), 7 deletions(-)

diff --git a/drivers/infiniband/ulp/srp/ib_srp.c b/drivers/infiniband/ulp/srp/ib_srp.c
index 0747c05..22b7529 100644
--- a/drivers/infiniband/ulp/srp/ib_srp.c
+++ b/drivers/infiniband/ulp/srp/ib_srp.c
@@ -1063,9 +1063,7 @@ static void srp_unmap_data(struct scsi_cmnd *scmnd,
 	struct ib_device *ibdev = dev->dev;
 	int i, res;
 
-	if (!scsi_sglist(scmnd) ||
-	    (scmnd->sc_data_direction != DMA_TO_DEVICE &&
-	     scmnd->sc_data_direction != DMA_FROM_DEVICE))
+	if (!scsi_sglist(scmnd))
 		return;
 
 	if (dev->use_fast_reg) {
@@ -1524,11 +1522,9 @@ static int srp_map_data(struct scsi_cmnd *scmnd, struct srp_rdma_ch *ch,
 	if (!scsi_sglist(scmnd) || scmnd->sc_data_direction == DMA_NONE)
 		return sizeof (struct srp_cmd);
 
-	if (scmnd->sc_data_direction != DMA_FROM_DEVICE &&
-	    scmnd->sc_data_direction != DMA_TO_DEVICE) {
+	if (scsi_bidi_cmnd(scmnd)) {
 		shost_printk(KERN_WARNING, target->scsi_host,
-			     PFX "Unhandled data direction %d\n",
-			     scmnd->sc_data_direction);
+			     PFX "Bidi commands are not yet supported\n");
 		return -EINVAL;
 	}
 
-- 
2.1.2

Re: [PATCH 17/17] IB/srp: Detect bidi commands properly

[Sagi Grimberg]

On 1/23/2015 2:17 PM, Bart Van Assche wrote:
> The SCSI core never sets sc_data_direction to DMA_BIDIRECTIONAL. This allows
> to simplify srp_unmap_data(). Fix the bidi test in srp_map_data().
>
> Signed-off-by: Bart Van Assche <bart.vanassche@sandisk.com>
> Cc: Roland Dreier <roland@kernel.org>
> ---
>   drivers/infiniband/ulp/srp/ib_srp.c | 10 +++-------
>   1 file changed, 3 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/infiniband/ulp/srp/ib_srp.c b/drivers/infiniband/ulp/srp/ib_srp.c
> index 0747c05..22b7529 100644
> --- a/drivers/infiniband/ulp/srp/ib_srp.c
> +++ b/drivers/infiniband/ulp/srp/ib_srp.c
> @@ -1063,9 +1063,7 @@ static void srp_unmap_data(struct scsi_cmnd *scmnd,
>   	struct ib_device *ibdev = dev->dev;
>   	int i, res;
>
> -	if (!scsi_sglist(scmnd) ||
> -	    (scmnd->sc_data_direction != DMA_TO_DEVICE &&
> -	     scmnd->sc_data_direction != DMA_FROM_DEVICE))
> +	if (!scsi_sglist(scmnd))
>   		return;
>
>   	if (dev->use_fast_reg) {
> @@ -1524,11 +1522,9 @@ static int srp_map_data(struct scsi_cmnd *scmnd, struct srp_rdma_ch *ch,
>   	if (!scsi_sglist(scmnd) || scmnd->sc_data_direction == DMA_NONE)
>   		return sizeof (struct srp_cmd);
>
> -	if (scmnd->sc_data_direction != DMA_FROM_DEVICE &&
> -	    scmnd->sc_data_direction != DMA_TO_DEVICE) {
> +	if (scsi_bidi_cmnd(scmnd)) {
>   		shost_printk(KERN_WARNING, target->scsi_host,
> -			     PFX "Unhandled data direction %d\n",
> -			     scmnd->sc_data_direction);
> +			     PFX "Bidi commands are not yet supported\n");
>   		return -EINVAL;
>   	}
>
>

Looks good.

Acked-by: Sagi Grimberg <sagig@mellanox.com>

Re: [PATCH 0/17] Clear up bidi command confusion

[Christoph Hellwig]

On Fri, Jan 23, 2015 at 01:05:30PM +0100, Bart Van Assche wrote:
> There is some confusion in the SCSI core and in SCSI LLDs around the
> meaning of sc_data_direction and whether or not this member can have the
> value DMA_BIDIRECTIONAL. Clear up this confusion. The patches in this
> series are:

I wonder if we should change the code to set DMA_BIDIRECTIONAL for
bidi commands.  That seems a lot more logical than the current
version.

Also I don't think all the debug checks for bidi commands that you
change should stay at all - driver need to set the QUEUE_FLAG_BIDI to
ever see a bidi command.

It would also nice to add a host template flag for bidi support instead
of having to poke into the block layer request_queue while we're at it.

Re: [PATCH 0/17] Clear up bidi command confusion

[Bart Van Assche]

On 01/23/15 14:12, Christoph Hellwig wrote:
> On Fri, Jan 23, 2015 at 01:05:30PM +0100, Bart Van Assche wrote:
>> There is some confusion in the SCSI core and in SCSI LLDs around the
>> meaning of sc_data_direction and whether or not this member can have the
>> value DMA_BIDIRECTIONAL. Clear up this confusion. The patches in this
>> series are:
> 
> I wonder if we should change the code to set DMA_BIDIRECTIONAL for
> bidi commands.  That seems a lot more logical than the current
> version.
> 
> Also I don't think all the debug checks for bidi commands that you
> change should stay at all - driver need to set the QUEUE_FLAG_BIDI to
> ever see a bidi command.
> 
> It would also nice to add a host template flag for bidi support instead
> of having to poke into the block layer request_queue while we're at it.

Hello Christoph,

This makes sense to me. I will rework this patch series as you proposed.

Bart.

Re: [PATCH 0/17] Clear up bidi command confusion

[Boaz Harrosh]

On 01/26/2015 11:58 AM, Bart Van Assche wrote:

> Hello Christoph,
> 
> This makes sense to me. I will rework this patch series as you proposed.
> 

Do you have a bidi setup to test against?

Sending xor command to scsi_dbg is only half the test for me because, yes
there are two buffers, but they are of same size so bugs might be masked.
(From experience)

Thanks
Boaz

> Bart.

Re: [PATCH 0/17] Clear up bidi command confusion

[Bart Van Assche]

On 01/29/15 14:07, Boaz Harrosh wrote:
> On 01/26/2015 11:58 AM, Bart Van Assche wrote:
> 
>> Hello Christoph,
>>
>> This makes sense to me. I will rework this patch series as you proposed.
> 
> Do you have a bidi setup to test against?
> 
> Sending xor command to scsi_dbg is only half the test for me because, yes
> there are two buffers, but they are of same size so bugs might be masked.
> (From experience)

Hello Boaz,

If anyone would like to submit a scsi_debug patch that adds support to
that kernel driver for a bidi command for which the input and output
buffers have different sizes I think that would be helpful.

Bart.

Re: [PATCH 0/17] Clear up bidi command confusion

[Boaz Harrosh]

On 01/29/2015 03:20 PM, Bart Van Assche wrote:
> On 01/29/15 14:07, Boaz Harrosh wrote:
>> On 01/26/2015 11:58 AM, Bart Van Assche wrote:
>>
>>> Hello Christoph,
>>>
>>> This makes sense to me. I will rework this patch series as you proposed.
>>
>> Do you have a bidi setup to test against?
>>
>> Sending xor command to scsi_dbg is only half the test for me because, yes
>> there are two buffers, but they are of same size so bugs might be masked.
>> (From experience)
> 
> Hello Boaz,
> 
> If anyone would like to submit a scsi_debug patch that adds support to
> that kernel driver for a bidi command for which the input and output
> buffers have different sizes I think that would be helpful.
> 

Hi Bart.

scsi_dbg is a scsi-blk-device type device (forgot the exact name).
I do not think there is such a command defined by the STD for this command
set.

its only for other device types like osd, printer and so on.

But sending XOR commands to scsi_dbg is a good start.

> Bart.
> 

Please Note: I do not agree for the use of the constant DMA_BIDIRECTIONAL
to denote scsi_bidi_cmnd(cmd). This is wrong and takes us back not
forward.

If anything at all is done, cmd->sc_data_direction should be just removed
all together. It is not needed and denotes nothing more than what is already
available at the request level. Please do not add any more new code on top
of cmd->sc_data_direction. (At minimum just remove the all DMA_BIDIRECTIONAL
checks and you are done)

Cheers
Boaz

Re: [PATCH 0/17] Clear up bidi command confusion

[Boaz Harrosh]

On 01/23/2015 03:12 PM, Christoph Hellwig wrote:
> On Fri, Jan 23, 2015 at 01:05:30PM +0100, Bart Van Assche wrote:
>> There is some confusion in the SCSI core and in SCSI LLDs around the
>> meaning of sc_data_direction and whether or not this member can have the
>> value DMA_BIDIRECTIONAL. Clear up this confusion. The patches in this
>> series are:
> 
> I wonder if we should change the code to set DMA_BIDIRECTIONAL for
> bidi commands.  That seems a lot more logical than the current
> version.
> 

You cannot do this. Because a Bidi Command is actually two commands
one for the WRITE side (DMA_TO_DEVICE) which is this one, and another
command for the READ side (DMA_FROM_DEVICE).

The DMA_XXX_ enum must not to be confused with the t10-scsi Bidi commands.
In DMA world the enum means: What are the allowed access on the memory buffer,
is Device allowed to read-from-memory-only or write-to-memory-only or both
simultaneously "on the same buffer".
It is a flag to the IO-mmu on the direction of its mappings.

A t10-scsi bidi command is "two buffers". The command caries two distinct
buffers one is only-written-to 2nd only-read-from. But these are two distinct
buffers each his proper direction.

If you ask me you need to remove sc_data_direction all together and just
go directly to the READ/WRITE flag at request level. SCSI has no business
duplicating the same information in another member another way.

In any way t10-scsi has no use in the entire of its STD of the DMA_BIDIRECTIONAL
sense. ie. same buffer, two directional access. So DMA_BIDIRECTIONAL is just
dead code for sc_data_direction. It could be imagined but t10-scsi does not
have a use for it.

Again Not to be confused with one-command two buffers, which is exactly the opposite.

> Also I don't think all the debug checks for bidi commands that you
> change should stay at all - driver need to set the QUEUE_FLAG_BIDI to
> ever see a bidi command.
> 

Exactly just remove the all checks.

> It would also nice to add a host template flag for bidi support instead
> of having to poke into the block layer request_queue while we're at it.

Cheers
Boaz

Re: [PATCH 0/17] Clear up bidi command confusion

[James Bottomley]

On Thu, 2015-01-29 at 15:00 +0200, Boaz Harrosh wrote:
> On 01/23/2015 03:12 PM, Christoph Hellwig wrote:
> > On Fri, Jan 23, 2015 at 01:05:30PM +0100, Bart Van Assche wrote:
> >> There is some confusion in the SCSI core and in SCSI LLDs around the
> >> meaning of sc_data_direction and whether or not this member can have the
> >> value DMA_BIDIRECTIONAL. Clear up this confusion. The patches in this
> >> series are:
> > 
> > I wonder if we should change the code to set DMA_BIDIRECTIONAL for
> > bidi commands.  That seems a lot more logical than the current
> > version.
> > 
> 
> You cannot do this. Because a Bidi Command is actually two commands
> one for the WRITE side (DMA_TO_DEVICE) which is this one, and another
> command for the READ side (DMA_FROM_DEVICE).

You're not thinking about this the correct way.  DMA_BIDIRECTIONAL is a
DMA engine flag.  We use it in SCSI for historical reasons (mainly to
prevent a translation from the SCSI version).  Since it was never used,
most SCSI drivers have code to check and reject commands with it set.
For actual bidirectional commands, you have to check scsi_bidi_command()
and programme the DMA engine separately for both phases, so the flag is
useless in this case.

The proposal is to make it the signal for bidirectional commands (in
which case most HBAs would make it do the right thing; i.e. reject) but
it still wouldn't be how you'd program the DMA enigne; that still would
have to be done in two phases as it is today.

James

Re: [PATCH 0/17] Clear up bidi command confusion

[Boaz Harrosh]

On 01/29/2015 04:41 PM, James Bottomley wrote:
> On Thu, 2015-01-29 at 15:00 +0200, Boaz Harrosh wrote:
>> On 01/23/2015 03:12 PM, Christoph Hellwig wrote:
>>> On Fri, Jan 23, 2015 at 01:05:30PM +0100, Bart Van Assche wrote:
>>>> There is some confusion in the SCSI core and in SCSI LLDs around the
>>>> meaning of sc_data_direction and whether or not this member can have the
>>>> value DMA_BIDIRECTIONAL. Clear up this confusion. The patches in this
>>>> series are:
>>>
>>> I wonder if we should change the code to set DMA_BIDIRECTIONAL for
>>> bidi commands.  That seems a lot more logical than the current
>>> version.
>>>
>>
>> You cannot do this. Because a Bidi Command is actually two commands
>> one for the WRITE side (DMA_TO_DEVICE) which is this one, and another
>> command for the READ side (DMA_FROM_DEVICE).
> 
> You're not thinking about this the correct way.  DMA_BIDIRECTIONAL is a
> DMA engine flag.  We use it in SCSI for historical reasons (mainly to
> prevent a translation from the SCSI version).  Since it was never used,
> most SCSI drivers have code to check and reject commands with it set.
> For actual bidirectional commands, you have to check scsi_bidi_command()
> and programme the DMA engine separately for both phases, so the flag is
> useless in this case.
> 

This is what I meant how is above different from what I said?

> The proposal is to make it the signal for bidirectional commands

This I disagree, and would be a very bad idea and will take us back, to
hacking time. As you noted, please let us leave DMA_BIDIRECTIONAL to the
DMA engines. Let us just stir away from the DMA_XXX flags altogether and
move back to block layer flags. 

 (in which case most HBAs would make it do the right thing; i.e. reject) 

This is not necessary, (the check and rejection), commands will not be issued
to LLDs that did not mark support for BiDi. So they will never receive
such commands and it is added dead code.

> but
> it still wouldn't be how you'd program the DMA enigne; that still would
> have to be done in two phases as it is today.
> 

Exactly my point. Only one small correction, these are not two phases, as in
phases-1 then phases-2. These are "two" memory buffers independently programmed
for DMA transfer. The actual transfer occurs simultaneously, on both buffers.

> James
> 
> 

Thanks
Boaz