Re: [Linux-usb-users] Possible bug in usb storage (2.6.11 kernel)

Re: [Linux-usb-users] Possible bug in usb storage (2.6.11 kernel)

[Jim Ramsay]

On 9/8/05, Jim Ramsay <jim.ramsay@gmail.com> wrote:
> On 9/8/05, Matthew Dharm <mdharm-kernel@one-eyed-alien.net> wrote:
> > On Thu, Sep 08, 2005 at 11:14:36AM -0600, Jim Ramsay wrote:
> > > I think I have found a possible bug:
> > > [...]
> > > I suppose the scsi code could be changed to guarantee that
> > > srb->request_buffer is page-aligned or cache-aligned, but that seems
> > > like the wrong solution for this bug.
> >
> > Fixing the SCSI layer is -exactly- the correct solution.  The SCSI layer is
> > supposed to guarantee us that those buffers are suitable for DMA'ing, and
> > apparently it's violating that promise.
> 
> Thanks, I'll check on what buffer I'm actually getting, where it's
> allocated, and post back what I find, or how I fixed it.

More information:

The error only occurrs during device sensing when the
srb->request_buffer is assigned as follows, by usb/storage/transport.c
in the routine usb_stor_invoke_transport:

old_request_buffer = srb->request_buffer;
srb->request_buffer = srb->sense_buffer;

Now, this is a problem because srb->sense_buffer is defined as follows
in the struct scsi_cmnd:

#define SCSI_SENSE_BUFFERSIZE   96
        unsigned char sense_buffer[SCSI_SENSE_BUFFERSIZE];

Since it is not allocated at runtime there is NO WAY the SCSI layer
can possibly guarantee it is page- or cache-aligned and ready for DMA.

Any suggestions on best fix for this?  Is it still a SCSI-layer issue?
 Or should USB step up in this case and ensure this buffer is dma-safe
itself?

-- 
Jim Ramsay
"Me fail English?  That's unpossible!"

Re: Possible bug in usb storage (2.6.11 kernel)

[Alan Stern]

On Thu, 8 Sep 2005, Jim Ramsay wrote:

> More information:
> 
> The error only occurrs during device sensing when the
> srb->request_buffer is assigned as follows, by usb/storage/transport.c
> in the routine usb_stor_invoke_transport:
> 
> old_request_buffer = srb->request_buffer;
> srb->request_buffer = srb->sense_buffer;
> 
> Now, this is a problem because srb->sense_buffer is defined as follows
> in the struct scsi_cmnd:
> 
> #define SCSI_SENSE_BUFFERSIZE   96
>         unsigned char sense_buffer[SCSI_SENSE_BUFFERSIZE];
> 
> Since it is not allocated at runtime there is NO WAY the SCSI layer
> can possibly guarantee it is page- or cache-aligned and ready for DMA.
> 
> Any suggestions on best fix for this?  Is it still a SCSI-layer issue?
>  Or should USB step up in this case and ensure this buffer is dma-safe
> itself?

Aha!

I've long thought that usb-storage should allocate its own transfer buffer 
for sense data.  In the past people have said, "No, don't bother, it's not 
really needed."  Here's a good reason for doing it.

Expect a patch before long.

Alan Stern



-------------------------------------------------------
SF.Net email is Sponsored by the Better Software Conference & EXPO
September 19-22, 2005 * San Francisco, CA * Development Lifecycle Practices
Agile & Plan-Driven Development * Managing Projects & Teams * Testing & QA
Security * Process Improvement & Measurement * http://www.sqe.com/bsce5sf
_______________________________________________
Linux-usb-users@lists.sourceforge.net
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-users

Re: Possible bug in usb storage (2.6.11 kernel)

[Atsushi Nemoto]

>>>>> On Thu, 8 Sep 2005 16:40:16 -0400 (EDT), Alan Stern <stern@rowland.harvard.edu> said:

stern> I've long thought that usb-storage should allocate its own
stern> transfer buffer for sense data.  In the past people have said,
stern> "No, don't bother, it's not really needed."  Here's a good
stern> reason for doing it.

stern> Expect a patch before long.

Did you already create the patch?  If not, how about this (against 2.6.13) ?


Signed-off-by: Atsushi Nemoto <anemo@mba.ocn.ne.jp>

diff -u linux-2.6.13/drivers/usb/storage/transport.c linux/drivers/usb/storage/transport.c
--- linux-2.6.13/drivers/usb/storage/transport.c	2005-08-29 08:41:01.000000000 +0900
+++ linux/drivers/usb/storage/transport.c	2005-09-27 18:03:32.000000000 +0900
@@ -638,7 +638,8 @@
 
 		/* use the new buffer we have */
 		old_request_buffer = srb->request_buffer;
-		srb->request_buffer = srb->sense_buffer;
+		srb->request_buffer =
+			kmalloc(max(dma_get_cache_alignment(), 18), GFP_NOIO);
 
 		/* set the buffer length for transfer */
 		old_request_bufflen = srb->request_bufflen;
@@ -655,7 +656,13 @@
 		/* issue the auto-sense command */
 		old_resid = srb->resid;
 		srb->resid = 0;
-		temp_result = us->transport(us->srb, us);
+		if (srb->request_buffer) {
+			temp_result = us->transport(us->srb, us);
+			memcpy(srb->sense_buffer, srb->request_buffer, 18);
+			kfree(srb->request_buffer);
+		} else {
+			temp_result = USB_STOR_TRANSPORT_FAILED;
+		}
 
 		/* let's clean up right away */
 		srb->resid = old_resid;


---
Atsushi Nemoto


-------------------------------------------------------
SF.Net email is sponsored by:
Tame your development challenges with Apache's Geronimo App Server. Download
it for free - -and be entered to win a 42" plasma tv or your very own
Sony(tm)PSP.  Click here to play: http://sourceforge.net/geronimo.php
_______________________________________________
Linux-usb-users@lists.sourceforge.net
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-users

Re: [Linux-usb-users] Possible bug in usb storage (2.6.11 kernel)

[Alan Stern]

On Tue, 27 Sep 2005, Atsushi Nemoto wrote:

> >>>>> On Thu, 8 Sep 2005 16:40:16 -0400 (EDT), Alan Stern <stern@rowland.harvard.edu> said:
> 
> stern> I've long thought that usb-storage should allocate its own
> stern> transfer buffer for sense data.  In the past people have said,
> stern> "No, don't bother, it's not really needed."  Here's a good
> stern> reason for doing it.
> 
> stern> Expect a patch before long.
> 
> Did you already create the patch?  If not, how about this (against 2.6.13) ?

Yes I did.  You can see it at

https://lists.one-eyed-alien.net/pipermail/usb-storage/2005-September/001953.html

Alan Stern

Re: Possible bug in usb storage (2.6.11 kernel)

[Atsushi Nemoto]

>>>>> On Tue, 27 Sep 2005 10:21:17 -0400 (EDT), Alan Stern <stern@rowland.harvard.edu> said:

stern> Yes I did.  You can see it at
stern> https://lists.one-eyed-alien.net/pipermail/usb-storage/2005-September/001953.html

Thank you.  But 'kmalloc(US_SENSE_SIZE, GFP_KERNEL)' is not enough (at
least) for MIPS since some MIPS chips have 32 byte cacheline and
ARCH_KMALLOC_MINALIGN is 8 on linux-mips.

Using 'max(dma_get_cache_alignment(), US_SENSE_SIZE)' would be OK.

---
Atsushi Nemoto


-------------------------------------------------------
SF.Net email is sponsored by:
Tame your development challenges with Apache's Geronimo App Server. Download
it for free - -and be entered to win a 42" plasma tv or your very own
Sony(tm)PSP.  Click here to play: http://sourceforge.net/geronimo.php
_______________________________________________
Linux-usb-users@lists.sourceforge.net
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-users

Re: [Linux-usb-users] Possible bug in usb storage (2.6.11 kernel)

[Alan Stern]

On Tue, 27 Sep 2005, Atsushi Nemoto wrote:

> >>>>> On Tue, 27 Sep 2005 10:21:17 -0400 (EDT), Alan Stern <stern@rowland.harvard.edu> said:
> 
> stern> Yes I did.  You can see it at
> stern> https://lists.one-eyed-alien.net/pipermail/usb-storage/2005-September/001953.html
> 
> Thank you.  But 'kmalloc(US_SENSE_SIZE, GFP_KERNEL)' is not enough (at
> least) for MIPS since some MIPS chips have 32 byte cacheline and
> ARCH_KMALLOC_MINALIGN is 8 on linux-mips.
> 
> Using 'max(dma_get_cache_alignment(), US_SENSE_SIZE)' would be OK.

If that is so, it's a bug in linux-mips.  ARCH_KMALLOC_MINALIGN is 
supposed to be at least as large as a cacheline.  See this comment in 
mm/slab.c:

/*
 * Enforce a minimum alignment for the kmalloc caches.
 * Usually, the kmalloc caches are cache_line_size() aligned, except when
 * DEBUG and FORCED_DEBUG are enabled, then they are BYTES_PER_WORD aligned.
 * Some archs want to perform DMA into kmalloc caches and need a guaranteed
 * alignment larger than BYTES_PER_WORD. ARCH_KMALLOC_MINALIGN allows that.
 * Note that this flag disables some debug features.
 */

and also this comment (referring to the kmalloc caches):

		/*
		 * For performance, all the general caches are L1 aligned.
		 * This should be particularly beneficial on SMP boxes, as it
		 * eliminates "false sharing".
		 * Note for systems short on memory removing the alignment will
		 * allow tighter packing of the smaller caches.
		 */

Alan Stern

Re: [Linux-usb-users] Possible bug in usb storage (2.6.11 kernel)

[Atsushi Nemoto]

>>>>> On Tue, 27 Sep 2005 11:38:35 -0400 (EDT), Alan Stern <stern@rowland.harvard.edu> said:

stern> If that is so, it's a bug in linux-mips.  ARCH_KMALLOC_MINALIGN
stern> is supposed to be at least as large as a cacheline.  See this
stern> comment in mm/slab.c:

Thank you for pointing out this.

Some time ago I also supposed so, but I was told to use
dma_get_cache_alignment() instead.  The comment was not exist at that
time...

OK, I will talk to MIPS maintainar again.
---
Atsushi Nemoto

Re: [Linux-usb-users] Possible bug in usb storage (2.6.11 kernel)

[Ralf Baechle]

On Thu, Sep 29, 2005 at 01:27:05AM +0900, Atsushi Nemoto wrote:

> >>>>> On Tue, 27 Sep 2005 11:38:35 -0400 (EDT), Alan Stern <stern@rowland.harvard.edu> said:
> 
> stern> If that is so, it's a bug in linux-mips.  ARCH_KMALLOC_MINALIGN
> stern> is supposed to be at least as large as a cacheline.  See this
> stern> comment in mm/slab.c:
> 
> Thank you for pointing out this.
> 
> Some time ago I also supposed so, but I was told to use
> dma_get_cache_alignment() instead.  The comment was not exist at that
> time...

ARCH_KMALLOC_MINALIGN is set to 8 on MIPS because we used to have 64-bit
members in struct semaphore but on 32-bit kernels kmalloc would return 4-byte
allocated memory only.  Whoops, an ooops ;)  Obviously that's been a thinko
as it was done without consideration for DMA.

Coherent I/O isn't affected, also there are a few R3000-class processors where
8 bytes happens to be just the right value.  For anything else this is a bug
which we probably get away most of the time and the value should would be
the largest size of any cacheline in the cache hierarchy, so upto 128 for some
systems.

  Ralf

Re: [Linux-usb-users] Possible bug in usb storage (2.6.11 kernel)

[Matthew Dharm]

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

On Thu, Sep 08, 2005 at 02:28:09PM -0600, Jim Ramsay wrote:
> On 9/8/05, Jim Ramsay <jim.ramsay@gmail.com> wrote:
> > On 9/8/05, Matthew Dharm <mdharm-kernel@one-eyed-alien.net> wrote:
> > > On Thu, Sep 08, 2005 at 11:14:36AM -0600, Jim Ramsay wrote:
> > > > I think I have found a possible bug:
> > > > [...]
> > > > I suppose the scsi code could be changed to guarantee that
> > > > srb->request_buffer is page-aligned or cache-aligned, but that seems
> > > > like the wrong solution for this bug.
> > >
> > > Fixing the SCSI layer is -exactly- the correct solution.  The SCSI layer is
> > > supposed to guarantee us that those buffers are suitable for DMA'ing, and
> > > apparently it's violating that promise.
> > 
> > Thanks, I'll check on what buffer I'm actually getting, where it's
> > allocated, and post back what I find, or how I fixed it.
> 
> More information:
> 
> The error only occurrs during device sensing when the
> srb->request_buffer is assigned as follows, by usb/storage/transport.c
> in the routine usb_stor_invoke_transport:
> 
> old_request_buffer = srb->request_buffer;
> srb->request_buffer = srb->sense_buffer;
> 
> Now, this is a problem because srb->sense_buffer is defined as follows
> in the struct scsi_cmnd:
> 
> #define SCSI_SENSE_BUFFERSIZE   96
>         unsigned char sense_buffer[SCSI_SENSE_BUFFERSIZE];
> 
> Since it is not allocated at runtime there is NO WAY the SCSI layer
> can possibly guarantee it is page- or cache-aligned and ready for DMA.
> 
> Any suggestions on best fix for this?  Is it still a SCSI-layer issue?
>  Or should USB step up in this case and ensure this buffer is dma-safe
> itself?

Ah, that buffer doesn't come from SCSI (tho I've long thought they should
provide us with a sense data buffer).  So this is a real usb-storage bug.

Matt

-- 
Matthew Dharm                              Home: mdharm-usb@one-eyed-alien.net 
Maintainer, Linux USB Mass Storage Driver

What, are you one of those Microsoft-bashing Linux freaks?
					-- Customer to Greg
User Friendly, 2/10/1999

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]