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

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

[Ed Lin]

> -----Original Message-----
> From: FUJITA Tomonori [mailto:fujita.tomonori@lab.ntt.co.jp] 
> Sent: Thursday, June 14, 2007 3:21 PM
> To: Ed Lin
> Cc: bharrosh@panasas.com; fujita.tomonori@lab.ntt.co.jp; 
> g.liakhovetski@gmx.de; linux-scsi@vger.kernel.org
> Subject: RE: [PATCH 3/3] Farther clean up of stex.c driver
> 
> 
> From: "Ed Lin" <ed.lin@promise.com>
> Subject: RE: [PATCH 3/3] Farther clean up of stex.c driver
> Date: Thu, 14 Jun 2007 12:29:05 -0700
> 
> > 
> > 
> > > -----Original Message-----
> > > From: Boaz Harrosh [mailto:bharrosh@panasas.com] 
> > > Sent: Thursday, June 14, 2007 9:34 AM
> > > To: FUJITA Tomonori; Ed Lin; Guennadi Liakhovetski
> > > Cc: linux-scsi@vger.kernel.org
> > > Subject: Re: [PATCH 3/3] Farther clean up of stex.c driver
> > > 
> > > 
> > > 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
> > > 
> > > 
> > > 
> > 
> > The software allocates a big enough buffer(so don't worry 
> about this),
> > and it trims the data size based on returned length. So it needs the
> > actual data length.
> 
> What's the software? I mean that who trims the data size based on
> returned length?
> 

The management software provided by Promise.

> > So this length fix is necessary if software doesn't
> > change policy. The whole thing is guaranteed by both software and
> > firmware, software will do a check, firmware will do a 
> check, so a check
> > in driver is not necessary.
> 
> 
> > I wonder whether the following will go upstream:
> > #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)
> > 
> > For this particular case, because there is data exchange, so
> > (cmd)->sg_table is not NULL, so we can have something like:
> > 		ccb->cmd->sg_table->length =
> >   			le32_to_cpu(*(__le32 *)&resp->variable[0]);
> > Although I am not sure whether the software can work without change
> > after the scsi_sgtable approach becomes upstream. But I 
> just wonder why
> > we use some code that is known to be temporary even before its
> > introduction.
> 
> I think that it would be better to apply Boaz patchset with scsi-bidi
> support (not now). Then you can avoid adding temporary hack.
> 

Does the scsi_sgtable above belong to the scsi-bidi patchset? If it
does, then this patch is just a temporary hack. The code needs to be
changed again at that time. This is stated in the comment of this patch.

--Ed Lin

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

[FUJITA Tomonori]

From: "Ed Lin" <ed.lin@promise.com>
Subject: RE: [PATCH 3/3] Farther clean up of stex.c driver
Date: Thu, 14 Jun 2007 16:02:30 -0700

> 
> 
> > -----Original Message-----
> > From: FUJITA Tomonori [mailto:fujita.tomonori@lab.ntt.co.jp] 
> > Sent: Thursday, June 14, 2007 3:21 PM
> > To: Ed Lin
> > Cc: bharrosh@panasas.com; fujita.tomonori@lab.ntt.co.jp; 
> > g.liakhovetski@gmx.de; linux-scsi@vger.kernel.org
> > Subject: RE: [PATCH 3/3] Farther clean up of stex.c driver
> > 
> > 
> > From: "Ed Lin" <ed.lin@promise.com>
> > Subject: RE: [PATCH 3/3] Farther clean up of stex.c driver
> > Date: Thu, 14 Jun 2007 12:29:05 -0700
> > 
> > > 
> > > 
> > > > -----Original Message-----
> > > > From: Boaz Harrosh [mailto:bharrosh@panasas.com] 
> > > > Sent: Thursday, June 14, 2007 9:34 AM
> > > > To: FUJITA Tomonori; Ed Lin; Guennadi Liakhovetski
> > > > Cc: linux-scsi@vger.kernel.org
> > > > Subject: Re: [PATCH 3/3] Farther clean up of stex.c driver
> > > > 
> > > > 
> > > > 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
> > > > 
> > > > 
> > > > 
> > > 
> > > The software allocates a big enough buffer(so don't worry 
> > about this),
> > > and it trims the data size based on returned length. So it needs the
> > > actual data length.
> > 
> > What's the software? I mean that who trims the data size based on
> > returned length?
> > 
> 
> The management software provided by Promise.

Any chance to use the resid field to tell a returned length to the
software?


> > > I wonder whether the following will go upstream:
> > > #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)
> > > 
> > > For this particular case, because there is data exchange, so
> > > (cmd)->sg_table is not NULL, so we can have something like:
> > > 		ccb->cmd->sg_table->length =
> > >   			le32_to_cpu(*(__le32 *)&resp->variable[0]);
> > > Although I am not sure whether the software can work without change
> > > after the scsi_sgtable approach becomes upstream. But I 
> > just wonder why
> > > we use some code that is known to be temporary even before its
> > > introduction.
> > 
> > I think that it would be better to apply Boaz patchset with scsi-bidi
> > support (not now). Then you can avoid adding temporary hack.
> > 
> 
> Does the scsi_sgtable above belong to the scsi-bidi patchset?

Yeah.

> If it does, then this patch is just a temporary hack.

Yeah. That's why I prefer to apply this patch with the scsi-bidi
patchset as I said in the previous mail.


> The code needs to be changed again at that time. This is stated in
> the comment of this patch.

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

[Boaz Harrosh]

Ed Lin wrote:
> 
>>>>
>>> The software allocates a big enough buffer(so don't worry about this),
>>> and it trims the data size based on returned length. So it needs the
>>> actual data length.

This sounds like a security breach to me. An untrusted user-mode
with knowledge of such hardware in the machine can gain access to kernel
through the wrong( cracker-right) sized request.

>> What's the software? I mean that who trims the data size based on
>> returned length?
>>
> 
> The management software provided by Promise.
> 

I had a deeper look into this, and I am totally confused.
The code in question at stex.c::stex_ys_commands() is called
at softirq from stex_mu_intr(). It will than immediately go 
on to eventually call cmd->scsi_done(cmd);
Now what is the upper layer that calls this command??!

There can be 3 possibilities that I know. From unlikely to likely order:
1. REQ_TYPE_FS command which is not possible because I don't know
   of any one that can issue. a ccb->cmd->cmnd[0] == MGT_CMD
   as an REQ_TYPE_FS. And anyway if bufflen is truncated like that
   a REQ_TYPE_FS command will retry the command until complete.

2. REQ_TYPE_BLOCK_PC through sg, bsg or others at which truncating
   a command like that will leak BIOs when trying to release the 
   command, since scsi-ml relies on bufflen in the call to 
   end_that_request_first.

3. A special upper layer that overrides the done functions at 
   scsi_cmnd or request levels and interprets the return info
   and/or frees buffers. If this is the case, can such a driver
   use a reserved member of scsi_cmnd like the fields in 
   struct scsi_pointer or something hanging on host_scribble?
   Than later return to user-mode what it used to return before
   but leave bufflen alone?

Setting of bufflen like that in a driver sounds like a highway
rubbery to me. Please stop that Hack. Now!

>>> So this length fix is necessary if software doesn't
>>> change policy. The whole thing is guaranteed by both software and
>>> firmware, software will do a check, firmware will do a check, 
>>> so a check in driver is not necessary.


As I said. I'm sure we can change upper layer polices to not
break user-mode but to also not touch a delicate and private member
of scsi-ml. It limits the scsi-ml ability to change and evolve.
The most I can do for now is declare this driver broken once 
scsi_sgtable goes in because I sure am not going to do:
ccb->cmd->sg_table->length = le32_to_cpu(*(__le32 *)&resp->variable[0]);
(Do we need an explanation why this is plain broken, leaking code)

If you point me to the upper layer driver used, I can see if I can change it
in a way that will not touch bufflen.

Another thing I do not understand is where in the SCSI standard
such an operation is defined. because if it is a standard SCSI
command or extended or vendor-specific command than, current code
is not built to cope with such scenario. How would one defined
it? a "truncated read"?

Boaz Harrosh

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

[Lin Yu]

> -----Original Message-----
> From: linux-scsi-owner@vger.kernel.org 
> [mailto:linux-scsi-owner@vger.kernel.org] On Behalf Of Boaz Harrosh
> Sent: Sunday, July 01, 2007 6:56 AM
> To: Ed Lin; FUJITA Tomonori
> Cc: g.liakhovetski@gmx.de; linux-scsi@vger.kernel.org; James Bottomley
> Subject: Re: [PATCH 3/3] Farther clean up of stex.c driver
> 
> 
> Ed Lin wrote:
> > 
> >>>>
> >>> The software allocates a big enough buffer(so don't worry 
> about this),
> >>> and it trims the data size based on returned length. So 
> it needs the
> >>> actual data length.
> 
> This sounds like a security breach to me. An untrusted user-mode
> with knowledge of such hardware in the machine can gain 
> access to kernel
> through the wrong( cracker-right) sized request.
> 
> >> What's the software? I mean that who trims the data size based on
> >> returned length?
> >>
> > 
> > The management software provided by Promise.
> > 
> 
> I had a deeper look into this, and I am totally confused.
> The code in question at stex.c::stex_ys_commands() is called
> at softirq from stex_mu_intr(). It will than immediately go 
> on to eventually call cmd->scsi_done(cmd);
> Now what is the upper layer that calls this command??!
> 
> There can be 3 possibilities that I know. From unlikely to 
> likely order:
> 1. REQ_TYPE_FS command which is not possible because I don't know
>    of any one that can issue. a ccb->cmd->cmnd[0] == MGT_CMD
>    as an REQ_TYPE_FS. And anyway if bufflen is truncated like that
>    a REQ_TYPE_FS command will retry the command until complete.
> 
> 2. REQ_TYPE_BLOCK_PC through sg, bsg or others at which truncating
>    a command like that will leak BIOs when trying to release the 
>    command, since scsi-ml relies on bufflen in the call to 
>    end_that_request_first.
> 
> 3. A special upper layer that overrides the done functions at 
>    scsi_cmnd or request levels and interprets the return info
>    and/or frees buffers. If this is the case, can such a driver
>    use a reserved member of scsi_cmnd like the fields in 
>    struct scsi_pointer or something hanging on host_scribble?
>    Than later return to user-mode what it used to return before
>    but leave bufflen alone?
> 
> Setting of bufflen like that in a driver sounds like a highway
> rubbery to me. Please stop that Hack. Now!
> 
> >>> So this length fix is necessary if software doesn't
> >>> change policy. The whole thing is guaranteed by both software and
> >>> firmware, software will do a check, firmware will do a check, 
> >>> so a check in driver is not necessary.
> 
> 
> As I said. I'm sure we can change upper layer polices to not
> break user-mode but to also not touch a delicate and private member
> of scsi-ml. It limits the scsi-ml ability to change and evolve.
> The most I can do for now is declare this driver broken once 
> scsi_sgtable goes in because I sure am not going to do:
> ccb->cmd->sg_table->length = le32_to_cpu(*(__le32 
> *)&resp->variable[0]);
> (Do we need an explanation why this is plain broken, leaking code)
> 
> If you point me to the upper layer driver used, I can see if 
> I can change it
> in a way that will not touch bufflen.
> 
> Another thing I do not understand is where in the SCSI standard
> such an operation is defined. because if it is a standard SCSI
> command or extended or vendor-specific command than, current code
> is not built to cope with such scenario. How would one defined
> it? a "truncated read"?
> 
> Boaz Harrosh
> 

We are investigating it. Please wait a while. We are also trying
to make this driver better.
Thanks for your findings and efforts.

Ed Lin

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

[Ed Lin]

> -----Original Message-----
> From: Boaz Harrosh [mailto:bharrosh@panasas.com] 
> Sent: Thursday, June 14, 2007 9:34 AM
> To: FUJITA Tomonori; Ed Lin; Guennadi Liakhovetski
> Cc: linux-scsi@vger.kernel.org
> Subject: Re: [PATCH 3/3] Farther clean up of stex.c driver
> 
> 
> 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
> 
> 
> 

The software allocates a big enough buffer(so don't worry about this),
and it trims the data size based on returned length. So it needs the
actual data length. So this length fix is necessary if software doesn't
change policy. The whole thing is guaranteed by both software and
firmware, software will do a check, firmware will do a check, so a check
in driver is not necessary.

I wonder whether the following will go upstream:
#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)

For this particular case, because there is data exchange, so
(cmd)->sg_table is not NULL, so we can have something like:
		ccb->cmd->sg_table->length =
  			le32_to_cpu(*(__le32 *)&resp->variable[0]);
Although I am not sure whether the software can work without change
after the scsi_sgtable approach becomes upstream. But I just wonder why
we use some code that is known to be temporary even before its
introduction.

--Ed Lin

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

[FUJITA Tomonori]

From: "Ed Lin" <ed.lin@promise.com>
Subject: RE: [PATCH 3/3] Farther clean up of stex.c driver
Date: Thu, 14 Jun 2007 12:29:05 -0700

> 
> 
> > -----Original Message-----
> > From: Boaz Harrosh [mailto:bharrosh@panasas.com] 
> > Sent: Thursday, June 14, 2007 9:34 AM
> > To: FUJITA Tomonori; Ed Lin; Guennadi Liakhovetski
> > Cc: linux-scsi@vger.kernel.org
> > Subject: Re: [PATCH 3/3] Farther clean up of stex.c driver
> > 
> > 
> > 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
> > 
> > 
> > 
> 
> The software allocates a big enough buffer(so don't worry about this),
> and it trims the data size based on returned length. So it needs the
> actual data length.

What's the software? I mean that who trims the data size based on
returned length?

> So this length fix is necessary if software doesn't
> change policy. The whole thing is guaranteed by both software and
> firmware, software will do a check, firmware will do a check, so a check
> in driver is not necessary.


> I wonder whether the following will go upstream:
> #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)
> 
> For this particular case, because there is data exchange, so
> (cmd)->sg_table is not NULL, so we can have something like:
> 		ccb->cmd->sg_table->length =
>   			le32_to_cpu(*(__le32 *)&resp->variable[0]);
> Although I am not sure whether the software can work without change
> after the scsi_sgtable approach becomes upstream. But I just wonder why
> we use some code that is known to be temporary even before its
> introduction.

I think that it would be better to apply Boaz patchset with scsi-bidi
support (not now). Then you can avoid adding temporary hack.

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

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