scsi_cmnd accessors issues

scsi_cmnd accessors issues

[Boaz Harrosh]

[-- Attachment #1: Type: text/plain, Size: 2242 bytes --]

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




[-- Attachment #2: 0003-Add-scsi_set_-accessors-to-bufflen-and-sg_count.patch --]
[-- Type: text/plain, Size: 3073 bytes --]

>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(-)

diff --git a/drivers/scsi/stex.c b/drivers/scsi/stex.c
index adda296..4795d53 100644
--- a/drivers/scsi/stex.c
+++ b/drivers/scsi/stex.c
@@ -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;
 	}
 
diff --git a/drivers/scsi/tmscsim.c b/drivers/scsi/tmscsim.c
index 73c5ca0..bfc718a 100644
--- a/drivers/scsi/tmscsim.c
+++ b/drivers/scsi/tmscsim.c
@@ -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);
diff --git a/include/scsi/scsi_cmnd.h b/include/scsi/scsi_cmnd.h
index 53e1705..d0c1101 100644
--- a/include/scsi/scsi_cmnd.h
+++ b/include/scsi/scsi_cmnd.h
@@ -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)++)
 
-- 
1.5.0.4.402.g8035

Re: scsi_cmnd accessors issues

[Boaz Harrosh]

Boaz Harrosh wrote:
> 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
> 
> 
> 
> 

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

Re: scsi_cmnd accessors issues

[FUJITA Tomonori]

From: Boaz Harrosh <bharrosh@panasas.com>
Subject: scsi_cmnd accessors issues
Date: Tue, 12 Jun 2007 21:02:20 +0300

> [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)

> 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.

RE: scsi_cmnd accessors issues

[Harrosh, Boaz]

From: FUJITA Tomonori [mailto:fujita.tomonori@lab.ntt.co.jp]
Sent: Tue 6/12/2007 7:51 PM
To: Harrosh, Boaz
Cc: linux-scsi@vger.kernel.org; fujita.tomonori@lab.ntt.co.jp; ed.lin@promise.com; g.liakhovetski@gmx.de
Subject: Re: scsi_cmnd accessors issues
 
> From: Boaz Harrosh <bharrosh@panasas.com>
> Subject: scsi_cmnd accessors issues
> Date: Tue, 12 Jun 2007 21:02:20 +0300
>
>> [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)
>
>> 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.

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

RE: scsi_cmnd accessors issues

[FUJITA Tomonori]

From: "Harrosh, Boaz" <bharrosh@panasas.com>
Subject: RE: scsi_cmnd accessors issues
Date: Wed, 13 Jun 2007 06:32:23 -0400

> From: FUJITA Tomonori [mailto:fujita.tomonori@lab.ntt.co.jp]
> Sent: Tue 6/12/2007 7:51 PM
> To: Harrosh, Boaz
> Cc: linux-scsi@vger.kernel.org; fujita.tomonori@lab.ntt.co.jp; ed.lin@promise.com; g.liakhovetski@gmx.de
> Subject: Re: scsi_cmnd accessors issues
>  
> > From: Boaz Harrosh <bharrosh@panasas.com>
> > Subject: scsi_cmnd accessors issues
> > Date: Tue, 12 Jun 2007 21:02:20 +0300
> >
> >> [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)
> >
> >> 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.
> 
> 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.

[PATCH 0/0] scsi_cmnd accessors issues

[Boaz Harrosh]

FUJITA Tomonori wrote:
> 
> 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.

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

[Boaz Harrosh]

>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(-)

diff --git a/include/scsi/scsi_cmnd.h b/include/scsi/scsi_cmnd.h
index 53e1705..aaf8282 100644
--- a/include/scsi/scsi_cmnd.h
+++ b/include/scsi/scsi_cmnd.h
@@ -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)
 {
-- 
1.5.0.4.402.g8035

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

[Jeff Garzik]

Boaz Harrosh wrote:
> +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

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

[Boaz Harrosh]

>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(-)

diff --git a/drivers/scsi/tmscsim.c b/drivers/scsi/tmscsim.c
index 73c5ca0..5758f20 100644
--- a/drivers/scsi/tmscsim.c
+++ b/drivers/scsi/tmscsim.c
@@ -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);
diff --git a/drivers/scsi/tmscsim.h b/drivers/scsi/tmscsim.h
index c3d8c80..fbe7a3e 100644
--- a/drivers/scsi/tmscsim.h
+++ b/drivers/scsi/tmscsim.h
@@ -57,7 +57,6 @@ u8		SGcount;

 u8		MsgCnt;
 u8		EndMessage;
-u8		SavedSGCount;			

 u8		MsgInBuf[6];
 u8		MsgOutBuf[6];
-- 
1.5.0.4.402.g8035

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

[Guennadi Liakhovetski]

On Thu, 14 Jun 2007, Boaz Harrosh wrote:

> >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 <g.liakhovetski@gmx.de>

Thanks
Guennadi

> ---
>  drivers/scsi/tmscsim.c |    3 ---
>  drivers/scsi/tmscsim.h |    1 -
>  2 files changed, 0 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/scsi/tmscsim.c b/drivers/scsi/tmscsim.c
> index 73c5ca0..5758f20 100644
> --- a/drivers/scsi/tmscsim.c
> +++ b/drivers/scsi/tmscsim.c
> @@ -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);
> diff --git a/drivers/scsi/tmscsim.h b/drivers/scsi/tmscsim.h
> index c3d8c80..fbe7a3e 100644
> --- a/drivers/scsi/tmscsim.h
> +++ b/drivers/scsi/tmscsim.h
> @@ -57,7 +57,6 @@ u8		SGcount;
> 
>  u8		MsgCnt;
>  u8		EndMessage;
> -u8		SavedSGCount;			
> 
>  u8		MsgInBuf[6];
>  u8		MsgOutBuf[6];
> -- 
> 1.5.0.4.402.g8035
> 

---
Guennadi Liakhovetski

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

[Boaz Harrosh]

>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(-)

diff --git a/drivers/scsi/stex.c b/drivers/scsi/stex.c
index adda296..d784b58 100644
--- a/drivers/scsi/stex.c
+++ b/drivers/scsi/stex.c
@@ -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;
 	}
-- 
1.5.0.4.402.g8035

Re: [PATCH 0/0] scsi_cmnd accessors issues

[Boaz Harrosh]

Boaz Harrosh wrote:
> FUJITA Tomonori wrote:
>> 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.
> 
> 
> -
> To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
> the body of a message to majordomo@vger.kernel.org
> 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

Re: [PATCH 0/0] scsi_cmnd accessors issues

[FUJITA Tomonori]

From: Boaz Harrosh <bharrosh@panasas.com>
Subject: Re: [PATCH 0/0] scsi_cmnd accessors issues
Date: Sun, 17 Jun 2007 12:45:17 +0300

> Boaz Harrosh wrote:
> > FUJITA Tomonori wrote:
> >> 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.
> > 
> > 
> > -
> > To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
> > the body of a message to majordomo@vger.kernel.org
> > 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.

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.

RE: [PATCH 0/0] scsi_cmnd accessors issues

[Harrosh, Boaz]

FUJITA Tomonori wrote:
>> 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.
>> 
>> - 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.

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

Re: scsi_cmnd accessors issues

[FUJITA Tomonori]

From: Boaz Harrosh <bharrosh@panasas.com>
Subject: scsi_cmnd accessors issues
Date: Tue, 12 Jun 2007 21:02:20 +0300

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

> [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.

> drivers/infiniband/ulp/srp/ib_srp.c

In scsi-pending tree and waits for ACKs.

> drivers/message/i2o/i2o_scsi.c

I've not converted yet.

> drivers/scsi/aacraid/aachba.c

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

> drivers/scsi/lpfc/lpfc_scsi.c

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

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

I've not converted yet.

> 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.

> drivers/scsi/ncr53c8xx.c

In scsi-pending tree and waits for ACKs.

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

I've not converted yet.

> drivers/scsi/ips.c

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

> 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.

> drivers/scsi/dc395x.c

In scsi-pending tree and waits for ACKs.

> 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.

> drivers/scsi/hptiop.c

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

> 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.

Re: scsi_cmnd accessors issues

[FUJITA Tomonori]

From: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
Subject: Re: scsi_cmnd accessors issues
Date: Wed, 13 Jun 2007 20:24:42 +0900

> > 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

> 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
> 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

> 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

> 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

> 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

> drivers/scsi/ips.c


o In scsi-pending tree but not necessary now

> drivers/scsi/lpfc/lpfc_scsi.c

Re: scsi_cmnd accessors issues

[Guennadi Liakhovetski]

Hi

On Tue, 12 Jun 2007, Boaz Harrosh wrote:

> 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