date:20071017

Re: Inquiry data and emulated SG devices

2007-10-17 Thread Jeff Garzik


Robert Hancock wrote:

This doesn't seem a very reliable way to identify an IDE device, as all
that 0 means is that the device does not claim conformance to any
standard. I would think it would be legitimate for an IDE device to put
a value like 5 in there as well, if it complies with SPC-4..


Via the this-doesnt-really-matter-but-it-should-be-noted department:

According to the latest on t10.org, MMC retroactively permitted SCSI 
version to be zero, for MMC-compliant USB and ATAPI devices.




In the case of libata though, that appears to be due to this code in
drivers/ata/libata-scsi.c:

/* ATAPI devices typically report zero for their SCSI version,
 * and sometimes deviate from the spec WRT response data
 * format.  If SCSI version is reported as zero like normal,
 * then we make the following fixups:  1) Fake MMC-5 version,
 * to indicate to the Linux scsi midlayer this is a modern
 * device.  2) Ensure response data format / ATAPI information
 * are always correct.
 */
if (buf[2] == 0) {
buf[2] = 0x5;
buf[3] = 0x32;
}

This technically seems to go against what the SCSI/ATA Translation (SAT)
spec says regarding INQUIRY on ATAPI devices: "the SATL shall use the
ATA PACKET Command feature set to pass all INQUIRY commands and
parameter data to the ATAPI device without altering the INQUIRY
commands or the parameter data." However, it might realistically be
needed if the SCSI layer in the kernel has problems with a device
indicating it supports no SCSI version..


The above tweak is entirely software->software communication...  as the 
comment you quoted notes, it's just a signal to the SCSI midlayer.


At the moment, the SCSI midlayer assumes any device that reports scsi 
version as less than 2 is forced to SCSI version 2.  Ultimately that's 
incorrect behavior for all ATAPI devices (and later MMC revisions).


At the time, libata simply worked around this SCSI buglet in its own 
code, since that was easier than auditing all SCSI code paths to ensure 
new ATAPI/USB MMC logic does not break ancient devices.


But if someone is motivated enough to revisit this...

Jeff


-
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

Re: [PATCH 27/32] scsi_data_buffer

2007-10-17 Thread Matthew Wilcox

On Wed, Oct 17, 2007 at 08:21:15PM +0200, Boaz Harrosh wrote:
>   - Group all IO members of scsi_cmnd into a scsi_data_buffer
> structure.

> +struct scsi_data_buffer {
> + unsigned length;
> + int resid;
> + unsigned short sg_count;
> + unsigned short alloc_sg_count;
> + struct scatterlist* sglist;
> +};

This has exactly the problems I thought it would have.  Due to alignment
rules, it grows the scsi_cmnd from 368 to 376 bytes on x86-64.
It remains at 272 bytes on i386 though, which is some consolation.
By porting the patch I had for shrinking the scsi_cmnd, I can get it
down to 352 bytes, but not as far as the 344 bytes I had it at before.

The problem is the padding at the *end* of scsi_data_buffer (er, after I
rearrange it in the patch below).  There's nothing we can realistically
put in it, given that we don't want to expand scsi_data_buffer's size on
32-bit machines.  I have a fix ... but I don't think you'll like it.
I certainly don't.  But it does get us back down to 344 bytes.

Updated patch below.  I'm fully expecting the 'result' shenanigan to get
it NACKed, but I'd like to see if it inspires anyone else to a more
creative way of saving this space.

---

Thanks to acme's pahole utility, I found some places where we can save
a lot of bytes in scsi_cmnd, just by rearranging struct elements and
reducing the size of some elements.  We go from 272 to 260 bytes on x86
and from 368 to 344 bytes on x86-64.

 - eh_eflags had a 4-byte hole after it on 64-bit.  In fact, this has
   value 0 or 1, so reduce it to an unsigned char, and put it with the
   other chars in scsi_cmnd.  Saves 8 bytes on 64-bit.
 - sc_data_direction has a value from 0-3, so make it an unsigned char
   rather than an enum.  Saves at least 4 bytes.
 - Putting 'tag' with the other char elements saves another 4 bytes
 - Moving 'result' into the union with the deprecated members saves 8
   bytes on 64-bit.

diff --git a/include/scsi/scsi_cmnd.h b/include/scsi/scsi_cmnd.h
index 047ffe6..2b10779 100644
--- a/include/scsi/scsi_cmnd.h
+++ b/include/scsi/scsi_cmnd.h
@@ -13,22 +13,21 @@ struct Scsi_Host;
 struct scsi_device;
 
 struct scsi_data_buffer {
+   struct scatterlist* sglist;
unsigned length;
int resid;
unsigned short sg_count;
unsigned short alloc_sg_count;
-   struct scatterlist* sglist;
 };
 
 /* embedded in scsi_cmnd */
 struct scsi_pointer {
char *ptr;  /* data pointer */
-   int this_residual;  /* left in this buffer */
struct scatterlist *buffer; /* which buffer */
+   dma_addr_t dma_handle;
+   int this_residual;  /* left in this buffer */
int buffers_residual;   /* how many buffers left */
 
-dma_addr_t dma_handle;
-
volatile int Status;
volatile int Message;
volatile int have_data_in;
@@ -40,7 +39,6 @@ struct scsi_cmnd {
struct scsi_device *device;
struct list_head list;  /* scsi_cmnd participates in queue lists */
struct list_head eh_entry; /* entry for the host eh_cmd_q */
-   int eh_eflags;  /* Used by error handlr */
 
/*
 * A SCSI Command is assigned a nonzero serial_number before passed
@@ -64,7 +62,9 @@ struct scsi_cmnd {
int timeout_per_command;
 
unsigned char cmd_len;
-   enum dma_data_direction sc_data_direction;
+   unsigned char eh_eflags;/* Used by error handler */
+   unsigned char sc_data_direction;/* enum dma_data_direction */
+   unsigned char tag;  /* SCSI-II queued command tag */
 
/* These elements define the operation we are about to perform */
 #define MAX_COMMAND_SIZE   16
@@ -109,10 +109,6 @@ struct scsi_cmnd {
 * obtained by scsi_malloc is guaranteed
 * to be at an address < 16Mb). */
 
-   int result; /* Status code from lower level driver */
-
-   unsigned char tag;  /* SCSI-II queued command tag */
-
union {
struct scsi_data_buffer sdb;
/*
@@ -121,11 +117,12 @@ struct scsi_cmnd {
 *of struct scsi_data_buffer members.
 */
struct {
+   void __deprecated *request_buffer;
unsigned __deprecated request_bufflen;
int __deprecated resid;
unsigned short __deprecated use_sg;
unsigned short __deprecated place_holder_sg_alloc;
-   void __deprecated *request_buffer;
+   int result; /* Status code from lower level driver */
};
};
 };

-- 
Intel are signing my paycheques ... these opinions are still mine
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours.  We can't possibly take such
a retrograde step."
-

Re: [patchset 0/33] scsi_data_buffer for after the last driver is converted

2007-10-17 Thread FUJITA Tomonori

On Wed, 17 Oct 2007 19:14:46 +0200
Boaz Harrosh <[EMAIL PROTECTED]> wrote:

> This is a resend of all the remaining drivers rebased
> to latest code. And once the entire tree is converted,
> move to scsi_data_buffer in scsi_cmnd.
> 
> The work is on top of Jens's for-linus branch which
> is effectively sglist-arch (+ last minute fixes)
> This is because sglist-arch was already rebased to
> latest scsi-misc through linus tree, merged with 
> the extra work done there on scsi-ml (and the drivers)
> (Thanks Jens)
> 
> summery of patches:
> 
> left over from scsi_eh last change
>   [PATCH 01/32] arm: fas216 Use scsi_eh API for REQUEST_SENSE invocation
> 
> lots and lots of drivers left:
>   [PATCH 02/32] isd200.c: use one-element sg list in issuing commands
>   [PATCH 03/32] usb: transport - convert to accessors and !use_sg code path 
> removal
>   [PATCH 04/32] usb: protocol.c - convert to accessors and !use_sg code path 
> removal
>   [PATCH 05/32] usb: shuttle_usbat.c - convert to accessors and !use_sg code 
> path removal
>   [PATCH 06/32] usb: freecom.c & sddr09.c - convert to accessors and !use_sg 
> cleanup
>   [PATCH 07/32] NCR5380 familly convert to accessors & !use_sg cleanup
>   [PATCH 08/32] arm: scsi convert to accessors and !use_sg cleanup
>   [PATCH 09/32] nsp_cs.c convert to data accessors and !use_sg cleanup
>   [PATCH 10/32] eata_pio.c: convert to accessors and !use_sg cleanup
>   [PATCH 11/32] a2091.c: convert to accessors and !use_sg cleanup
>   [PATCH 12/32] a3000.c: convert to accessors and !use_sg cleanup
>   [PATCH 13/32] aha1542.c: convert to accessors and !use_sg cleanup
>   [PATCH 14/32] atp870u.c: convert to accessors and !use_sg cleanup
>   [PATCH 15/32] fd_mcs.c: convert to accessors and !use_sg cleanup
>   [PATCH 16/32] imm.c: convert to accessors and !use_sg cleanup
>   [PATCH 17/32] ppa.c: convert to accessors and !use_sg cleanup
>   [PATCH 18/32] wd32c93.c: convert to accessors and !use_sg cleanup
>   [PATCH 19/32] qlogicpti.c: convert to accessors and !use_sg cleanup
>   [PATCH 20/32] in2000.c: convert to accessors and !use_sg cleanup
>   [PATCH 21/32] scsi_debug: convert to use the data buffer accessors
> 
> Tomo's patch cleaned up:
>[PATCH 22/32] qla1280: convert to use the data buffer accessors
>[PATCH 23/32] qla1280: Indentation fix
> 
>  I separated the indentation from the real change make
>  the patch humanly readable.

It doesn't hurt but I'm not sure how useful the separation is.

-
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

Re: [PATCH 27/32] scsi_data_buffer

2007-10-17 Thread FUJITA Tomonori

On Wed, 17 Oct 2007 20:21:15 +0200
Boaz Harrosh <[EMAIL PROTECTED]> wrote:

> 
>   In preparation for bidi we abstract all IO members of scsi_cmnd,
>   that will need to duplicate, into a substructure.
> 
>   - Group all IO members of scsi_cmnd into a scsi_data_buffer
> structure.
>   - Adjust accessors to new members.
>   - scsi_{alloc,free}_sgtable receive a scsi_data_buffer instead of
> scsi_cmnd. And work on it.
>   - Adjust scsi_init_io() and  scsi_release_buffers() for above
> change.
>   - Fix other parts of scsi_lib/scsi.c to members migration. Use
> accessors where appropriate.
> 
>   - Old I/O members are kept for backward compatibility, since
> not all of scsi-ml/ul is converted yet. Once done they will
> be removed in a closing patch. (Other wise the patchset will
> not be bisectable)

(snip)

> @@ -114,6 +112,22 @@ struct scsi_cmnd {
>   int result; /* Status code from lower level driver */
>  
>   unsigned char tag;  /* SCSI-II queued command tag */
> +
> + union {
> + struct scsi_data_buffer sdb;
> + /*
> +  * FIXME: Here for compatibility with unconverted drivers.
> +  *Must be kept in sync with exact type and order
> +  *of struct scsi_data_buffer members.
> +  */
> + struct {
> + unsigned __deprecated request_bufflen;
> + int __deprecated resid;
> + unsigned short __deprecated use_sg;
> + unsigned short __deprecated place_holder_sg_alloc;
> + void __deprecated *request_buffer;
> + };
> + };
>  };

If we add something like this, why couldn't we merge the
scsi_data_buffer patchset when I submitted last month?

I thought that I wait you to fix all the old LLDs, then will send a
clean scsi_data_buffer patchset.
-
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

Re: What still uses the block layer?

2007-10-17 Thread Bill Davidsen

Jeff Garzik wrote:

But again, please remember that these USB devices are really SCSI
devices. Same for SATA devices. There is a reason they are using the
SCSI layer, and it isn't just because the developers felt like it :)

/somewhat/ true I'm afraid: libata uses the SCSI layer for ATAPI
devices because they are essentially bridges to SCSI devices. It uses
the SCSI layer for ATA devices because the SCSI layer provided a huge
amount of infrastructure that would need to have been otherwise
duplicated, /then/ massaged into coordinating between layer> and when dealing with ATAPI.

There is also a detail that was of /huge/ value when introducing a new
device class: distro installers automatically work, if you use SCSI. If
you use a new block device type, that behaves differently from other
types and is on a different major, you have to poke the distros into
action or do it yourself.

IOW, it was the high Just Works(tm) value of the SCSI layer when it came
to ATA (not ATAPI) devices.

For the future, ATA will eventually be more independent (though the SCSI
simulator will be available as an option, for compat), but the value is
big enough to put that task on the back-burner.

I remember being told that I didn't understand the problem when I
suggested using ide-scsi for everything and just hiding the transport. I
get great pleasure from having been (mostly) right on that one. I still
have old systems running ZIP drives as scsi...

--
Bill Davidsen <[EMAIL PROTECTED]>
"We have more to fear from the bungling of the incompetent than from
the machinations of the wicked." - from Slashdot
-
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

Re: SCSI target drivers?

2007-10-17 Thread FUJITA Tomonori

On Wed, 17 Oct 2007 10:27:54 -0400
Jeff Garzik <[EMAIL PROTECTED]> wrote:

> Where can one find patches/code that illustrates target mode support?

I think that the srp class and ibmvstgt (drivers/ibmvscsi/ibmvstgt)
are useful.

The fc class and qla2xxx are useful too but more complicated than
ibmvstgt:

- fc_transport: add target driver support

http://marc.info/?l=linux-scsi&m=118857984316539&w=2

- qla2xxx: add target mode support

http://marc.info/?l=linux-scsi&m=118857984513557&w=2

The former patch is in mainline and the latter has been reviewed by
Qlogic.

> Marvell 6440 ("mvsas") supports target mode for both SAS and SATA, and I 
> would like to prepare the driver as best I can for target mode.

Great! It would be really nice to have SAS target support.
-
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

Re: [PATCH] SPARC64: fix iommu sg chaining

2007-10-17 Thread FUJITA Tomonori

On Wed, 17 Oct 2007 20:37:58 +0900
FUJITA Tomonori <[EMAIL PROTECTED]> wrote:

> On Wed, 17 Oct 2007 13:01:42 +0200
> Jens Axboe <[EMAIL PROTECTED]> wrote:
> 
> > On Wed, Oct 17 2007, Jens Axboe wrote:
> > > On Wed, Oct 17 2007, David Miller wrote:
> > > > From: Jens Axboe <[EMAIL PROTECTED]>
> > > > Date: Wed, 17 Oct 2007 11:16:29 +0200
> > > > 
> > > > > On Wed, Oct 17 2007, David Miller wrote:
> > > > > > From: Jens Axboe <[EMAIL PROTECTED]>
> > > > > > Date: Wed, 17 Oct 2007 10:45:28 +0200
> > > > > > 
> > > > > > > Righto, it's invalid to call sg_next() on the last entry!
> > > > > > 
> > > > > > Unfortunately, that's what the sparc64 code wanted to do, this
> > > > > > transformation in the sparc64 sg chaining patch is not equilavent:
> > > > > > 
> > > > > > -   struct scatterlist *sg_end = sg + nelems;
> > > > > > +   struct scatterlist *sg_end = sg_last(sg, nelems);
> > > > > >  ...
> > > > > > -   while (sg < sg_end &&
> > > > > > +   while (sg != sg_end &&
> > > > > 
> > > > > Auch indeed. That'd probably be better as a
> > > > > 
> > > > > do {
> > > > > ...
> > > > > } while (sg != sg_end);
> > > > 
> > > > Ok, next bug, introduced by this change:
> > > > 
> > > > commit f565913ef8a8d0cfa46a1faaf8340cc357a46f3a
> > > > Author: Jens Axboe <[EMAIL PROTECTED]>
> > > > Date:   Fri Sep 21 10:44:19 2007 +0200
> > > > 
> > > > block: convert to using sg helpers
> > > > 
> > > > Convert the main rq mapper (blk_rq_map_sg()) to the sg helper setup.
> > > > 
> > > > Signed-off-by: Jens Axboe <[EMAIL PROTECTED]>
> > > > 
> > > > Specifically this part:
> > > > 
> > > >  new_segment:
> > > > -   memset(&sg[nsegs],0,sizeof(struct scatterlist));
> > > > -   sg[nsegs].page = bvec->bv_page;
> > > > -   sg[nsegs].length = nbytes;
> > > > -   sg[nsegs].offset = bvec->bv_offset;
> > > > +   sg = next_sg;
> > > > +   next_sg = sg_next(sg);
> > > >  
> > > > +   sg->page = bvec->bv_page;
> > > > +   sg->length = nbytes;
> > > > +   sg->offset = bvec->bv_offset;
> > > > 
> > > > You can't remove that memset(), it's there for a reason.  The IOMMU
> > > > layers depended upon the code zero'ing out the whole scatterlist
> > > > struct, there might be more to it than page, length and offset :-)
> > > 
> > > I realize that, and I was pretty worried about this specific change. But
> > > there's only been one piece of fallout because if it until now - well
> > > two, with the sparc64 stuff.
> > > 
> > > The problem is that you cannot zero the entire sg entry, because then
> > > you'd potentially overwrite the chain pointer.
> > > 
> > > I'd propose just adding a
> > > 
> > > sg_dma_address(sg) = 0;
> > > sg_dma_len(sg) = 0;
> > > 
> > > there for now, or provide an arch_clear_sg_entry() helper if we need
> > > more killed.
> > 
> > Actually, just clearing AFTER sg_next() would be fine, since we know
> > that is not a link entry. Duh...
> > 
> > diff --git a/block/ll_rw_blk.c b/block/ll_rw_blk.c
> > index 9eabac9..1014d34 100644
> > --- a/block/ll_rw_blk.c
> > +++ b/block/ll_rw_blk.c
> > @@ -1352,6 +1352,7 @@ new_segment:
> > sg = next_sg;
> > next_sg = sg_next(sg);
> >  
> > +   memset(sg, 0, sizeof(*sg));
> > sg->page = bvec->bv_page;
> > sg->length = nbytes;
> > sg->offset = bvec->bv_offset;
> > 
> > -- 
> 
> So now how about removing zero'ing out sglist in scsi-ml?

Really sorry, I must have been stoned.
-
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

Re: [PATCH 22/32] qla1280: Indentation fix

2007-10-17 Thread Matthew Wilcox

On Wed, Oct 17, 2007 at 08:10:09PM +0200, Boaz Harrosh wrote:
> 
>   - Indentation fix of last patch. Don't be fooled by diff,
> all I did was back-indent from open-bracket to close-bracket,
> and remove the brackets.

Applying 'qla1280: Indentation fix'

Adds trailing whitespace.
.dotest/patch:90: sn_pci_set_vchan(ha->pdev, 
warning: 1 line adds trailing whitespaces.

-- 
Intel are signing my paycheques ... these opinions are still mine
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours.  We can't possibly take such
a retrograde step."
-
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

Re: What still uses the block layer?

2007-10-17 Thread david


On Wed, 17 Oct 2007, Gabor Gombas wrote:


On Tue, Oct 16, 2007 at 01:55:07PM -0700, [EMAIL PROTECTED] wrote:


why is this any different from the external enclosures? they have always
appeared as the type of device that connects them to the motherboard, (and
even with SCSI, there are some controllers that don't generate sdX devices)


In the past enclosures supported only one kind of connector so this
assumption was fine. But nowadays an external disk may have several
connectors (like USB, Firewire and eSata). Why should the disk's name
depend on what type of cable did I manage to grab first? It is the
_same_ disk regardless of the cable type.


the right type for the type of cable you choose to use. yes it's the same 
disk, but by choosing to hook it up in a different way you get different 
results from it (different performance, different predictability)


again, if you want to have a udev rule that then maps these different name 
onto the same name, more power to you, but why do you insist on makeing 
_everyone_ work that way (or go to significant extra effort to find the 
info in the changing directory structure of sysfs to track down the info 
that you throw away)



There is one thing however that could be improved: renaming a disk in an
udev rule should propagate the new name back to the kernel, just like
renaming an ethernet interface does. That way mapping error messages to
physical disk locations could be made much easier.


definantly.

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

Re: [Bug 3680] sym53c8xx_2 SMP deadlock on driver load

2007-10-17 Thread Matthew Wilcox

On Wed, Oct 17, 2007 at 08:46:48PM +0200, Willy Tarreau wrote:
> Sincere thanks for your help and review. I'll apply this patch in
> 2.4.36-pre2 and in next 2.4.35.X. An ugly fix is clearly better than
> a massive change at this stage.

You're most welcome.  I have no desire to act as maintainer for sym2 in
2.4 at this point, but if you've got any other related bugs, feel free
to point me at them.

-- 
Intel are signing my paycheques ... these opinions are still mine
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours.  We can't possibly take such
a retrograde step."
-
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

Re: [Bug 3680] sym53c8xx_2 SMP deadlock on driver load

2007-10-17 Thread Willy Tarreau

On Wed, Oct 17, 2007 at 10:27:47AM -0600, Matthew Wilcox wrote:
> On Wed, Oct 17, 2007 at 10:53:06AM -0400, Tony Battersby wrote:
> > After looking at it carefully, this is true of pci_map_mem, but not
> > pci_unmap_mem.  pci_unmap_mem can be called from both ->detect and
> > ->release.  io_request_lock is held in ->detect but not in ->release.
> > So, your patch locks up the system on module unload.
> 
> My mistake.  I should have checked the iorl was held over ->release too.
> 
> This is a pretty ugly patch, but I think the three alternatives are
> worse:
> 
>  - Drop the iorl at the beginning of ->detect and reacquire it at exit.
>We don't know what else the iorl might be protecting.  Probably
>nothing, but I don't want to audit it.
>  - Convert sym2 back to old error handling so the midlayer doesn't
>acquire the iorl for us in ->detect.
>  - Acquire the iorl in ->release.  We might deadlock on something.
> 
> So, sure, apply this patch.

Matthew,

Sincere thanks for your help and review. I'll apply this patch in
2.4.36-pre2 and in next 2.4.35.X. An ugly fix is clearly better than
a massive change at this stage.

Natalie, if you want, you can close the bug right now, I've queued the
patch for both branches.

Best regards,
Willy

-
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

93 matches

Mail list logo