Skip to contentSkip to conversation

scsi_cmnd accessors issues

PATCH In review Boaz Harrosh 4 participants 17 messages Jun 12, 2007 – Jun 18, 2007 SCSI

lore
Share & tools
lore

Boaz Harrosh

author
#0 lore raw

2 things

[1]
There are 2 files (see attached patch) that use scsi_befflen() & scsi_sg_count() as L-value. If I try the scsi_sgtable approach and do:

#define scsi_sg_count(cmd) ((cmd)->sg_table ? (cmd)->sg_table->sg_count : 0) #define scsi_sglist(cmd) ((cmd)->sg_table ? (cmd)->sg_table->sglist : NULL) #define scsi_bufflen(cmd) ((cmd)->sg_table ? (cmd)->sg_table->length : 0)

I get compilation errors. Attached solution is to add scsi_set_ accessors to these members. But it would be best to STOP these drivers from doing that ugly hack of modifying sg_count and bufflen.
(CC sign-off-by maintainers of the drivers in question. Files are drivers/scsi/tmscsim.c & drivers/scsi/stex.c )

see: http://www.bhalevy.com/open-osd/download/sgtable_bidi_varlen/
for the full bidi over scsi_sgtable solution

[2]
if I use __deprecated on request_buffer, request_bufflen, and use_sg with scsi_sgtable implementation Than I get below list of files complaining:
drivers/ata/libata-scsi.c
drivers/firewire/fw-sbp2.c drivers/infiniband/ulp/srp/ib_srp.c drivers/message/i2o/i2o_scsi.c
drivers/scsi/aacraid/aachba.c
drivers/scsi/lpfc/lpfc_scsi.c
drivers/scsi/aha152x.c
drivers/scsi/pcmcia/nsp_cs.c drivers/scsi/sym53c8xx_2/sym_glue.h drivers/scsi/sym53c8xx_2/sym_glue.c
drivers/scsi/ncr53c8xx.c
drivers/scsi/sd.c
drivers/scsi/sr.c
drivers/scsi/advansys.c
drivers/scsi/psi240i.c
drivers/scsi/aha1542.c
drivers/scsi/ips.c
drivers/scsi/fd_mcs.c
drivers/scsi/in2000.c
drivers/scsi/NCR5380.c
drivers/scsi/qla1280.c
drivers/scsi/seagate.c
drivers/scsi/dc395x.c
drivers/scsi/atp870u.c
drivers/scsi/gdth.c
drivers/scsi/ide-scsi.c
drivers/scsi/ppa.c
drivers/scsi/imm.c
drivers/scsi/hptiop.c
drivers/usb/image/microtek.c
drivers/usb/storage/protocol.c drivers/usb/storage/transport.c drivers/usb/storage/shuttle_usbat.c drivers/usb/storage/sddr09.c
drivers/usb/storage/freecom.c
drivers/usb/storage/isd200.c

and also these files from scsi-ml that need changing when implementation changes:
drivers/scsi/scsi.c
drivers/scsi/scsi_error.c
drivers/scsi/scsi_debug.c

(see: 0004-Convert-scsi-ml-to-use-of-new-scsi_sgtable.patch at scsi_cmnd.h)

Which of the files do you have pending patches for? Which do you need that I send what I have for them?

Thanks
Boaz

Quoted 1 line

From 4f3c5a3b6dd6acb6240a52cecf52fc6787f625e8 Mon Sep 17 00:00:00 2001

From: Boaz Harrosh <bharrosh@bh-buildlin2.(none)>
Date: Mon, 11 Jun 2007 17:19:39 +0300
Subject: [PATCH] Add scsi_set_ accessors to bufflen and sg_count

- drivers/scsi/stex.c && drivers/scsi/tmscsim.c modify bufflen and sg_count members from struct scsi_cmnd. This will not compile when in the future accessors are implemented over scsi_sgtable. So we add set_ accessors for these drivers to use.
- It would be best if these drivers STOP modifying these members and this patch is reverted

---
 drivers/scsi/stex.c      |    4 ++--
 drivers/scsi/tmscsim.c   |    4 ++--
 include/scsi/scsi_cmnd.h |   10 ++++++++++
 3 files changed, 14 insertions(+), 4 deletions(-)
drivers/scsi/stex.c +2 -2
index adda296..4795d53 100644
@@ -719,8 +719,8 @@ static void stex_ys_commands(struct st_hba *hba,
 
 	if (ccb->cmd->cmnd[0] == MGT_CMD &&
 		resp->scsi_status != SAM_STAT_CHECK_CONDITION) {
-		scsi_bufflen(ccb->cmd) =
-			le32_to_cpu(*(__le32 *)&resp->variable[0]);
+		scsi_set_bufflen(ccb->cmd,
+			le32_to_cpu(*(__le32 *)&resp->variable[0]));
 		return;
 	}
drivers/scsi/tmscsim.c +2 -2
index 73c5ca0..bfc718a 100644
@@ -1728,7 +1728,7 @@ dc390_SRBdone( struct dc390_acb* pACB, struct dc390_dcb* pDCB, struct dc390_srb*
 		       (u32) pcmd->result, (u32) pSRB->TotalXferredLen));
 	    } else {
 		SET_RES_DRV(pcmd->result, DRIVER_SENSE);
-		scsi_sg_count(pcmd) = pSRB->SavedSGCount;
+		scsi_set_sg_count(pcmd, pSRB->SavedSGCount);
 		//pSRB->ScsiCmdLen	 = (u8) (pSRB->Segment1[0] >> 8);
 		DEBUG0 (printk ("DC390: RETRY pid %li (%02x), target %02i-%02i\n", pcmd->pid, pcmd->cmnd[0], pcmd->device->id, pcmd->device->lun));
 		pSRB->TotalXferredLen = 0;

@@ -1750,7 +1750,7 @@ dc390_SRBdone( struct dc390_acb* pACB, struct dc390_dcb* pDCB, struct dc390_srb*
 	else if( status_byte(status) == QUEUE_FULL )
 	{
 	    scsi_track_queue_full(pcmd->device, pDCB->GoingSRBCnt - 1);
-	    scsi_sg_count(pcmd) = pSRB->SavedSGCount;
+	    scsi_set_sg_count(pcmd, pSRB->SavedSGCount);
 	    DEBUG0 (printk ("DC390: RETRY pid %li (%02x), target %02i-%02i\n", pcmd->pid, pcmd->cmnd[0], pcmd->device->id, pcmd->device->lun));
 	    pSRB->TotalXferredLen = 0;
 	    SET_RES_DID(pcmd->result, DID_SOFT_ERROR);
include/scsi/scsi_cmnd.h +10 -0
index 53e1705..d0c1101 100644
@@ -152,6 +152,16 @@ static inline int scsi_get_resid(struct scsi_cmnd *cmd)
 	return cmd->resid;
 }
 
+static inline void scsi_set_sg_count(struct scsi_cmnd *cmd, int sg_count)
+{
+	cmd->use_sg = sg_count;
+}
+
+static inline void scsi_set_bufflen(struct scsi_cmnd *cmd, int length)
+{
+	cmd->request_bufflen = length;
+}
+
 #define scsi_for_each_sg(cmd, sg, nseg, __i)			\
 	for (__i = 0, sg = scsi_sglist(cmd); __i < (nseg); __i++, (sg)++)
Signature · 1 line
1.5.0.4.402.g8035
author
#1 lore raw
Quoted 64 lines · Boaz Harrosh

2 things

[1]
There are 2 files (see attached patch) that use scsi_befflen() & scsi_sg_count() as L-value. If I try the scsi_sgtable approach and do:

#define scsi_sg_count(cmd) ((cmd)->sg_table ? (cmd)->sg_table->sg_count : 0) #define scsi_sglist(cmd) ((cmd)->sg_table ? (cmd)->sg_table->sglist : NULL) #define scsi_bufflen(cmd) ((cmd)->sg_table ? (cmd)->sg_table->length : 0)

I get compilation errors. Attached solution is to add scsi_set_ accessors to these members. But it would be best to STOP these drivers from doing that ugly hack of modifying sg_count and bufflen.
(CC sign-off-by maintainers of the drivers in question. Files are drivers/scsi/tmscsim.c & drivers/scsi/stex.c )

see: http://www.bhalevy.com/open-osd/download/sgtable_bidi_varlen/
for the full bidi over scsi_sgtable solution

[2]
if I use __deprecated on request_buffer, request_bufflen, and use_sg with scsi_sgtable implementation Than I get below list of files complaining:
drivers/ata/libata-scsi.c
drivers/firewire/fw-sbp2.c drivers/infiniband/ulp/srp/ib_srp.c drivers/message/i2o/i2o_scsi.c
drivers/scsi/aacraid/aachba.c
drivers/scsi/lpfc/lpfc_scsi.c
drivers/scsi/aha152x.c
drivers/scsi/pcmcia/nsp_cs.c drivers/scsi/sym53c8xx_2/sym_glue.h drivers/scsi/sym53c8xx_2/sym_glue.c
drivers/scsi/ncr53c8xx.c
drivers/scsi/sd.c
drivers/scsi/sr.c
drivers/scsi/advansys.c
drivers/scsi/psi240i.c
drivers/scsi/aha1542.c
drivers/scsi/ips.c
drivers/scsi/fd_mcs.c
drivers/scsi/in2000.c
drivers/scsi/NCR5380.c
drivers/scsi/qla1280.c
drivers/scsi/seagate.c
drivers/scsi/dc395x.c
drivers/scsi/atp870u.c
drivers/scsi/gdth.c
drivers/scsi/ide-scsi.c
drivers/scsi/ppa.c
drivers/scsi/imm.c
drivers/scsi/hptiop.c
drivers/usb/image/microtek.c
drivers/usb/storage/protocol.c drivers/usb/storage/transport.c drivers/usb/storage/shuttle_usbat.c drivers/usb/storage/sddr09.c
drivers/usb/storage/freecom.c
drivers/usb/storage/isd200.c

and also these files from scsi-ml that need changing when implementation changes:
drivers/scsi/scsi.c
drivers/scsi/scsi_error.c
drivers/scsi/scsi_debug.c

Quoted 11 lines

(see: 0004-Convert-scsi-ml-to-use-of-new-scsi_sgtable.patch at scsi_cmnd.h)

Which of the files do you have pending patches for? Which do you need that I send what I have for them?

Thanks
Boaz

Attached patch was wrong in regard to sign of set_ parameters. See above URL for a fixed version. Sorry
Boaz

#2 lore raw
In reply to Boaz Harrosh

From: Boaz Harrosh <[email protected]>
Subject: scsi_cmnd accessors issues
Date: Tue, 12 Jun 2007 21:02:20 +0300

Quoted 3 lines

[2]
if I use __deprecated on request_buffer, request_bufflen, and use_sg with scsi_sgtable implementation Than I get below list of files complaining:

(snip)

Quoted 9 lines

and also these files from scsi-ml that need changing when implementation changes:
drivers/scsi/scsi.c
drivers/scsi/scsi_error.c
drivers/scsi/scsi_debug.c

(see: 0004-Convert-scsi-ml-to-use-of-new-scsi_sgtable.patch at scsi_cmnd.h)

Which of the files do you have pending patches for? Which do you need that I send what I have for them?

I don't think that SCSI-ml bidi will be got into 2.6.23. 2.6.23 will convert as many LLDs as possible. I don't think that we need such patches in scsi-misc or scsi-pending now.

We need to agree on what the scsi bidi looks like first. When we add bidi support to scsi core, we can change scsi core (scsi.c, etc) together.

author
#3 lore raw

From: FUJITA Tomonori [mailto:[email protected]]
Sent: Tue 6/12/2007 7:51 PM
To: Harrosh, Boaz
Cc: [email protected]; [email protected]; [email protected]; [email protected]
Subject: Re: scsi_cmnd accessors issues

Quoted 20 lines

From: Boaz Harrosh <[email protected]>
Subject: scsi_cmnd accessors issues
Date: Tue, 12 Jun 2007 21:02:20 +0300

Quoted 3 lines

[2]
if I use __deprecated on request_buffer, request_bufflen, and use_sg with scsi_sgtable implementation Than I get below list of files complaining:

(snip)

Quoted 9 lines

and also these files from scsi-ml that need changing when implementation changes:
drivers/scsi/scsi.c
drivers/scsi/scsi_error.c
drivers/scsi/scsi_debug.c

(see: 0004-Convert-scsi-ml-to-use-of-new-scsi_sgtable.patch at scsi_cmnd.h)

Which of the files do you have pending patches for? Which do you need that I send what I have for them?

Quoted 7 lines

I don't think that SCSI-ml bidi will be got into 2.6.23. 2.6.23 will convert as many LLDs as possible. I don't think that we need such patches in scsi-misc or scsi-pending now.

We need to agree on what the scsi bidi looks like first. When we add bidi support to scsi core, we can change scsi core (scsi.c, etc) together.

OK! But what about the _set_ accessors what are we doing with these? are we fixing the code or are we adding new _set_ accessors?

Boaz

#4 lore raw

From: "Harrosh, Boaz" <[email protected]>
Subject: RE: scsi_cmnd accessors issues
Date: Wed, 13 Jun 2007 06:32:23 -0400

Quoted 30 lines

From: FUJITA Tomonori [mailto:[email protected]]
Sent: Tue 6/12/2007 7:51 PM
To: Harrosh, Boaz
Cc: [email protected]; [email protected]; [email protected]; [email protected]
Subject: Re: scsi_cmnd accessors issues

Quoted 24 lines

From: Boaz Harrosh <[email protected]>
Subject: scsi_cmnd accessors issues
Date: Tue, 12 Jun 2007 21:02:20 +0300

Quoted 3 lines

[2]
if I use __deprecated on request_buffer, request_bufflen, and use_sg with scsi_sgtable implementation Than I get below list of files complaining:

(snip)

Quoted 9 lines

and also these files from scsi-ml that need changing when implementation changes:
drivers/scsi/scsi.c
drivers/scsi/scsi_error.c
drivers/scsi/scsi_debug.c

(see: 0004-Convert-scsi-ml-to-use-of-new-scsi_sgtable.patch at scsi_cmnd.h)

Which of the files do you have pending patches for? Which do you need that I send what I have for them?

I don't think that SCSI-ml bidi will be got into 2.6.23. 2.6.23 will convert as many LLDs as possible. I don't think that we need such patches in scsi-misc or scsi-pending now.

Quoted 6 lines
Quoted 3 lines

We need to agree on what the scsi bidi looks like first. When we add bidi support to scsi core, we can change scsi core (scsi.c, etc) together.

OK! But what about the _set_ accessors what are we doing with these? are we fixing the code or are we adding new _set_ accessors?

There are just two llds. I suspect that we can fix them. Or they can access to the data directly (without the accessors) if we can't.

I don't think that introducing something like scsi_set_sg_count, which is meaningless to most of llds, is a good idea.

author
#5 lore raw

cover scsi_cmnd accessors issues

Quoted 6 lines · FUJITA Tomonori

There are just two llds. I suspect that we can fix them. Or they can access to the data directly (without the accessors) if we can't.

I don't think that introducing something like scsi_set_sg_count, which is meaningless to most of llds, is a good idea.

Following are 3 patches for fixing the l-value issues for accessors.

0/1 - proposed new accessors which will not let l-value code compile. 0/2 - Clean fix of drivers/scsi/tmscsim.c to not need setting of sg_count 0/3 - In drivers/scsi/stex.c open code of write access to request_bufflen.
when new code is committed this driver will have to change with it.

author
#6 lore raw

Re: [PATCH 1/3] Restrict scsi accessors access to read-only

Quoted 1 line

From 33dff41f5aaa05efd4aca80e12be4aaf4692a113 Mon Sep 17 00:00:00 2001

From: Boaz Harrosh <bharrosh@bh-buildlin2.(none)>
Date: Thu, 14 Jun 2007 18:55:18 +0300
Subject: [PATCH] Restrict scsi accessors access to read-only

- Users of scsi_cmnd might not change sg_count bufflen or sglist. So reflect that in code.

---
 include/scsi/scsi_cmnd.h |   17 ++++++++++++++---
 1 files changed, 14 insertions(+), 3 deletions(-)
include/scsi/scsi_cmnd.h +14 -3
index 53e1705..aaf8282 100644
@@ -138,9 +138,20 @@ extern void scsi_free_sgtable(struct scatterlist *, int);
 extern int scsi_dma_map(struct scsi_cmnd *cmd);
 extern void scsi_dma_unmap(struct scsi_cmnd *cmd);
 
-#define scsi_sg_count(cmd) ((cmd)->use_sg)
-#define scsi_sglist(cmd) ((struct scatterlist *)(cmd)->request_buffer)
-#define scsi_bufflen(cmd) ((cmd)->request_bufflen)
+static inline unsigned scsi_sg_count(struct scsi_cmnd *cmd)
+{
+	return cmd->use_sg;
+}
+
+static inline struct scatterlist *scsi_sglist(struct scsi_cmnd *cmd)
+{
+	return (struct scatterlist *)cmd->request_buffer;
+}
+
+static inline unsigned scsi_bufflen(struct scsi_cmnd *cmd)
+{
+	return cmd->request_bufflen;
+}
 
 static inline void scsi_set_resid(struct scsi_cmnd *cmd, int resid)
 {
Signature · 1 line
1.5.0.4.402.g8035
#7 lore raw

Re: [PATCH 1/3] Restrict scsi accessors access to read-only

include/scsi/scsi_cmnd.h extern void scsi_free_sgtable(struct scatterlist *, int); · line 149

Quoted 4 lines · Boaz Harrosh
include/scsi/scsi_cmnd.h +4 -0
+static inline struct scatterlist *scsi_sglist(struct scsi_cmnd *cmd)
+{
+	return (struct scatterlist *)cmd->request_buffer;
+}

It is quite nice to have this accessor, especially.

Jeff

author
#8 lore raw

Re: [PATCH 2/3] Farther clean-up of tmscsim driver

In reply to Boaz Harrosh
Quoted 1 line

From 4a7ac954dcc11531a09fa07d6a6365d98c67b216 Mon Sep 17 00:00:00 2001

From: Boaz Harrosh <bharrosh@bh-buildlin2.(none)>
Date: Thu, 14 Jun 2007 19:04:17 +0300
Subject: [PATCH] Farther clean-up of tmscsim driver

- this is a followup of commit 85289f2efa108d1586a86d0c426ffc9d641bbdc2 [SCSI] tmscsim: convert to use the data buffer accessors. The saved sg_count was a leftover from the time the driver was doing dma mapping by himself. But now that scsi-ml is called for the mapping it is not the drivers responsibility.

---
 drivers/scsi/tmscsim.c |    3 ---
 drivers/scsi/tmscsim.h |    1 -
 2 files changed, 0 insertions(+), 4 deletions(-)
drivers/scsi/tmscsim.c +0 -0
index 73c5ca0..5758f20 100644
@@ -1680,7 +1680,6 @@ dc390_RequestSense(struct dc390_acb* pACB, struct dc390_dcb*

pDCB, struct dc390_
pcmd->cmnd[0], pDCB->TargetID, pDCB->TargetLUN));

pSRB->SRBFlag |= AUTO_REQSENSE;
- pSRB->SavedSGCount = scsi_sg_count(pcmd);
pSRB->SavedTotXLen = pSRB->TotalXferredLen;
pSRB->AdaptStatus = 0;
pSRB->TargetStatus = 0; /* CHECK_CONDITION<<1; */
@@ -1728,7 +1727,6 @@ dc390_SRBdone( struct dc390_acb* pACB, struct dc390_dcb* pDCB, struct dc390_srb*
(u32) pcmd->result, (u32) pSRB->TotalXferredLen));
} else {
SET_RES_DRV(pcmd->result, DRIVER_SENSE);
- scsi_sg_count(pcmd) = pSRB->SavedSGCount;
//pSRB->ScsiCmdLen = (u8) (pSRB->Segment1[0] >> 8);
DEBUG0 (printk ("DC390: RETRY pid %li (%02x), target %02i-%02i\n", pcmd->pid,
pcmd->cmnd[0], pcmd->device->id, pcmd->device->lun));
pSRB->TotalXferredLen = 0;
@@ -1750,7 +1748,6 @@ dc390_SRBdone( struct dc390_acb* pACB, struct dc390_dcb* pDCB, struct dc390_srb*
else if( status_byte(status) == QUEUE_FULL )
{
scsi_track_queue_full(pcmd->device, pDCB->GoingSRBCnt - 1);
- scsi_sg_count(pcmd) = pSRB->SavedSGCount;
DEBUG0 (printk ("DC390: RETRY pid %li (%02x), target %02i-%02i\n", pcmd->pid,
pcmd->cmnd[0], pcmd->device->id, pcmd->device->lun));
pSRB->TotalXferredLen = 0;
SET_RES_DID(pcmd->result, DID_SOFT_ERROR);

drivers/scsi/tmscsim.h +0 -1
index c3d8c80..fbe7a3e 100644
@@ -57,7 +57,6 @@ u8		SGcount;
 
 u8		MsgCnt;
 u8		EndMessage;
-u8		SavedSGCount;			
 
 u8		MsgInBuf[6];
 u8		MsgOutBuf[6];
Signature · 1 line
1.5.0.4.402.g8035
#9 lore raw

Re: [PATCH 2/3] Farther clean-up of tmscsim driver

Quoted 10 lines · Boaz Harrosh
Quoted 1 line

From 4a7ac954dcc11531a09fa07d6a6365d98c67b216 Mon Sep 17 00:00:00 2001

From: Boaz Harrosh <bharrosh@bh-buildlin2.(none)>
Date: Thu, 14 Jun 2007 19:04:17 +0300
Subject: [PATCH] Farther clean-up of tmscsim driver

- this is a followup of commit 85289f2efa108d1586a86d0c426ffc9d641bbdc2 [SCSI] tmscsim: convert to use the data buffer accessors. The saved sg_count was a leftover from the time the driver was doing dma mapping by himself. But now that scsi-ml is called for the mapping it is not the drivers responsibility.

Apart from being line-wrapped and missing a "Signed-off-by" tag, this patch looks ok to me. Please resend properly formatted and then you can add my

Signed-off-by G. Liakhovetski <[email protected]>

Thanks
Guennadi

Quoted 53 lines
---
 drivers/scsi/tmscsim.c |    3 ---
 drivers/scsi/tmscsim.h |    1 -
 2 files changed, 0 insertions(+), 4 deletions(-)
drivers/scsi/tmscsim.c +0 -0
index 73c5ca0..5758f20 100644
@@ -1680,7 +1680,6 @@ dc390_RequestSense(struct dc390_acb* pACB, struct dc390_dcb*

pDCB, struct dc390_

drivers/scsi/tmscsim.c +0 -1
 			      pcmd->cmnd[0], pDCB->TargetID, pDCB->TargetLUN));
 
 	pSRB->SRBFlag |= AUTO_REQSENSE;
-	pSRB->SavedSGCount = scsi_sg_count(pcmd);
 	pSRB->SavedTotXLen = pSRB->TotalXferredLen;
 	pSRB->AdaptStatus = 0;
 	pSRB->TargetStatus = 0; /* CHECK_CONDITION<<1; */

@@ -1728,7 +1727,6 @@ dc390_SRBdone( struct dc390_acb* pACB, struct dc390_dcb*

pDCB, struct dc390_srb*

drivers/scsi/tmscsim.c +0 -1
 		       (u32) pcmd->result, (u32) pSRB->TotalXferredLen));
 	    } else {
 		SET_RES_DRV(pcmd->result, DRIVER_SENSE);
-		scsi_sg_count(pcmd) = pSRB->SavedSGCount;
 		//pSRB->ScsiCmdLen	 = (u8) (pSRB->Segment1[0] >> 8);
 		DEBUG0 (printk ("DC390: RETRY pid %li (%02x), target %02i-%02i\n", pcmd->pid,

pcmd->cmnd[0], pcmd->device->id, pcmd->device->lun));

drivers/scsi/tmscsim.c +0 -0
 		pSRB->TotalXferredLen = 0;

@@ -1750,7 +1748,6 @@ dc390_SRBdone( struct dc390_acb* pACB, struct dc390_dcb*

pDCB, struct dc390_srb*

drivers/scsi/tmscsim.c +0 -1
 	else if( status_byte(status) == QUEUE_FULL )
 	{
 	    scsi_track_queue_full(pcmd->device, pDCB->GoingSRBCnt - 1);
-	    scsi_sg_count(pcmd) = pSRB->SavedSGCount;
 	    DEBUG0 (printk ("DC390: RETRY pid %li (%02x), target %02i-%02i\n", pcmd->pid,

pcmd->cmnd[0], pcmd->device->id, pcmd->device->lun));

drivers/scsi/tmscsim.c +0 -0
 	    pSRB->TotalXferredLen = 0;
 	    SET_RES_DID(pcmd->result, DID_SOFT_ERROR);
drivers/scsi/tmscsim.h +0 -1
index c3d8c80..fbe7a3e 100644
@@ -57,7 +57,6 @@ u8		SGcount;
 
 u8		MsgCnt;
 u8		EndMessage;
-u8		SavedSGCount;			
 
 u8		MsgInBuf[6];
 u8		MsgOutBuf[6];
Signature · 1 line
1.5.0.4.402.g8035

---
Guennadi Liakhovetski

author
#10 lore raw

Re: [PATCH 3/3] Farther clean up of stex.c driver

In reply to Boaz Harrosh
Quoted 1 line

From 2b82909202cab8dc35184daef45b4b388f93112a Mon Sep 17 00:00:00 2001

From: Boaz Harrosh <bharrosh@bh-buildlin2.(none)>
Date: Thu, 14 Jun 2007 19:14:40 +0300
Subject: [PATCH] Farther clean up of stex.c driver

- now that scsi-ml accessors do not allow modifying of sg_count bufflen and sglist. The code in question is open coded to directly hack the scsi_cmnd structure. When implementation changes the driver will need to change with it.

- Maintainer of this driver should please review again if we absolutely need this read-sense length fixing. What if less bytes are read and 0 are left at the end. Also no check is made if the buffer is large enough.

---
 drivers/scsi/stex.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
drivers/scsi/stex.c +1 -1
index adda296..d784b58 100644
@@ -719,7 +719,7 @@ static void stex_ys_commands(struct st_hba *hba,
 
 	if (ccb->cmd->cmnd[0] == MGT_CMD &&
 		resp->scsi_status != SAM_STAT_CHECK_CONDITION) {
-		scsi_bufflen(ccb->cmd) =
+		ccb->cmd->request_bufflen =
 			le32_to_cpu(*(__le32 *)&resp->variable[0]);
 		return;
 	}
Signature · 1 line
1.5.0.4.402.g8035
author
#11 lore raw
In reply to Boaz Harrosh
Quoted 15 lines · Boaz Harrosh
Quoted 5 lines · FUJITA Tomonori

There are just two llds. I suspect that we can fix them. Or they can access to the data directly (without the accessors) if we can't.

I don't think that introducing something like scsi_set_sg_count, which is meaningless to most of llds, is a good idea.

Following are 3 patches for fixing the l-value issues for accessors.

0/1 - proposed new accessors which will not let l-value code compile. 0/2 - Clean fix of drivers/scsi/tmscsim.c to not need setting of sg_count 0/3 - In drivers/scsi/stex.c open code of write access to request_bufflen.
when new code is committed this driver will have to change with it.

Quoted 4 lines

-
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Hi Tomo.

I do not understand.
- Do you not accept my first patch. This is to go in now for 2.6.23. It is for protection against out-of-tree and the rest of the not converted drivers. To not use new accessors as l-value.

- Guennadi have you had a look at second patch it is clear that SavedSGCount is no longer used and that scsi_sg_count(pcmd) needs not be saved since it never changes.

- Tomo, Ed. The 3rd patch was sent so the first patch can be accepted now. As stated, it is a fix to the first cleanup patch that wrongfully used scsi_bufflen as l-value.

Boaz

#12 lore raw

From: Boaz Harrosh <[email protected]>
Subject: Re: [PATCH 0/0] scsi_cmnd accessors issues
Date: Sun, 17 Jun 2007 12:45:17 +0300

Quoted 29 lines
Quoted 15 lines · Boaz Harrosh
Quoted 5 lines · FUJITA Tomonori

There are just two llds. I suspect that we can fix them. Or they can access to the data directly (without the accessors) if we can't.

I don't think that introducing something like scsi_set_sg_count, which is meaningless to most of llds, is a good idea.

Following are 3 patches for fixing the l-value issues for accessors.

0/1 - proposed new accessors which will not let l-value code compile. 0/2 - Clean fix of drivers/scsi/tmscsim.c to not need setting of sg_count 0/3 - In drivers/scsi/stex.c open code of write access to request_bufflen.
when new code is committed this driver will have to change with it.

Quoted 4 lines

-
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Hi Tomo.

I do not understand.
- Do you not accept my first patch. This is to go in now for 2.6.23. It is for protection against out-of-tree and the rest of the not converted drivers. To not use new accessors as l-value.

Quoted 7 lines

- Guennadi have you had a look at second patch it is clear that SavedSGCount is no longer used and that scsi_sg_count(pcmd) needs not be saved since it never changes.

- Tomo, Ed. The 3rd patch was sent so the first patch can be accepted now. As stated, it is a fix to the first cleanup patch that wrongfully used scsi_bufflen as l-value.

As Ed (and I) said, the 3rd patch is temporary.

Now we will change:

- scsi_bufflen(ccb->cmd) =
+ ccb->cmd->request_bufflen =
le32_to_cpu(*(__le32 *)&resp->variable[0]);

Then with the bidi support, we will change again:

		if (ccb->sg_table) {
			ccb->sg_table->length = 
	 			le32_to_cpu(*(__le32 *)&resp->variable[0]);

We can live without the first patch. We don't need to care about out-of-tree drivers and we can handle the rest of the not-converted drivers.

So I prefer applying the first patch with the bidi support and the proper stex fix than putting the temporary third patch now thought it doesn't matter much.

I'm not sure why you think that the first patch is so important and we must push it to 2.6.23.

author
#13 lore raw
Quoted 36 lines · FUJITA Tomonori
Quoted 8 lines

Boaz Harrosh wrote:
Hi Tomo.

I do not understand.
- Do you not accept my first patch. This is to go in now for 2.6.23. It is for protection against out-of-tree and the rest of the not converted drivers. To not use new accessors as l-value.

Quoted 7 lines

- Guennadi have you had a look at second patch it is clear that SavedSGCount is no longer used and that scsi_sg_count(pcmd) needs not be saved since it never changes.

- Tomo, Ed. The 3rd patch was sent so the first patch can be accepted now. As stated, it is a fix to the first cleanup patch that wrongfully used scsi_bufflen as l-value.

As Ed (and I) said, the 3rd patch is temporary.

Now we will change:

(quoted diff) +1 -1
-		scsi_bufflen(ccb->cmd) =
+		ccb->cmd->request_bufflen =

			le32_to_cpu(*(__le32 *)&resp->variable[0]);

Then with the bidi support, we will change again:

		if (ccb->sg_table) {
			ccb->sg_table->length = 
	 			le32_to_cpu(*(__le32 *)&resp->variable[0]);

We can live without the first patch. We don't need to care about out-of-tree drivers and we can handle the rest of the not-converted drivers.

Quoted 6 lines

So I prefer applying the first patch with the bidi support and the proper stex fix than putting the temporary third patch now thought it doesn't matter much.

I'm not sure why you think that the first patch is so important and we must push it to 2.6.23.

I think first patch is so important because accessors were wrongly defined to look like l-value which they must not be. Currently code is only in scsi-misc tree and was never in any official Linux tree. Once it will go into 2.6.23-rcx it will be official. And it better be right. We do not know how long it will stay the way it is, it can be for a long time. The way I see it, this is the last chance we have to fix it, before it goes in. Than it will be to late.

The 3rd patch is not a temporary hack. It is a "Revert". You submitted a patch that wrongfully changed:
- ccb->cmd->request_bufflen =
+ scsi_bufflen(ccb->cmd) =
le32_to_cpu(*(__le32 *)&resp->variable[0]);

I'm simply reverting it to what it is originally in 2.6.22 .

Boaz

#14 lore raw
In reply to Boaz Harrosh

From: Boaz Harrosh <[email protected]>
Subject: scsi_cmnd accessors issues
Date: Tue, 12 Jun 2007 21:02:20 +0300

Oops, I forgot to answer the status of lld conversion.

Quoted 5 lines

[2]
if I use __deprecated on request_buffer, request_bufflen, and use_sg with scsi_sgtable implementation Than I get below list of files complaining:
drivers/ata/libata-scsi.c
drivers/firewire/fw-sbp2.c

I've not converted yet.

Quoted 1 line

drivers/infiniband/ulp/srp/ib_srp.c

In scsi-pending tree and waits for ACKs.

Quoted 1 line

drivers/message/i2o/i2o_scsi.c

I've not converted yet.

Quoted 1 line

drivers/scsi/aacraid/aachba.c

In scsi-pending tree and got the ACK (will be moved to scsi-misc).

Quoted 1 line

drivers/scsi/lpfc/lpfc_scsi.c

In scsi-pending tree but Emulex reworked this patch. We can drop this.

Quoted 2 lines

drivers/scsi/aha152x.c
drivers/scsi/pcmcia/nsp_cs.c

I've not converted yet.

Quoted 2 lines

drivers/scsi/sym53c8xx_2/sym_glue.h
drivers/scsi/sym53c8xx_2/sym_glue.c

The patch was in my old bidi tree but seems that it's in neither scsi-misc nor scsi-pending. I'll resend this.

Quoted 1 line

drivers/scsi/ncr53c8xx.c

In scsi-pending tree and waits for ACKs.

Quoted 3 lines

drivers/scsi/advansys.c
drivers/scsi/psi240i.c
drivers/scsi/aha1542.c

I've not converted yet.

Quoted 1 line

drivers/scsi/ips.c

The maintainer pointed out that the initial patch has problems. I sent an updated patch and waits for the ACK.

Quoted 5 lines

drivers/scsi/fd_mcs.c
drivers/scsi/in2000.c
drivers/scsi/NCR5380.c
drivers/scsi/qla1280.c
drivers/scsi/seagate.c

I've not converted yet.

Quoted 1 line

drivers/scsi/dc395x.c

In scsi-pending tree and waits for ACKs.

Quoted 5 lines

drivers/scsi/atp870u.c
drivers/scsi/gdth.c
drivers/scsi/ide-scsi.c
drivers/scsi/ppa.c
drivers/scsi/imm.c

I've not converted yet.

Quoted 1 line

drivers/scsi/hptiop.c

In scsi-pending tree and got ACKs (will be moved to scsi-misc).

Quoted 7 lines

drivers/usb/image/microtek.c
drivers/usb/storage/protocol.c
drivers/usb/storage/transport.c
drivers/usb/storage/shuttle_usbat.c
drivers/usb/storage/sddr09.c
drivers/usb/storage/freecom.c
drivers/usb/storage/isd200.c

I've not converted yet.

#15 lore raw

From: FUJITA Tomonori <[email protected]>
Subject: Re: scsi_cmnd accessors issues
Date: Wed, 13 Jun 2007 20:24:42 +0900

Quoted 5 lines
Quoted 2 lines

drivers/scsi/sym53c8xx_2/sym_glue.h
drivers/scsi/sym53c8xx_2/sym_glue.c

The patch was in my old bidi tree but seems that it's in neither scsi-misc nor scsi-pending. I'll resend this.

Oops, this is in scsi-pending tree.

Here's the summary.

---
o Not converted yet

Quoted 16 lines

drivers/ata/libata-scsi.c
drivers/firewire/fw-sbp2.c
drivers/scsi/advansys.c
drivers/scsi/psi240i.c
drivers/scsi/aha1542.c
drivers/scsi/fd_mcs.c
drivers/scsi/in2000.c
drivers/scsi/NCR5380.c
drivers/scsi/qla1280.c
drivers/scsi/seagate.c
drivers/scsi/atp870u.c
drivers/scsi/gdth.c
drivers/scsi/ide-scsi.c
drivers/scsi/ppa.c
drivers/scsi/imm.c
drivers/scsi/aha152x.c

Quoted 8 lines

drivers/scsi/pcmcia/nsp_cs.c
drivers/usb/image/microtek.c
drivers/usb/storage/protocol.c
drivers/usb/storage/transport.c
drivers/usb/storage/shuttle_usbat.c
drivers/usb/storage/sddr09.c
drivers/usb/storage/freecom.c
drivers/usb/storage/isd200.c

o In scsi-pending tree and waits for ACKs

Quoted 5 lines

drivers/infiniband/ulp/srp/ib_srp.c
drivers/scsi/sym53c8xx_2/sym_glue.h
drivers/scsi/sym53c8xx_2/sym_glue.c
drivers/scsi/ncr53c8xx.c
drivers/scsi/ibmvscsi/ibmvscsi.c

o In scsi-pending tree and got the ACKs

Quoted 5 lines

drivers/scsi/aacraid/aachba.c
drivers/scsi/hptiop.c
drivers/scsi/mesh.c
drivers/scsi/esp_scsi.c
drivers/scsi/dc395x.c

o Not in scsi-pending tree and waits for ACKs

Quoted 5 lines

drivers/scsi/iscsi_tcp.c
drivers/scsi/libiscsi.c
drivers/infiniband/ulp/iser/iscsi_iser.c
drivers/infiniband/ulp/iser/iser_initiator.c
drivers/message/i2o/i2o_scsi.c

o Not in scsi-pending tree and got the ACKs

Quoted 1 line

drivers/scsi/ips.c

o In scsi-pending tree but not necessary now

Quoted 1 line

drivers/scsi/lpfc/lpfc_scsi.c

#16 lore raw
In reply to Boaz Harrosh

Hi

Quoted 5 lines · Boaz Harrosh

I get compilation errors. Attached solution is to add scsi_set_ accessors to these members. But it would be best to STOP these drivers from doing that ugly hack of modifying sg_count and bufflen. (CC sign-off-by maintainers of the drivers in question. Files are drivers/scsi/tmscsim.c & drivers/scsi/stex.c )

Thanks for the patch. I think, getting rid of the SavedSGCount field is a bit more complicated (would be better if you had sent the patch inline). I'm currently working on a solution, please, give me some time.

Thanks
Guennadi
---
Guennadi Liakhovetski