tgt: convert ibmvstgt and libsrp to use scsi_data_buffer

Signed-off-by FUJITA Tomonori <[email protected]>

---
 drivers/scsi/ibmvscsi/ibmvstgt.c |    2 +-
 drivers/scsi/libsrp.c            |   27 ++++++++++++++-------------
 2 files changed, 15 insertions(+), 14 deletions(-)

drivers/scsi/ibmvscsi/ibmvstgt.c +1 -1

index 8ba7dd0..ab173d4 100644
@@ -286,7 +286,7 @@ static int ibmvstgt_cmd_done(struct scsi_cmnd *sc,
 	dprintk("%p %p %x %u\n", iue, target, vio_iu(iue)->srp.cmd.cdb[0],
 		cmd->usg_sg);
 
-	if (sc->use_sg)
+	if (scsi_sg_count(sc))
 		err = srp_transfer_data(sc, &vio_iu(iue)->srp.cmd, ibmvstgt_rdma, 1, 1);
 
 	spin_lock_irqsave(&target->lock, flags);

drivers/scsi/libsrp.c +12 -11

index 732446e..32e1529 100644
@@ -192,18 +192,18 @@ static int srp_direct_data(struct scsi_cmnd *sc, struct srp_direct_buf *md,
 
 	if (dma_map) {
 		iue = (struct iu_entry *) sc->SCp.ptr;
-		sg = sc->request_buffer;
+		sg = scsi_sglist(sc);
 
-		dprintk("%p %u %u %d\n", iue, sc->request_bufflen,
-			md->len, sc->use_sg);
+		dprintk("%p %u %u %d\n", iue, scsi_bufflen(sc),
+			md->len, scsi_sg_count(sc));
 
-		nsg = dma_map_sg(iue->target->dev, sg, sc->use_sg,
+		nsg = dma_map_sg(iue->target->dev, sg, scsi_sg_count(sc),
 				 DMA_BIDIRECTIONAL);
 		if (!nsg) {
-			printk("fail to map %p %d\n", iue, sc->use_sg);
+			printk("fail to map %p %d\n", iue, scsi_sg_count(sc));
 			return 0;
 		}
-		len = min(sc->request_bufflen, md->len);
+		len = min(scsi_bufflen(sc), md->len);
 	} else
 		len = md->len;
 

@@ -229,10 +229,10 @@ static int srp_indirect_data(struct scsi_cmnd *sc, struct srp_cmd *cmd,
 
 	if (dma_map || ext_desc) {
 		iue = (struct iu_entry *) sc->SCp.ptr;
-		sg = sc->request_buffer;
+		sg = scsi_sglist(sc);
 
 		dprintk("%p %u %u %d %d\n",
-			iue, sc->request_bufflen, id->len,
+			iue, scsi_bufflen(sc), id->len,
 			cmd->data_in_desc_cnt, cmd->data_out_desc_cnt);
 	}
 

@@ -268,13 +268,14 @@ static int srp_indirect_data(struct scsi_cmnd *sc, struct srp_cmd *cmd,
 
 rdma:
 	if (dma_map) {
-		nsg = dma_map_sg(iue->target->dev, sg, sc->use_sg, DMA_BIDIRECTIONAL);
+		nsg = dma_map_sg(iue->target->dev, sg, scsi_sg_count(sc),
+				 DMA_BIDIRECTIONAL);
 		if (!nsg) {
-			eprintk("fail to map %p %d\n", iue, sc->use_sg);
+			eprintk("fail to map %p %d\n", iue, scsi_sg_count(sc));
 			err = -EIO;
 			goto free_mem;
 		}
-		len = min(sc->request_bufflen, id->len);
+		len = min(scsi_bufflen(sc), id->len);
 	} else
 		len = id->len;
 

Quoted 81 lines · FUJITA Tomonori

Signed-off-by FUJITA Tomonori <[email protected]>

---
 drivers/scsi/ibmvscsi/ibmvstgt.c |    2 +-
 drivers/scsi/libsrp.c            |   27 ++++++++++++++-------------
 2 files changed, 15 insertions(+), 14 deletions(-)

drivers/scsi/ibmvscsi/ibmvstgt.c +1 -1

index 8ba7dd0..ab173d4 100644
@@ -286,7 +286,7 @@ static int ibmvstgt_cmd_done(struct scsi_cmnd *sc,
 	dprintk("%p %p %x %u\n", iue, target, vio_iu(iue)->srp.cmd.cdb[0],
 		cmd->usg_sg);
 
-	if (sc->use_sg)
+	if (scsi_sg_count(sc))
 		err = srp_transfer_data(sc, &vio_iu(iue)->srp.cmd, ibmvstgt_rdma, 1, 1);
 
 	spin_lock_irqsave(&target->lock, flags);

drivers/scsi/libsrp.c +12 -11

index 732446e..32e1529 100644
@@ -192,18 +192,18 @@ static int srp_direct_data(struct scsi_cmnd *sc, struct srp_direct_buf *md,
 
 	if (dma_map) {
 		iue = (struct iu_entry *) sc->SCp.ptr;
-		sg = sc->request_buffer;
+		sg = scsi_sglist(sc);
 
-		dprintk("%p %u %u %d\n", iue, sc->request_bufflen,
-			md->len, sc->use_sg);
+		dprintk("%p %u %u %d\n", iue, scsi_bufflen(sc),
+			md->len, scsi_sg_count(sc));
 
-		nsg = dma_map_sg(iue->target->dev, sg, sc->use_sg,
+		nsg = dma_map_sg(iue->target->dev, sg, scsi_sg_count(sc),
 				 DMA_BIDIRECTIONAL);
 		if (!nsg) {
-			printk("fail to map %p %d\n", iue, sc->use_sg);
+			printk("fail to map %p %d\n", iue, scsi_sg_count(sc));
 			return 0;
 		}
-		len = min(sc->request_bufflen, md->len);
+		len = min(scsi_bufflen(sc), md->len);
 	} else
 		len = md->len;
 

@@ -229,10 +229,10 @@ static int srp_indirect_data(struct scsi_cmnd *sc, struct srp_cmd *cmd,
 
 	if (dma_map || ext_desc) {
 		iue = (struct iu_entry *) sc->SCp.ptr;
-		sg = sc->request_buffer;
+		sg = scsi_sglist(sc);
 
 		dprintk("%p %u %u %d %d\n",
-			iue, sc->request_bufflen, id->len,
+			iue, scsi_bufflen(sc), id->len,
 			cmd->data_in_desc_cnt, cmd->data_out_desc_cnt);
 	}
 

@@ -268,13 +268,14 @@ static int srp_indirect_data(struct scsi_cmnd *sc, struct srp_cmd *cmd,
 
 rdma:
 	if (dma_map) {
-		nsg = dma_map_sg(iue->target->dev, sg, sc->use_sg, DMA_BIDIRECTIONAL);
+		nsg = dma_map_sg(iue->target->dev, sg, scsi_sg_count(sc),
+				 DMA_BIDIRECTIONAL);
 		if (!nsg) {
-			eprintk("fail to map %p %d\n", iue, sc->use_sg);
+			eprintk("fail to map %p %d\n", iue, scsi_sg_count(sc));
 			err = -EIO;
 			goto free_mem;
 		}
-		len = min(sc->request_bufflen, id->len);
+		len = min(scsi_bufflen(sc), id->len);
 	} else
 		len = id->len;
 

@@ -425,8 +426,8 @@ int srp_cmd_queue(struct Scsi_Host *shost, struct srp_cmd *cmd, void *info,
 

drivers/scsi/libsrp.c int srp_cmd_queue(struct Scsi_Host *shost, struct srp_cmd *cmd, void *info, · line 433

Quoted 9 lines

drivers/scsi/libsrp.c +2 -2

 	sc->SCp.ptr = info;
 	memcpy(sc->cmnd, cmd->cdb, MAX_COMMAND_SIZE);
-	sc->request_bufflen = len;
-	sc->request_buffer = (void *) (unsigned long) addr;
+	sc->sdb.length = len;
+	sc->sdb.sglist = (void *) (unsigned long) addr;
 	sc->tag = tag;
 	err = scsi_tgt_queue_command(sc, (struct scsi_lun *) &cmd->lun, cmd->tag);
 	if (err)

What is done here in srp_cmd_queue() looks scary to me. even today. What is that u64 addr that gets truncated to unsigned long and put on sglist? What data-buffer "len" is suppose to describe? And "dir" is for what data?

It is made to look like addr is a linear pointer with a use_sg==0 issued command, which is no longer allowed. Only we know it is not, because of the "(void *)(unsigned long) addr;" which is a bug in 64-bit.

If this is a totally private message sent threw the scsi-ml. I would rather it was done with DMA_NONE,bufflen=0,sglist=NULL and put all the user-info into scsi_cmnd.SCp like above "void* info".

Quoted 6 lines · On Sun, 09 Sep 2007 13:12:03 +0300 Boaz Harrosh <[email protected]> wrote:

Quoted 5 lines · FUJITA Tomonori

Signed-off-by FUJITA Tomonori <[email protected]>

---
...

(quoted diff) +0 -0

@@ -425,8 +426,8 @@ int srp_cmd_queue(struct Scsi_Host *shost, struct srp_cmd *cmd, void *info,
 

Quoted 13 lines

drivers/scsi/libsrp.c int srp_cmd_queue(struct Scsi_Host *shost, struct srp_cmd *cmd, void *info, · line 433

Quoted 9 lines

drivers/scsi/libsrp.c +2 -2

 	sc->SCp.ptr = info;
 	memcpy(sc->cmnd, cmd->cdb, MAX_COMMAND_SIZE);
-	sc->request_bufflen = len;
-	sc->request_buffer = (void *) (unsigned long) addr;
+	sc->sdb.length = len;
+	sc->sdb.sglist = (void *) (unsigned long) addr;
 	sc->tag = tag;
 	err = scsi_tgt_queue_command(sc, (struct scsi_lun *) &cmd->lun, cmd->tag);
 	if (err)

What is done here in srp_cmd_queue() looks scary to me. even today. What is that u64 addr that gets truncated to unsigned long and put on sglist? What data-buffer "len" is suppose to describe? And "dir" is for what data?

No trancated since it's used for an address. addr, data length, and data transfer direction though they are not used now.

Quoted 9 lines

It is made to look like addr is a linear pointer with a use_sg==0 issued command, which is no longer allowed. Only we know it is not, because of the "(void *)(unsigned long) addr;" which is a bug in 64-bit.

If this is a totally private message sent threw the scsi-ml. I would rather it was done with DMA_NONE,bufflen=0,sglist=NULL and put all the user-info into scsi_cmnd.SCp like above "void* info".

tgt doesn't queue a command to scsi-ml. It queues it to user space.

Quoted 23 lines · FUJITA Tomonori

Quoted 19 lines · On Sun, 09 Sep 2007 13:12:03 +0300 Boaz Harrosh <[email protected]> wrote:

Quoted 4 lines · FUJITA Tomonori

Signed-off-by FUJITA Tomonori <[email protected]>

---
...

drivers/scsi/libsrp.c int srp_cmd_queue(struct Scsi_Host *shost, struct srp_cmd *cmd, void *info, · line 433

Quoted 10 lines

drivers/scsi/libsrp.c +2 -2

@@ -425,8 +426,8 @@ int srp_cmd_queue(struct Scsi_Host *shost, struct srp_cmd *cmd, void *info,
 
 	sc->SCp.ptr = info;
 	memcpy(sc->cmnd, cmd->cdb, MAX_COMMAND_SIZE);
-	sc->request_bufflen = len;
-	sc->request_buffer = (void *) (unsigned long) addr;
+	sc->sdb.length = len;
+	sc->sdb.sglist = (void *) (unsigned long) addr;
 	sc->tag = tag;
 	err = scsi_tgt_queue_command(sc, (struct scsi_lun *) &cmd->lun, cmd->tag);
 	if (err)

What is done here in srp_cmd_queue() looks scary to me. even today. What is that u64 addr that gets truncated to unsigned long and put on sglist? What data-buffer "len" is suppose to describe? And "dir" is for what data?

Quoted 3 lines

No trancated since it's used for an address. addr, data length, and data transfer direction though they are not used now.

I wish you could maybe clean up the unused stuff,

and/or give addr its proper type. If it's a pointer than make it a pointer.

Also you are implying a use_sg==0 here which is not allowed.

Quoted 10 lines

Quoted 9 lines

It is made to look like addr is a linear pointer with a use_sg==0 issued command, which is no longer allowed. Only we know it is not, because of the "(void *)(unsigned long) addr;" which is a bug in 64-bit.

If this is a totally private message sent threw the scsi-ml. I would rather it was done with DMA_NONE,bufflen=0,sglist=NULL and put all the user-info into scsi_cmnd.SCp like above "void* info".

Quoted 1 line

tgt doesn't queue a command to scsi-ml. It queues it to user space.

Even though. Please don't misuse scsi_cmnd members in this way. If you are not using any of scsi-ml functions on these commands and they do not carry sg-lists than why use a scsi_cmnd at all?

You can just use a tgt private structure.

If these commands do go through some scsi-ml functions than banging scsi_cmnd.sdb.sglist this way is dangerous.

Quoted 24 lines · On Sun, 09 Sep 2007 17:28:41 +0300 Boaz Harrosh <[email protected]> wrote:

Quoted 23 lines · FUJITA Tomonori

Quoted 19 lines · On Sun, 09 Sep 2007 13:12:03 +0300 Boaz Harrosh <[email protected]> wrote:

Quoted 4 lines · FUJITA Tomonori

Signed-off-by FUJITA Tomonori <[email protected]>

---
...

drivers/scsi/libsrp.c int srp_cmd_queue(struct Scsi_Host *shost, struct srp_cmd *cmd, void *info, · line 433

Quoted 10 lines

drivers/scsi/libsrp.c +2 -2

@@ -425,8 +426,8 @@ int srp_cmd_queue(struct Scsi_Host *shost, struct srp_cmd *cmd, void *info,
 
 	sc->SCp.ptr = info;
 	memcpy(sc->cmnd, cmd->cdb, MAX_COMMAND_SIZE);
-	sc->request_bufflen = len;
-	sc->request_buffer = (void *) (unsigned long) addr;
+	sc->sdb.length = len;
+	sc->sdb.sglist = (void *) (unsigned long) addr;
 	sc->tag = tag;
 	err = scsi_tgt_queue_command(sc, (struct scsi_lun *) &cmd->lun, cmd->tag);
 	if (err)

What is done here in srp_cmd_queue() looks scary to me. even today. What is that u64 addr that gets truncated to unsigned long and put on sglist? What data-buffer "len" is suppose to describe? And "dir" is for what data?

Quoted 4 lines

Quoted 3 lines

No trancated since it's used for an address. addr, data length, and data transfer direction though they are not used now.

I wish you could maybe clean up the unused stuff,

Well, I put them for Xen scsiback driver though now I could remove it.

Quoted 2 lines

and/or give addr its proper type. If it's a pointer than make it a pointer.

You need to read the SRP spec.

Quoted 12 lines

Also you are implying a use_sg==0 here which is not allowed.

Quoted 10 lines

Quoted 9 lines

It is made to look like addr is a linear pointer with a use_sg==0 issued command, which is no longer allowed. Only we know it is not, because of the "(void *)(unsigned long) addr;" which is a bug in 64-bit.

If this is a totally private message sent threw the scsi-ml. I would rather it was done with DMA_NONE,bufflen=0,sglist=NULL and put all the user-info into scsi_cmnd.SCp like above "void* info".

Quoted 5 lines

Quoted 1 line

tgt doesn't queue a command to scsi-ml. It queues it to user space.

Even though. Please don't misuse scsi_cmnd members in this way. If you are not using any of scsi-ml functions on these commands and they do not carry sg-lists than why use a scsi_cmnd at all?

We try to use scsi-ml and transport class as much as possible.

Quoted 1 line

You can just use a tgt private structure.

And we don't try to do this.

Quoted 2 lines · On Sun, 09 Sep 2007 17:28:41 +0300 Boaz Harrosh <[email protected]> wrote:

Quoted 1 line

I wish you could maybe clean up the unused stuff,

Well, I put them for Xen scsiback driver though now I could remove it.