Re: usb-storage and Sony Handycam

Re: usb-storage and Sony Handycam

[Alan Stern]

On Fri, 7 Nov 2003, Dmitri Katchalov wrote:

> Greetings,
> 
> I finally managet to get Sony DCR-TRV17E memory stick
> working with usb-storage.
> The solution turned out to be very simple:
> 
> echo "Sony:Sony DSC:0" > /proc/scsi/device_info
> 
> But I'm puzzled as to why didn't it work in the first 
> place despite several reports to the contrary. My guess
> is it used to work in earlier kernels and then some
> seemingly unrelated change broke it.
> 
> I investigated it a little and that's what I found:
> This device uses CB transport (SENSE status not reported).
> This device does not support MODE_SENSE at all.
> MODE_SENSE(6) fails with STALL condition. Subsequent 
> REQUEST-SENSE reports ILLEGAL REQUEST. SCSI layer is 
> able to handle this situation, the only drawback is 
> that write-protect status is not reported.
> 
> Unfortunately usb-storage sets use_10_for_ms flag 
> by default which causes SCSI to try MODE_SENSE(10)
> first. 
> The device responds to MODE_SENSE(10) with "babble" 
> (-EOVERFLOW). scsiglue sees transport error and 
> tries to do a soft reset (which also fails).
> Note that scsiglue does not automatically send 
> REQUEST-SENSE in this case.
> 
> scsiglue then gives up after several attempts and 
> reports the error to the upper layer. Upper layer
> (SCSI) correctly realises that MODE_SENSE(10) does
> not work and attempts to fallback to MODE_SENSE(6) 
> but before that it issues TEST_UNIT_READY. 
> 
> This time the device responds with ILLEGAL REQUEST.
> Now SCSI gets totally confused and decides to give up.
> 
> As I understand it this ILLEGAL REQUEST actually refers
> to the previous MODE_SENSE(10) command because this 
> error condition has never been cleared properly.

You're probably right.  But that's not how TEST-UNIT-READY is _supposed_ 
to behave.  It shouldn't be reporting the status from a previous command; 
it should indicate the device's current status.

> If I change interpret_usb_result() in transport.c 
> to ignore "babble" then the next REQUEST-SENSE 
> reports ILLEGAL REQUEST, SCSI handles that and 
> everything sort of works.
> 
> Regards,
> Dmitri

Patrick, what do you think about implementing that change to have the 
response checker retry when it gets ILLEGAL REQUEST from a 
TEST-UNIT-READY?

Alan Stern



-------------------------------------------------------
This SF.net email is sponsored by: SF.net Giveback Program.
Does SourceForge.net help you be more productive?  Does it
help you create better code?   SHARE THE LOVE, and help us help
YOU!  Click Here: http://sourceforge.net/donate/
_______________________________________________
linux-usb-devel@lists.sourceforge.net
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel

Re: usb-storage and Sony Handycam

[Patrick Mansfield]

On Fri, Nov 07, 2003 at 11:21:48AM -0500, Alan Stern wrote:
> On Fri, 7 Nov 2003, Dmitri Katchalov wrote:

> > I investigated it a little and that's what I found:
> > This device uses CB transport (SENSE status not reported).
> > This device does not support MODE_SENSE at all.
> > MODE_SENSE(6) fails with STALL condition. Subsequent 
> > REQUEST-SENSE reports ILLEGAL REQUEST. SCSI layer is 
> > able to handle this situation, the only drawback is 
> > that write-protect status is not reported.
> > 
> > Unfortunately usb-storage sets use_10_for_ms flag 
> > by default which causes SCSI to try MODE_SENSE(10)
> > first. 
> > The device responds to MODE_SENSE(10) with "babble" 
> > (-EOVERFLOW). scsiglue sees transport error and 
> > tries to do a soft reset (which also fails).
> > Note that scsiglue does not automatically send 
> > REQUEST-SENSE in this case.
> > 
> > scsiglue then gives up after several attempts and 
> > reports the error to the upper layer. Upper layer
> > (SCSI) correctly realises that MODE_SENSE(10) does
> > not work and attempts to fallback to MODE_SENSE(6) 
> > but before that it issues TEST_UNIT_READY. 


> > This time the device responds with ILLEGAL REQUEST.
> > Now SCSI gets totally confused and decides to give up.

The above sequence is not quite correct (assuming the same behaviour as
logs for another failing Sony/Sony DSC), the MODE SENSE goes out, but we
do not notice in scsi core that it failed (host byte DID_ERROR). 

The MODE SENSE is treated as if it succeeded, and yes the TEST UNIT READY
gets an ILLEGAL REQUEST.

I don't recall what happens to further MODE SENSE commands.

> > As I understand it this ILLEGAL REQUEST actually refers
> > to the previous MODE_SENSE(10) command because this 
> > error condition has never been cleared properly.
> 
> You're probably right.  But that's not how TEST-UNIT-READY is _supposed_ 
> to behave.  It shouldn't be reporting the status from a previous command; 
> it should indicate the device's current status.
> 
> > If I change interpret_usb_result() in transport.c 
> > to ignore "babble" then the next REQUEST-SENSE 
> > reports ILLEGAL REQUEST, SCSI handles that and 
> > everything sort of works.

I don't see how that worked, I would expect nearly the same command
sequence, since scsi core is not checking the error. 

USB storage debug logs are needed to verify what happened (when ignoring
babble/overflow).

> > Regards,
> > Dmitri
> 
> Patrick, what do you think about implementing that change to have the 
> response checker retry when it gets ILLEGAL REQUEST from a 
> TEST-UNIT-READY?

No, since we should fix scsi_status_is_good, and then we see a different
failure mode for the Sony/Sony DSC.

Dmitri -

Reference to the patch and discussion about scsi_status_is_good change,
you will likely get the same bad result if you apply the patch:

http://marc.theaimsgroup.com/?t=106744619500004&r=1&w=2

We should figure out what commands work for the device using SG_IO, try
using Pat L's plscsi or modify one of the sg_util programs to send the
exact failing command, reference:

http://marc.theaimsgroup.com/?l=linux-scsi&m=106755844319144&w=2

A USB trace under Windows would be helpful.

-- Patrick Mansfield


-------------------------------------------------------
This SF.Net email sponsored by: ApacheCon 2003,
16-19 November in Las Vegas. Learn firsthand the latest
developments in Apache, PHP, Perl, XML, Java, MySQL,
WebDAV, and more! http://www.apachecon.com/
_______________________________________________
linux-usb-devel@lists.sourceforge.net
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel

Re: usb-storage and Sony Handycam

[Alan Stern]

On Fri, 7 Nov 2003, Patrick Mansfield wrote:

> > On Fri, 7 Nov 2003, Dmitri Katchalov wrote:
> > > If I change interpret_usb_result() in transport.c 
> > > to ignore "babble" then the next REQUEST-SENSE 
> > > reports ILLEGAL REQUEST, SCSI handles that and 
> > > everything sort of works.
> 
> I don't see how that worked, I would expect nearly the same command
> sequence, since scsi core is not checking the error. 
> 
> USB storage debug logs are needed to verify what happened (when ignoring
> babble/overflow).

In any case, usb-storage should _never_ ignore a "babble" error.  It 
indicates a serious communications problem.

Alan Stern



-------------------------------------------------------
This SF.Net email sponsored by: ApacheCon 2003,
16-19 November in Las Vegas. Learn firsthand the latest
developments in Apache, PHP, Perl, XML, Java, MySQL,
WebDAV, and more! http://www.apachecon.com/
_______________________________________________
linux-usb-devel@lists.sourceforge.net
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel

Re: usb-storage and Sony Handycam

[Dmitri Katchalov]

Quoting Alan Stern <stern@rowland.harvard.edu>:

> On Fri, 7 Nov 2003, Patrick Mansfield wrote:
> 
> > > On Fri, 7 Nov 2003, Dmitri Katchalov wrote:
> > > > If I change interpret_usb_result() in transport.c 
> > > > to ignore "babble" then the next REQUEST-SENSE 
> > > > reports ILLEGAL REQUEST, SCSI handles that and 
> > > > everything sort of works.
> > 
> > I don't see how that worked, I would expect nearly the same command
> > sequence, since scsi core is not checking the error. 
> > 
> > USB storage debug logs are needed to verify what happened (when ignoring
> > babble/overflow).
> 
> In any case, usb-storage should _never_ ignore a "babble" error.  It 
> indicates a serious communications problem.

Well, when I said "ignore babble" I actually did this:
      case -EOVERFLOW:
-          return USB_STOR_XFER_LONG;
+          return USB_STOR_XFER_STALL;
I agree, it was not a good idea. I only tried it to see what happens.

As I said, my camera works just fine. All I need is a module parameter 
in modprobe.conf (or a new entry in scsi_devinfo.c), no other code changes
required. I just thought you may be interested in this rather peculiar
behaviour.

Regards,
Dmitri




-------------------------------------------------------
This SF.Net email sponsored by: ApacheCon 2003,
16-19 November in Las Vegas. Learn firsthand the latest
developments in Apache, PHP, Perl, XML, Java, MySQL,
WebDAV, and more! http://www.apachecon.com/
_______________________________________________
linux-usb-devel@lists.sourceforge.net
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel

Re: usb-storage and Sony Handycam

[Patrick Mansfield]

On Sat, Nov 08, 2003 at 01:54:11PM +1100, Dmitri Katchalov wrote:

> Well, when I said "ignore babble" I actually did this:
>       case -EOVERFLOW:
> -          return USB_STOR_XFER_LONG;
> +          return USB_STOR_XFER_STALL;
> I agree, it was not a good idea. I only tried it to see what happens.
> 
> As I said, my camera works just fine. All I need is a module parameter 
> in modprobe.conf (or a new entry in scsi_devinfo.c), no other code changes
> required. I just thought you may be interested in this rather peculiar
> behaviour.

We need to figure out what is different here, and why (based on other
reports for this device) this worked on 2.4 but not on 2.6.

Looking at usb/storage/transport.c, we are not generating the usb
"fake_sense" with your change.

Maybe the failed MODE SENSE had a check condition, but the fake_sense
prevented us from retrieving it?

So the next command sent got a check condition, illegal request.

That still doesn't tell us why the MODE SENSE failed, and why the failure
does not show up with 2.4.

Can you run the variants on MODE SENE via user space SG_IO, and figure out
which ones work?

AFAICT at least three different problems are contributing to the failure:

1) The MODE SENSE seems to work fine in 2.4, but is failing in 2.6. In
2.4, we always send a MODE SENSE page 3f with length 255; in 2.6 we start by
asking for 4 bytes.

2) scsi core ignores host byte errors (DID_ERROR). This means we behave as
if the MODE SENSE worked OK, even though USB tells scsi core that it
failed. Fixing this problem (for another user) caused even worse problems.

3) The next command - TEST UNIT READY - gets a check condition, illegal
request. Maybe the fake sense handling is screwing things up - the device
tells us it sent back more data than we asked for, but it is also trying
to check us.

-- Patrick Mansfield


-------------------------------------------------------
This SF.Net email sponsored by: ApacheCon 2003,
16-19 November in Las Vegas. Learn firsthand the latest
developments in Apache, PHP, Perl, XML, Java, MySQL,
WebDAV, and more! http://www.apachecon.com/
_______________________________________________
linux-usb-devel@lists.sourceforge.net
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel

Re: usb-storage and Sony Handycam

[Dmitri Katchalov]

Quoting Patrick Mansfield <patmans@us.ibm.com>:

> We need to figure out what is different here, and why (based on other
> reports for this device) this worked on 2.4 but not on 2.6.

USB log from "the other" OS is attached for your perusal.
I've compressed and parsed it a little. From what I see the device 
is anything but compliant, but then it does not claim to be.


Regards,
Dmitri

GET_DESCRIPTOR_FROM_DEVICE		
Descriptor Type: 0x0001 DEVICE
0000: 12 01 00 01 00 00 00 08 4c 05 2e 00 00 03 01 02 
0010: 00 01 
    bLength            : 0x12 (18)
    bDescriptorType    : 0x01 (1)
    bcdUSB             : 0x0100 (256)
    bDeviceClass       : 0x00 (0)
    bDeviceSubClass    : 0x00 (0)
    bDeviceProtocol    : 0x00 (0)
    bMaxPacketSize0    : 0x08 (8)
    idVendor           : 0x054c (1356)
    idProduct          : 0x002e (46)
    bcdDevice          : 0x0300 (768)
    iManufacturer      : 0x01 (1)
    iProduct           : 0x02 (2)
    iSerialNumber      : 0x00 (0)
    bNumConfigurations : 0x01 (1)


GET_DESCRIPTOR_FROM_DEVICE		
Descriptor Type: 0x0002   CONFIGURATION
0000: 09 02 27 00 01 01 00 40 01 09 04 00 00 03 08 ff 
0010: 01 00 07 05 01 02 40 00 00 07 05 82 02 40 00 00 
0020: 07 05 83 03 08 00 ff 
    bLength            : 0x09 (9)
    bDescriptorType    : 0x02 (2)
    wTotalLength       : 0x0027 (39)
    bNumInterfaces     : 0x01 (1)
    bConfigurationValue: 0x01 (1)
    iConfiguration     : 0x00 (0)
    bmAttributes       : 0x40 (64)
    MaxPower           : 0x01 (1)

SELECT_CONFIGURATION		
Configuration Descriptor:
bLength: 9 (0x09)
bDescriptorType: 2 (0x02)
wTotalLength: 39 (0x0027)
bNumInterfaces: 1 (0x01)
bConfigurationValue: 1 (0x01)
iConfiguration: 0 (0x00)
bmAttributes: 64 (0x40)
  0x40: Self Powered
MaxPower: 1 (0x01)
  (in 2 mA units, therefore 2 mA power consumption)

Number of interfaces: 1
Interface[0]:
  Length: 0x004c
  InterfaceNumber: 0x00
  AlternateSetting: 0x00
  Class             = 0x34
  SubClass          = 0x6d
  Protocol          = 0x37
  InterfaceHandle   = 0x00000000
  NumberOfPipes     = 0x00000003
  Pipe[0]:
    MaximumPacketSize = 0xaff0
    EndpointAddress   = 0x4b
    Interval          = 0xc1
    PipeType          = 0x00
      UsbdPipeTypeControl
    PipeHandle        = 0x0000ce54
    MaxTransferSize   = 0x00010000
    PipeFlags         = 0x00
  Pipe[1]:
    MaximumPacketSize = 0x2f68
    EndpointAddress   = 0x5b
    Interval          = 0xbc
    PipeType          = 0x206a2b1b
      !!! INVALID !!!
    PipeHandle        = 0x4e4f5300
    MaxTransferSize   = 0x00010000
    PipeFlags         = 0x00
  Pipe[2]:
    MaximumPacketSize = 0x000d
    EndpointAddress   = 0x00
    Interval          = 0x00
    PipeType          = 0xc15667d0
      !!! INVALID !!!
    PipeHandle        = 0xc1566810
    MaxTransferSize   = 0x00010000
    PipeFlags         = 0x00

SELECT_CONFIGURATION
Configuration Descriptor:
bLength: 9 (0x09)
bDescriptorType: 2 (0x02)
wTotalLength: 39 (0x0027)
bNumInterfaces: 1 (0x01)
bConfigurationValue: 1 (0x01)
iConfiguration: 0 (0x00)
bmAttributes: 64 (0x40)
  0x40: Self Powered
MaxPower: 1 (0x01)
  (in 2 mA units, therefore 2 mA power consumption)

Number of interfaces: 1
Interface[0]:
  Length: 0x004c
  InterfaceNumber: 0x00
  AlternateSetting: 0x00
  Class             = 0x08
  SubClass          = 0xff
  Protocol          = 0x01
  InterfaceHandle   = 0xc1567700
  NumberOfPipes     = 0x00000003
  Pipe[0]:
    MaximumPacketSize = 0x0040
    EndpointAddress   = 0x01
    Interval          = 0x00
    PipeType          = 0x02
      UsbdPipeTypeBulk
    PipeHandle        = 0xc1567718
    MaxTransferSize   = 0x00010000
    PipeFlags         = 0x00
  Pipe[1]:
    MaximumPacketSize = 0x0040
    EndpointAddress   = 0x82
    Interval          = 0x00
    PipeType          = 0x02
      UsbdPipeTypeBulk
    PipeHandle        = 0xc156772c
    MaxTransferSize   = 0x00010000
    PipeFlags         = 0x00
  Pipe[2]:
    MaximumPacketSize = 0x0008
    EndpointAddress   = 0x83
    Interval          = 0xff
    PipeType          = 0x03
      UsbdPipeTypeInterrupt
    PipeHandle        = 0xc1567740
    MaxTransferSize   = 0x00010000
    PipeFlags         = 0x00

---------------------------------------------
out 0c (12) length
0000: 12 00 00 00 24 00 00 00 00 00 00 00       INQUIRY

in 0x00000024 (36) length                       
0000: 00 80 00 01 1f 00 00 00 53 6f 6e 79 20 20 20 20 
0010: 53 6f 6e 79 20 44 53 43 20 20 20 20 20 20 20 20 
0020: 33 2e 30 30 

out 0x0000000c (12) length
0000: 25 00 00 00 00 00 00 00 00 00 00 00       READ CAPACITY

in 0x00000008 (8) length
0000: 00 00 1e df 00 00 02 00                   7903 blocks x 512 = 4,046,366 
bytes

out 0x0000000c (12) length
0000: 00 00 00 00 00 00 00 00 00 00 00 00       TEST UNIT READY

out 0x0000000c (12) length
0000: 25 00 00 00 00 00 00 00 00 00 00 00       READ CAPACITY

in 0x00000008 (8) length
0000: 00 00 1e df 00 00 02 00                   7903 blocks x 512 = 4,046,366 
bytes

out 0x0000000c (12) length
0000: 28 00 00 00 00 00 00 00 80 00 00 00       READ 128 blocks, starting from 0

<end of log>





-------------------------------------------------------
This SF.Net email sponsored by: ApacheCon 2003,
16-19 November in Las Vegas. Learn firsthand the latest
developments in Apache, PHP, Perl, XML, Java, MySQL,
WebDAV, and more! http://www.apachecon.com/
_______________________________________________
linux-usb-devel@lists.sourceforge.net
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel

Re: usb-storage and Sony Handycam

[Alan Stern]

On Fri, 7 Nov 2003, Patrick Mansfield wrote:

> We need to figure out what is different here, and why (based on other
> reports for this device) this worked on 2.4 but not on 2.6.
> 
> Looking at usb/storage/transport.c, we are not generating the usb
> "fake_sense" with your change.
> 
> Maybe the failed MODE SENSE had a check condition, but the fake_sense
> prevented us from retrieving it?

No, the "babble" prevented that.

How to deal with babble errors is a serious question that's been under
discussion in the usb-storage mailing list, but we haven't found a good
answer.  It doesn't come up often except for certain devices that go crazy
when given some of the less-common commands.  Babble is a nasty violation
of the USB protocol (and in particular the USB Mass Storage class
protocols); it indicates that something has gone very wrong but doesn't
provide any clue as to how to fix it.  I vaguely recall getting a report
from someone that even unplugging the USB cable and reinserting it wasn't
enough to unwedge his device.

The "fake sense" is a compromise.  It lets us give a clear indication to 
the higher layers that something is wrong and asks them not to send the 
same command again.  But it doesn't do much about getting the device back 
into a working state (if that's even possible).

> So the next command sent got a check condition, illegal request.
> 
> That still doesn't tell us why the MODE SENSE failed, and why the failure
> does not show up with 2.4.

The best approach for doing this is to have a side-by-side comparison of 
the usb-storage debugging logs for 2.4 and 2.6.  I think we have to assume 
that the MODE-SENSE failed because the device simply doesn't support it 
correctly.

> Can you run the variants on MODE SENE via user space SG_IO, and figure out
> which ones work?
> 
> AFAICT at least three different problems are contributing to the failure:
> 
> 1) The MODE SENSE seems to work fine in 2.4, but is failing in 2.6. In
> 2.4, we always send a MODE SENSE page 3f with length 255; in 2.6 we start by
> asking for 4 bytes.
> 
> 2) scsi core ignores host byte errors (DID_ERROR). This means we behave as
> if the MODE SENSE worked OK, even though USB tells scsi core that it
> failed. Fixing this problem (for another user) caused even worse problems.

I must have missed that.  It seems odd that fixing an oversight in the
kernel could make interoperability _worse_.  Maybe that indicates
something else needs to be fixed.

> 3) The next command - TEST UNIT READY - gets a check condition, illegal
> request. Maybe the fake sense handling is screwing things up - the device
> tells us it sent back more data than we asked for, but it is also trying
> to check us.

I don't understand your speculation here.  It seems clear to me that this 
is a situation where the device and the host are out of sync -- the host 
sends a command and the device sends back status from the _previous_ 
command.  Repeating the TEST-UNIT-READY may bring us back in sync.  Or 
maybe some sort of reset will -- no way to tell without trying it.

The ultimate problem is that this device doesn't like some of the 
MODE-SENSE commands used to determine read-write and cache status.  We 
need to have either per-device information saying which variants of the  
commands the device will accept or else per-device information saying not 
to send the MODE-SENSE commands at all.

I don't see any other way of coping.  Even trying to send a command and 
then trying to recover if it fails is not a viable approach if recovery 
requires the user to cycle the power on the device (or to unplug its 
cable).

Alan Stern



-------------------------------------------------------
This SF.Net email sponsored by: ApacheCon 2003,
16-19 November in Las Vegas. Learn firsthand the latest
developments in Apache, PHP, Perl, XML, Java, MySQL,
WebDAV, and more! http://www.apachecon.com/
_______________________________________________
linux-usb-devel@lists.sourceforge.net
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel

Re: usb-storage and Sony Handycam

[Patrick Mansfield]

On Sat, Nov 08, 2003 at 11:28:37AM -0500, Alan Stern wrote:
> On Fri, 7 Nov 2003, Patrick Mansfield wrote:

> > Looking at usb/storage/transport.c, we are not generating the usb
> > "fake_sense" with your change.
> > 
> > Maybe the failed MODE SENSE had a check condition, but the fake_sense
> > prevented us from retrieving it?
> 
> No, the "babble" prevented that.

> The "fake sense" is a compromise.  It lets us give a clear indication to 
> the higher layers that something is wrong and asks them not to send the 
> same command again.  But it doesn't do much about getting the device back 
> into a working state (if that's even possible).

Looking closer at the USB code, is the fake_sense code even hit? The logs
do not show any of the US_DEBUGP in usb_stor_Bulk_transport(), like
"Bulk Command S ...".

A DID_ERROR causes scsi core to retry the command - it will not even check
the sense data. A DID_ABORT might be better, scsi core will not retry.
(We still have issue 1, where the host byte is not checked in
scsi_status_is_good.)

> > So the next command sent got a check condition, illegal request.
> > 
> > That still doesn't tell us why the MODE SENSE failed, and why the failure
> > does not show up with 2.4.
> 
> The best approach for doing this is to have a side-by-side comparison of 
> the usb-storage debugging logs for 2.4 and 2.6.

Yes, we do not have a 2.4 trace of the same device.

> I think we have to assume 
> that the MODE-SENSE failed because the device simply doesn't support it 
> correctly.
> 
> > Can you run the variants on MODE SENE via user space SG_IO, and figure out
> > which ones work?

Still no data on the above (from Dmitri or Ronald).

> > 3) The next command - TEST UNIT READY - gets a check condition, illegal
> > request. Maybe the fake sense handling is screwing things up - the device
> > tells us it sent back more data than we asked for, but it is also trying
> > to check us.
> 
> I don't understand your speculation here.  It seems clear to me that this 
> is a situation where the device and the host are out of sync -- the host 
> sends a command and the device sends back status from the _previous_ 
> command.  Repeating the TEST-UNIT-READY may bring us back in sync.  Or 
> maybe some sort of reset will -- no way to tell without trying it.

The sense data is not cleared until it is retrieved, so if there is sense
waiting, and we do not send a REQUEST SENSE, the next REQUEST SENSE will
retrieve the pre-existing sense data (see SAM-3, I am looking at
sam3r07.pdf, section 5.9.6 sense data).

If the actual result of the command was checked in the babble cases, and a
REQUEST SENSE sent, it might have gotten the ILLEGAL REQUEST, and the
devices sense data might be cleared. It might be bad to do anything for
babble cases, but the REQUEST SENSE should be sent to at least clear out
any real sense data if we are going to generate our own fake sense.

Dmitri's change (return USB_STOR_XFER_STALL instead of USB_STOR_XFER_LONG)
causes us to return USB_STOR_TRANSPORT_GOOD vs USB_STOR_TRANSPORT_ERROR in
usb_stor_CB_transport. 

Then in usb_stor_invoke_transport we issue a REQUEST SENSE. 

If for a USB_STOR_TRANSPORT_ERROR we also did a REQUEST SENSE, the
following command (TEST UNIT READY) would not get the ILLEGAL REQUEST.

I don't know why the TEST UNIT READY caused an auto REQUEST SENSE to
be sent.

Changing the sense stuff does not address the MODE SENSE failing, we still
have 3 separate issues that are all hit by this device.

-- Patrick Mansfield


-------------------------------------------------------
This SF.Net email sponsored by: ApacheCon 2003,
16-19 November in Las Vegas. Learn firsthand the latest
developments in Apache, PHP, Perl, XML, Java, MySQL,
WebDAV, and more! http://www.apachecon.com/
_______________________________________________
linux-usb-devel@lists.sourceforge.net
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel

Re: [linux-usb-devel] usb-storage and Sony Handycam

[Alan Stern]

On Sat, 8 Nov 2003, Patrick Mansfield wrote:

> On Sat, Nov 08, 2003 at 11:28:37AM -0500, Alan Stern wrote:
> 
> > The "fake sense" is a compromise.  It lets us give a clear indication to 
> > the higher layers that something is wrong and asks them not to send the 
> > same command again.  But it doesn't do much about getting the device back 
> > into a working state (if that's even possible).
> 
> Looking closer at the USB code, is the fake_sense code even hit? The logs
> do not show any of the US_DEBUGP in usb_stor_Bulk_transport(), like
> "Bulk Command S ...".

It probably wasn't hit.  The fake sense data is only sent for devices 
using the Bulk-only transport, and Dmitri's and Idan's devices use the 
Control-Bulk transport IIRC.

> A DID_ERROR causes scsi core to retry the command - it will not even check
> the sense data. A DID_ABORT might be better, scsi core will not retry.
> (We still have issue 1, where the host byte is not checked in
> scsi_status_is_good.)

For CB and CBI devices, babble causes a DID_ERROR return.  For Bulk-only 
it causes a Check-Condition status to be returned along with Invalid 
Command sense data.  Like I said earlier, we haven't yet figured out the 
best way to handle these errors.

> > > That still doesn't tell us why the MODE SENSE failed, and why the failure
> > > does not show up with 2.4.
> > 
> > The best approach for doing this is to have a side-by-side comparison of 
> > the usb-storage debugging logs for 2.4 and 2.6.
> 
> Yes, we do not have a 2.4 trace of the same device.
> 
> > I think we have to assume 
> > that the MODE-SENSE failed because the device simply doesn't support it 
> > correctly.
> > 
> > > Can you run the variants on MODE SENE via user space SG_IO, and figure out
> > > which ones work?
> 
> Still no data on the above (from Dmitri or Ronald).

Doing that will probably be a waste of time.  I firmly believe that there 
is _no_ least common denominator among USB mass storage devices: Some 
work with any MODE-SENSE command, some work with only a few variants, 
others work with others.  Maybe some devices don't work with MODE-SENSE at 
all.  In any case, there isn't any single combination of 
pages/transfer-lengths that will be accepted by every device.  And when a 
device fails to accept such a command it often does so catastrophically, 
requiring a power-cycle to start working again.

The only safe course is to avoid sending these commands at all to the 
devices that can't handle them.

> > > 3) The next command - TEST UNIT READY - gets a check condition, illegal
> > > request. Maybe the fake sense handling is screwing things up - the device
> > > tells us it sent back more data than we asked for, but it is also trying
> > > to check us.
> > 
> > I don't understand your speculation here.  It seems clear to me that this 
> > is a situation where the device and the host are out of sync -- the host 
> > sends a command and the device sends back status from the _previous_ 
> > command.  Repeating the TEST-UNIT-READY may bring us back in sync.  Or 
> > maybe some sort of reset will -- no way to tell without trying it.
> 
> The sense data is not cleared until it is retrieved, so if there is sense
> waiting, and we do not send a REQUEST SENSE, the next REQUEST SENSE will
> retrieve the pre-existing sense data (see SAM-3, I am looking at
> sam3r07.pdf, section 5.9.6 sense data).

Hmmm...  I'm more used to an earlier version of the SCSI protocol,
according to which old sense data is lost whenever a new command is
received unless that command is REQUEST-SENSE.

> If the actual result of the command was checked in the babble cases, and a
> REQUEST SENSE sent, it might have gotten the ILLEGAL REQUEST, and the
> devices sense data might be cleared. It might be bad to do anything for
> babble cases, but the REQUEST SENSE should be sent to at least clear out
> any real sense data if we are going to generate our own fake sense.

There's isn't any reliable way to get the command result or the sense data
after a babble occurs -- in fact, there isn't a reliable way to
re-establish communication with the device at all.  My best thought on
this has been to try reading (and throwing away) data from the device
until a 5-second (for example) timeout expires without receiving anything,
then issue a device reset.  That's more likely to recover from this sort
of error than anything else I can think of, but it's never been 
implemented.

I have seen logs from users where a device reset was sent following a 
babble error.  It didn't help; the device remained wedged.

The USB Mass Storage specification documents are silent on the issue of
babble.  And even if they weren't it probably wouldn't help; the only
reason these devices start babbling is because they aren't compliant with
the standards in the first place.

> Dmitri's change (return USB_STOR_XFER_STALL instead of USB_STOR_XFER_LONG)
> causes us to return USB_STOR_TRANSPORT_GOOD vs USB_STOR_TRANSPORT_ERROR in
> usb_stor_CB_transport. 
> 
> Then in usb_stor_invoke_transport we issue a REQUEST SENSE. 
> 
> If for a USB_STOR_TRANSPORT_ERROR we also did a REQUEST SENSE, the
> following command (TEST UNIT READY) would not get the ILLEGAL REQUEST.

That doesn't make sense.  USB_STOR_TRANSPORT_ERROR means we aren't able to 
communicate with the device.  So how can we send it a REQUEST-SENSE?

> I don't know why the TEST UNIT READY caused an auto REQUEST SENSE to
> be sent.

The CB transport doesn't include any means for the device to return
status.  (Yes, it's stupid.)  The only way we can find out is to issue
auto REQUEST-SENSE for virtually every command.

> Changing the sense stuff does not address the MODE SENSE failing, we still
> have 3 separate issues that are all hit by this device.

If we simply avoided sending the MODE-SENSE in the first place, none of 
the other issues would come up.  IMHO that's the best solution.

Alan Stern

Re: [linux-usb-devel] usb-storage and Sony Handycam

[Dmitri Katchalov]

[My previous msg didn't make it to the list, resending]

Attached here for your perusal is USB log for this device on win98.

Regards,
Dmitri


1	in down	n/a	0.062	GET_DESCRIPTOR_FROM_DEVICE		
URB Header (length: 80)
SequenceNumber: 1
Function: 000b (GET_DESCRIPTOR_FROM_DEVICE)
1	in up	n/a	0.066	CONTROL_TRANSFER	12 01 00 01 00 00 00 08
	0x00000000
URB Header (length: 80)
SequenceNumber: 1
Function: 0008 (CONTROL_TRANSFER)
PipeHandle: c1565d8c

SetupPacket:
0000: 80 06 00 01 00 00 12 00 
bmRequestType: 80
  DIR: Device-To-Host
  TYPE: Standard
  RECIPIENT: Device
bRequest: 06  
  GET_DESCRIPTOR
Descriptor Type: 0x0001
  DEVICE


TransferBuffer: 0x00000012 (18) length
0000: 12 01 00 01 00 00 00 08 4c 05 2e 00 00 03 01 02 
0010: 00 01 
    bLength            : 0x12 (18)
    bDescriptorType    : 0x01 (1)
    bcdUSB             : 0x0100 (256)
    bDeviceClass       : 0x00 (0)
    bDeviceSubClass    : 0x00 (0)
    bDeviceProtocol    : 0x00 (0)
    bMaxPacketSize0    : 0x08 (8)
    idVendor           : 0x054c (1356)
    idProduct          : 0x002e (46)
    bcdDevice          : 0x0300 (768)
    iManufacturer      : 0x01 (1)
    iProduct           : 0x02 (2)
    iSerialNumber      : 0x00 (0)
    bNumConfigurations : 0x01 (1)
2	in down	n/a	0.066	GET_DESCRIPTOR_FROM_DEVICE		
URB Header (length: 80)
SequenceNumber: 2
Function: 000b (GET_DESCRIPTOR_FROM_DEVICE)
2	in up	n/a	0.076	CONTROL_TRANSFER	09 02 27 00 01 01 00 40
	0x00000000
URB Header (length: 80)
SequenceNumber: 2
Function: 0008 (CONTROL_TRANSFER)
PipeHandle: c1565d8c

SetupPacket:
0000: 80 06 00 02 00 00 09 02 
bmRequestType: 80
  DIR: Device-To-Host
  TYPE: Standard
  RECIPIENT: Device
bRequest: 06  
  GET_DESCRIPTOR
Descriptor Type: 0x0002
  CONFIGURATION


TransferBuffer: 0x00000027 (39) length
0000: 09 02 27 00 01 01 00 40 01 09 04 00 00 03 08 ff 
0010: 01 00 07 05 01 02 40 00 00 07 05 82 02 40 00 00 
0020: 07 05 83 03 08 00 ff 
    bLength            : 0x09 (9)
    bDescriptorType    : 0x02 (2)
    wTotalLength       : 0x0027 (39)
    bNumInterfaces     : 0x01 (1)
    bConfigurationValue: 0x01 (1)
    iConfiguration     : 0x00 (0)
    bmAttributes       : 0x40 (64)
    MaxPower           : 0x01 (1)
3	??? down	n/a	0.076	SELECT_CONFIGURATION		
URB Header (length: 100)
SequenceNumber: 3
Function: 0000 (SELECT_CONFIGURATION)
Configuration Descriptor:
bLength: 9 (0x09)
bDescriptorType: 2 (0x02)
wTotalLength: 39 (0x0027)
bNumInterfaces: 1 (0x01)
bConfigurationValue: 1 (0x01)
iConfiguration: 0 (0x00)
bmAttributes: 64 (0x40)
  0x40: Self Powered
MaxPower: 1 (0x01)
  (in 2 mA units, therefore 2 mA power consumption)

Number of interfaces: 1
Interface[0]:
  Length: 0x004c
  InterfaceNumber: 0x00
  AlternateSetting: 0x00
  Class             = 0x34
  SubClass          = 0x6d
  Protocol          = 0x37
  InterfaceHandle   = 0x00000000
  NumberOfPipes     = 0x00000003
  Pipe[0]:
    MaximumPacketSize = 0xaff0
    EndpointAddress   = 0x4b
    Interval          = 0xc1
    PipeType          = 0x00
      UsbdPipeTypeControl
    PipeHandle        = 0x0000ce54
    MaxTransferSize   = 0x00010000
    PipeFlags         = 0x00
  Pipe[1]:
    MaximumPacketSize = 0x2f68
    EndpointAddress   = 0x5b
    Interval          = 0xbc
    PipeType          = 0x206a2b1b
      !!! INVALID !!!
    PipeHandle        = 0x4e4f5300
    MaxTransferSize   = 0x00010000
    PipeFlags         = 0x00
  Pipe[2]:
    MaximumPacketSize = 0x000d
    EndpointAddress   = 0x00
    Interval          = 0x00
    PipeType          = 0xc15667d0
      !!! INVALID !!!
    PipeHandle        = 0xc1566810
    MaxTransferSize   = 0x00010000
    PipeFlags         = 0x00
3	??? up	n/a	0.076	SELECT_CONFIGURATION		0x00000000
URB Header (length: 100)
SequenceNumber: 3
Function: 0000 (SELECT_CONFIGURATION)
Configuration Descriptor:
bLength: 9 (0x09)
bDescriptorType: 2 (0x02)
wTotalLength: 39 (0x0027)
bNumInterfaces: 1 (0x01)
bConfigurationValue: 1 (0x01)
iConfiguration: 0 (0x00)
bmAttributes: 64 (0x40)
  0x40: Self Powered
MaxPower: 1 (0x01)
  (in 2 mA units, therefore 2 mA power consumption)

Number of interfaces: 1
Interface[0]:
  Length: 0x004c
  InterfaceNumber: 0x00
  AlternateSetting: 0x00
  Class             = 0x08
  SubClass          = 0xff
  Protocol          = 0x01
  InterfaceHandle   = 0xc1567700
  NumberOfPipes     = 0x00000003
  Pipe[0]:
    MaximumPacketSize = 0x0040
    EndpointAddress   = 0x01
    Interval          = 0x00
    PipeType          = 0x02
      UsbdPipeTypeBulk
    PipeHandle        = 0xc1567718
    MaxTransferSize   = 0x00010000
    PipeFlags         = 0x00
  Pipe[1]:
    MaximumPacketSize = 0x0040
    EndpointAddress   = 0x82
    Interval          = 0x00
    PipeType          = 0x02
      UsbdPipeTypeBulk
    PipeHandle        = 0xc156772c
    MaxTransferSize   = 0x00010000
    PipeFlags         = 0x00
  Pipe[2]:
    MaximumPacketSize = 0x0008
    EndpointAddress   = 0x83
    Interval          = 0xff
    PipeType          = 0x03
      UsbdPipeTypeInterrupt
    PipeHandle        = 0xc1567740
    MaxTransferSize   = 0x00010000
    PipeFlags         = 0x00
4	out down	n/a	3.142	CLASS_INTERFACE	12 00 00 00 24 00 00 00	
URB Header (length: 80)
SequenceNumber: 4
Function: 001b (CLASS_INTERFACE)
PipeHandle: c146f640

SetupPacket:
0000: 21 00 00 00 00 00 00 00 
bmRequestType: 21
  DIR: Host-To-Device
  TYPE: Class
  RECIPIENT: Interface
bRequest: 00  


TransferBuffer: 0x0000000c (12) length
0000: 12 00 00 00 24 00 00 00 00 00 00 00 
4	out up	n/a	3.146	CONTROL_TRANSFER	-	0x00000000
URB Header (length: 80)
SequenceNumber: 4
Function: 0008 (CONTROL_TRANSFER)
PipeHandle: c1565d8c

SetupPacket:
0000: 21 00 00 00 00 00 0c 00 
bmRequestType: 21
  DIR: Host-To-Device
  TYPE: Class
  RECIPIENT: Interface
bRequest: 00  


No TransferBuffer

5	in down	0x82	3.146	BULK_OR_INTERRUPT_TRANSFER	-	
URB Header (length: 72)
SequenceNumber: 5
Function: 0009 (BULK_OR_INTERRUPT_TRANSFER)
TransferFlags: 0x00000003

No TransferBuffer

5	in up	0x82	3.146	BULK_OR_INTERRUPT_TRANSFER	00 80 00 01 1f 
00 00 00	0x00000000
URB Header (length: 72)
SequenceNumber: 5
Function: 0009 (BULK_OR_INTERRUPT_TRANSFER)
TransferFlags: 0x00000003

TransferBuffer: 0x00000024 (36) length
0000: 00 80 00 01 1f 00 00 00 53 6f 6e 79 20 20 20 20 
0010: 53 6f 6e 79 20 44 53 43 20 20 20 20 20 20 20 20 
0020: 33 2e 30 30 
6	in down	0x82	3.146	CLASS_INTERFACE	25 00 00 00 00 00 00 00	
URB Header (length: 80)
SequenceNumber: 6
Function: 001b (CLASS_INTERFACE)
PipeHandle: c156772c

SetupPacket:
0000: 21 00 00 00 00 00 0c 00 
bmRequestType: 21
  DIR: Host-To-Device
  TYPE: Class
  RECIPIENT: Interface
bRequest: 00  


TransferBuffer: 0x0000000c (12) length
0000: 25 00 00 00 00 00 00 00 00 00 00 00 
6	out up	n/a	3.152	CONTROL_TRANSFER	-	0x00000000
URB Header (length: 80)
SequenceNumber: 6
Function: 0008 (CONTROL_TRANSFER)
PipeHandle: c1565d8c

SetupPacket:
0000: 21 00 00 00 00 00 0c 00 
bmRequestType: 21
  DIR: Host-To-Device
  TYPE: Class
  RECIPIENT: Interface
bRequest: 00  


No TransferBuffer

7	in down	0x82	3.152	BULK_OR_INTERRUPT_TRANSFER	-	
URB Header (length: 72)
SequenceNumber: 7
Function: 0009 (BULK_OR_INTERRUPT_TRANSFER)
TransferFlags: 0x00000003

No TransferBuffer

7	in up	0x82	3.156	BULK_OR_INTERRUPT_TRANSFER	00 00 1e df 00 
00 02 00	0x00000000
URB Header (length: 72)
SequenceNumber: 7
Function: 0009 (BULK_OR_INTERRUPT_TRANSFER)
TransferFlags: 0x00000003

TransferBuffer: 0x00000008 (8) length
0000: 00 00 1e df 00 00 02 00 
8	in down	0x82	3.166	CLASS_INTERFACE	00 00 00 00 00 00 00 00	
URB Header (length: 80)
SequenceNumber: 8
Function: 001b (CLASS_INTERFACE)
PipeHandle: c156772c

SetupPacket:
0000: 21 00 00 00 00 00 0c 00 
bmRequestType: 21
  DIR: Host-To-Device
  TYPE: Class
  RECIPIENT: Interface
bRequest: 00  


TransferBuffer: 0x0000000c (12) length
0000: 00 00 00 00 00 00 00 00 00 00 00 00 
8	out up	n/a	3.171	CONTROL_TRANSFER	-	0x00000000
URB Header (length: 80)
SequenceNumber: 8
Function: 0008 (CONTROL_TRANSFER)
PipeHandle: c1565d8c

SetupPacket:
0000: 21 00 00 00 00 00 0c 00 
bmRequestType: 21
  DIR: Host-To-Device
  TYPE: Class
  RECIPIENT: Interface
bRequest: 00  


No TransferBuffer

9	out down	n/a	3.172	CLASS_INTERFACE	25 00 00 00 00 00 00 00	
URB Header (length: 80)
SequenceNumber: 9
Function: 001b (CLASS_INTERFACE)
PipeHandle: c1565d8c

SetupPacket:
0000: 21 00 00 00 00 00 0c 00 
bmRequestType: 21
  DIR: Host-To-Device
  TYPE: Class
  RECIPIENT: Interface
bRequest: 00  


TransferBuffer: 0x0000000c (12) length
0000: 25 00 00 00 00 00 00 00 00 00 00 00 
9	out up	n/a	3.176	CONTROL_TRANSFER	-	0x00000000
URB Header (length: 80)
SequenceNumber: 9
Function: 0008 (CONTROL_TRANSFER)
PipeHandle: c1565d8c

SetupPacket:
0000: 21 00 00 00 00 00 0c 00 
bmRequestType: 21
  DIR: Host-To-Device
  TYPE: Class
  RECIPIENT: Interface
bRequest: 00  


No TransferBuffer

10	in down	0x82	3.181	BULK_OR_INTERRUPT_TRANSFER	-	
URB Header (length: 72)
SequenceNumber: 10
Function: 0009 (BULK_OR_INTERRUPT_TRANSFER)
TransferFlags: 0x00000003

No TransferBuffer

10	in up	0x82	3.182	BULK_OR_INTERRUPT_TRANSFER	00 00 1e df 00 
00 02 00	0x00000000
URB Header (length: 72)
SequenceNumber: 10
Function: 0009 (BULK_OR_INTERRUPT_TRANSFER)
TransferFlags: 0x00000003

TransferBuffer: 0x00000008 (8) length
0000: 00 00 1e df 00 00 02 00 
11	in down	0x82	3.191	CLASS_INTERFACE	28 00 00 00 00 00 00 00	
URB Header (length: 80)
SequenceNumber: 11
Function: 001b (CLASS_INTERFACE)
PipeHandle: c156772c

SetupPacket:
0000: 21 00 00 00 00 00 0c 00 
bmRequestType: 21
  DIR: Host-To-Device
  TYPE: Class
  RECIPIENT: Interface
bRequest: 00  


TransferBuffer: 0x0000000c (12) length
0000: 28 00 00 00 00 00 00 00 80 00 00 00 
11	out up	n/a	3.196	CONTROL_TRANSFER	-	0x00000000
URB Header (length: 80)
SequenceNumber: 11
Function: 0008 (CONTROL_TRANSFER)
PipeHandle: c1565d8c

SetupPacket:
0000: 21 00 00 00 00 00 0c 00 
bmRequestType: 21
  DIR: Host-To-Device
  TYPE: Class
  RECIPIENT: Interface
bRequest: 00  


No TransferBuffer

12	in down	0x82	3.201	BULK_OR_INTERRUPT_TRANSFER	-	
URB Header (length: 72)
SequenceNumber: 12
Function: 0009 (BULK_OR_INTERRUPT_TRANSFER)
TransferFlags: 0x00000003

No TransferBuffer

Re: usb-storage and Sony Handycam

[Patrick Mansfield]

On Sun, Nov 09, 2003 at 07:45:46PM +1100, Dmitri Katchalov wrote:
> [My previous msg didn't make it to the list, resending]
> 
> Attached here for your perusal is USB log for this device on win98.

Thanks.

Can someone verify my decoding? (This matches what Dmitri sent earlier
that did not make it to the list, execept it looks like it starts with an
INQUIRY VPD page 0 - get a list of supported VPD pages).

I can't figure out the apparent nonsense returned for INQUIRY VPD page 0.

And the "DIR" output seems odd.

In short:

	INQUIRY VPD page 0
	INQUIRY length 0x24 (36) bytes
	READ CAPACITY
	TEST UNIT READY
	READ CAPACITY
	READ 10

Sure enough no MODE SENSE. 

And no WRITE to see if it is write protected.

Any logs for write protected USB media, or for the device with linux 2.4?

A few comments inlined below.

> 1	in down	n/a	0.062	GET_DESCRIPTOR_FROM_DEVICE		
> URB Header (length: 80)
> SequenceNumber: 1
> Function: 000b (GET_DESCRIPTOR_FROM_DEVICE)
> 1	in up	n/a	0.066	CONTROL_TRANSFER	12 01 00 01 00 00 00 08
> 	0x00000000
> URB Header (length: 80)
> SequenceNumber: 1
> Function: 0008 (CONTROL_TRANSFER)
> PipeHandle: c1565d8c
> 
> SetupPacket:
> 0000: 80 06 00 01 00 00 12 00 
> bmRequestType: 80
>   DIR: Device-To-Host
>   TYPE: Standard
>   RECIPIENT: Device
> bRequest: 06  
>   GET_DESCRIPTOR
> Descriptor Type: 0x0001
>   DEVICE
> 
> 
> TransferBuffer: 0x00000012 (18) length
> 0000: 12 01 00 01 00 00 00 08 4c 05 2e 00 00 03 01 02 
> 0010: 00 01 

INQUIRY (0x12) VPD page 0 request.

>     bLength            : 0x12 (18)
>     bDescriptorType    : 0x01 (1)
>     bcdUSB             : 0x0100 (256)
>     bDeviceClass       : 0x00 (0)
>     bDeviceSubClass    : 0x00 (0)
>     bDeviceProtocol    : 0x00 (0)
>     bMaxPacketSize0    : 0x08 (8)
>     idVendor           : 0x054c (1356)
>     idProduct          : 0x002e (46)
>     bcdDevice          : 0x0300 (768)
>     iManufacturer      : 0x01 (1)
>     iProduct           : 0x02 (2)
>     iSerialNumber      : 0x00 (0)
>     bNumConfigurations : 0x01 (1)
> 2	in down	n/a	0.066	GET_DESCRIPTOR_FROM_DEVICE		
> URB Header (length: 80)
> SequenceNumber: 2
> Function: 000b (GET_DESCRIPTOR_FROM_DEVICE)
> 2	in up	n/a	0.076	CONTROL_TRANSFER	09 02 27 00 01 01 00 40
> 	0x00000000
> URB Header (length: 80)
> SequenceNumber: 2
> Function: 0008 (CONTROL_TRANSFER)
> PipeHandle: c1565d8c
> 
> SetupPacket:
> 0000: 80 06 00 02 00 00 09 02 
> bmRequestType: 80
>   DIR: Device-To-Host
>   TYPE: Standard
>   RECIPIENT: Device
> bRequest: 06  
>   GET_DESCRIPTOR
> Descriptor Type: 0x0002
>   CONFIGURATION
> 
> 
> TransferBuffer: 0x00000027 (39) length
> 0000: 09 02 27 00 01 01 00 40 01 09 04 00 00 03 08 ff 
> 0010: 01 00 07 05 01 02 40 00 00 07 05 82 02 40 00 00 
> 0020: 07 05 83 03 08 00 ff 

WTF?

>     bLength            : 0x09 (9)
>     bDescriptorType    : 0x02 (2)
>     wTotalLength       : 0x0027 (39)
>     bNumInterfaces     : 0x01 (1)
>     bConfigurationValue: 0x01 (1)
>     iConfiguration     : 0x00 (0)
>     bmAttributes       : 0x40 (64)
>     MaxPower           : 0x01 (1)
> 3	??? down	n/a	0.076	SELECT_CONFIGURATION		
> URB Header (length: 100)
> SequenceNumber: 3
> Function: 0000 (SELECT_CONFIGURATION)
> Configuration Descriptor:
> bLength: 9 (0x09)
> bDescriptorType: 2 (0x02)
> wTotalLength: 39 (0x0027)
> bNumInterfaces: 1 (0x01)
> bConfigurationValue: 1 (0x01)
> iConfiguration: 0 (0x00)
> bmAttributes: 64 (0x40)
>   0x40: Self Powered
> MaxPower: 1 (0x01)
>   (in 2 mA units, therefore 2 mA power consumption)
> 
> Number of interfaces: 1
> Interface[0]:
>   Length: 0x004c
>   InterfaceNumber: 0x00
>   AlternateSetting: 0x00
>   Class             = 0x34
>   SubClass          = 0x6d
>   Protocol          = 0x37
>   InterfaceHandle   = 0x00000000
>   NumberOfPipes     = 0x00000003
>   Pipe[0]:
>     MaximumPacketSize = 0xaff0
>     EndpointAddress   = 0x4b
>     Interval          = 0xc1
>     PipeType          = 0x00
>       UsbdPipeTypeControl
>     PipeHandle        = 0x0000ce54
>     MaxTransferSize   = 0x00010000
>     PipeFlags         = 0x00
>   Pipe[1]:
>     MaximumPacketSize = 0x2f68
>     EndpointAddress   = 0x5b
>     Interval          = 0xbc
>     PipeType          = 0x206a2b1b
>       !!! INVALID !!!
>     PipeHandle        = 0x4e4f5300
>     MaxTransferSize   = 0x00010000
>     PipeFlags         = 0x00
>   Pipe[2]:
>     MaximumPacketSize = 0x000d
>     EndpointAddress   = 0x00
>     Interval          = 0x00
>     PipeType          = 0xc15667d0
>       !!! INVALID !!!
>     PipeHandle        = 0xc1566810
>     MaxTransferSize   = 0x00010000
>     PipeFlags         = 0x00
> 3	??? up	n/a	0.076	SELECT_CONFIGURATION		0x00000000
> URB Header (length: 100)
> SequenceNumber: 3
> Function: 0000 (SELECT_CONFIGURATION)
> Configuration Descriptor:
> bLength: 9 (0x09)
> bDescriptorType: 2 (0x02)
> wTotalLength: 39 (0x0027)
> bNumInterfaces: 1 (0x01)
> bConfigurationValue: 1 (0x01)
> iConfiguration: 0 (0x00)
> bmAttributes: 64 (0x40)
>   0x40: Self Powered
> MaxPower: 1 (0x01)
>   (in 2 mA units, therefore 2 mA power consumption)
> 
> Number of interfaces: 1
> Interface[0]:
>   Length: 0x004c
>   InterfaceNumber: 0x00
>   AlternateSetting: 0x00
>   Class             = 0x08
>   SubClass          = 0xff
>   Protocol          = 0x01
>   InterfaceHandle   = 0xc1567700
>   NumberOfPipes     = 0x00000003
>   Pipe[0]:
>     MaximumPacketSize = 0x0040
>     EndpointAddress   = 0x01
>     Interval          = 0x00
>     PipeType          = 0x02
>       UsbdPipeTypeBulk
>     PipeHandle        = 0xc1567718
>     MaxTransferSize   = 0x00010000
>     PipeFlags         = 0x00
>   Pipe[1]:
>     MaximumPacketSize = 0x0040
>     EndpointAddress   = 0x82
>     Interval          = 0x00
>     PipeType          = 0x02
>       UsbdPipeTypeBulk
>     PipeHandle        = 0xc156772c
>     MaxTransferSize   = 0x00010000
>     PipeFlags         = 0x00
>   Pipe[2]:
>     MaximumPacketSize = 0x0008
>     EndpointAddress   = 0x83
>     Interval          = 0xff
>     PipeType          = 0x03
>       UsbdPipeTypeInterrupt
>     PipeHandle        = 0xc1567740
>     MaxTransferSize   = 0x00010000
>     PipeFlags         = 0x00
> 4	out down	n/a	3.142	CLASS_INTERFACE	12 00 00 00 24 00 00 00	
> URB Header (length: 80)
> SequenceNumber: 4
> Function: 001b (CLASS_INTERFACE)
> PipeHandle: c146f640
> 
> SetupPacket:
> 0000: 21 00 00 00 00 00 00 00 
> bmRequestType: 21
>   DIR: Host-To-Device
>   TYPE: Class
>   RECIPIENT: Interface
> bRequest: 00  
> 
> 
> TransferBuffer: 0x0000000c (12) length
> 0000: 12 00 00 00 24 00 00 00 00 00 00 00 

Standard INQUIRY, 0x24 (36) bytes.

> 4	out up	n/a	3.146	CONTROL_TRANSFER	-	0x00000000
> URB Header (length: 80)
> SequenceNumber: 4
> Function: 0008 (CONTROL_TRANSFER)
> PipeHandle: c1565d8c
> 
> SetupPacket:
> 0000: 21 00 00 00 00 00 0c 00 
> bmRequestType: 21
>   DIR: Host-To-Device
>   TYPE: Class
>   RECIPIENT: Interface
> bRequest: 00  
> 
> 
> No TransferBuffer
> 
> 5	in down	0x82	3.146	BULK_OR_INTERRUPT_TRANSFER	-	
> URB Header (length: 72)
> SequenceNumber: 5
> Function: 0009 (BULK_OR_INTERRUPT_TRANSFER)
> TransferFlags: 0x00000003
> 
> No TransferBuffer
> 
> 5	in up	0x82	3.146	BULK_OR_INTERRUPT_TRANSFER	00 80 00 01 1f 
> 00 00 00	0x00000000
> URB Header (length: 72)
> SequenceNumber: 5
> Function: 0009 (BULK_OR_INTERRUPT_TRANSFER)
> TransferFlags: 0x00000003
> 
> TransferBuffer: 0x00000024 (36) length
> 0000: 00 80 00 01 1f 00 00 00 53 6f 6e 79 20 20 20 20 
> 0010: 53 6f 6e 79 20 44 53 43 20 20 20 20 20 20 20 20 
> 0020: 33 2e 30 30 

INQUIRY reponse

> 6	in down	0x82	3.146	CLASS_INTERFACE	25 00 00 00 00 00 00 00	
> URB Header (length: 80)
> SequenceNumber: 6
> Function: 001b (CLASS_INTERFACE)
> PipeHandle: c156772c
> 
> SetupPacket:
> 0000: 21 00 00 00 00 00 0c 00 
> bmRequestType: 21
>   DIR: Host-To-Device
>   TYPE: Class
>   RECIPIENT: Interface
> bRequest: 00  
> 
> 
> TransferBuffer: 0x0000000c (12) length
> 0000: 25 00 00 00 00 00 00 00 00 00 00 00 

READ CAPACITY

> 6	out up	n/a	3.152	CONTROL_TRANSFER	-	0x00000000
> URB Header (length: 80)
> SequenceNumber: 6
> Function: 0008 (CONTROL_TRANSFER)
> PipeHandle: c1565d8c
> 
> SetupPacket:
> 0000: 21 00 00 00 00 00 0c 00 
> bmRequestType: 21
>   DIR: Host-To-Device
>   TYPE: Class
>   RECIPIENT: Interface
> bRequest: 00  
> 
> 
> No TransferBuffer
> 
> 7	in down	0x82	3.152	BULK_OR_INTERRUPT_TRANSFER	-	
> URB Header (length: 72)
> SequenceNumber: 7
> Function: 0009 (BULK_OR_INTERRUPT_TRANSFER)
> TransferFlags: 0x00000003
> 
> No TransferBuffer
> 
> 7	in up	0x82	3.156	BULK_OR_INTERRUPT_TRANSFER	00 00 1e df 00 
> 00 02 00	0x00000000
> URB Header (length: 72)
> SequenceNumber: 7
> Function: 0009 (BULK_OR_INTERRUPT_TRANSFER)
> TransferFlags: 0x00000003
> 
> TransferBuffer: 0x00000008 (8) length
> 0000: 00 00 1e df 00 00 02 00 

0x00001edf, 7903 blocks decimal

0x00000200, 512 byte blocks

> 8	in down	0x82	3.166	CLASS_INTERFACE	00 00 00 00 00 00 00 00	
> URB Header (length: 80)
> SequenceNumber: 8
> Function: 001b (CLASS_INTERFACE)
> PipeHandle: c156772c
> 
> SetupPacket:
> 0000: 21 00 00 00 00 00 0c 00 
> bmRequestType: 21
>   DIR: Host-To-Device
>   TYPE: Class
>   RECIPIENT: Interface
> bRequest: 00  
> 
> 
> TransferBuffer: 0x0000000c (12) length
> 0000: 00 00 00 00 00 00 00 00 00 00 00 00 

TEST UNIT READY 

> 8	out up	n/a	3.171	CONTROL_TRANSFER	-	0x00000000
> URB Header (length: 80)
> SequenceNumber: 8
> Function: 0008 (CONTROL_TRANSFER)
> PipeHandle: c1565d8c
> 
> SetupPacket:
> 0000: 21 00 00 00 00 00 0c 00 
> bmRequestType: 21
>   DIR: Host-To-Device
>   TYPE: Class
>   RECIPIENT: Interface
> bRequest: 00  
> 
> 
> No TransferBuffer
> 
> 9	out down	n/a	3.172	CLASS_INTERFACE	25 00 00 00 00 00 00 00	
> URB Header (length: 80)
> SequenceNumber: 9
> Function: 001b (CLASS_INTERFACE)
> PipeHandle: c1565d8c
> 
> SetupPacket:
> 0000: 21 00 00 00 00 00 0c 00 
> bmRequestType: 21
>   DIR: Host-To-Device
>   TYPE: Class
>   RECIPIENT: Interface
> bRequest: 00  

Nothing back for the TEST UNIT READY?

> TransferBuffer: 0x0000000c (12) length
> 0000: 25 00 00 00 00 00 00 00 00 00 00 00 

READ CAPACITY again

> 9	out up	n/a	3.176	CONTROL_TRANSFER	-	0x00000000
> URB Header (length: 80)
> SequenceNumber: 9
> Function: 0008 (CONTROL_TRANSFER)
> PipeHandle: c1565d8c
> 
> SetupPacket:
> 0000: 21 00 00 00 00 00 0c 00 
> bmRequestType: 21
>   DIR: Host-To-Device
>   TYPE: Class
>   RECIPIENT: Interface
> bRequest: 00  
> 
> 
> No TransferBuffer
> 
> 10	in down	0x82	3.181	BULK_OR_INTERRUPT_TRANSFER	-	
> URB Header (length: 72)
> SequenceNumber: 10
> Function: 0009 (BULK_OR_INTERRUPT_TRANSFER)
> TransferFlags: 0x00000003
> 
> No TransferBuffer
> 
> 10	in up	0x82	3.182	BULK_OR_INTERRUPT_TRANSFER	00 00 1e df 00 
> 00 02 00	0x00000000
> URB Header (length: 72)
> SequenceNumber: 10
> Function: 0009 (BULK_OR_INTERRUPT_TRANSFER)
> TransferFlags: 0x00000003
> 
> TransferBuffer: 0x00000008 (8) length
> 0000: 00 00 1e df 00 00 02 00 

READ CAPACITY result again

> 11	in down	0x82	3.191	CLASS_INTERFACE	28 00 00 00 00 00 00 00	
> URB Header (length: 80)
> SequenceNumber: 11
> Function: 001b (CLASS_INTERFACE)
> PipeHandle: c156772c
> 
> SetupPacket:
> 0000: 21 00 00 00 00 00 0c 00 
> bmRequestType: 21
>   DIR: Host-To-Device
>   TYPE: Class
>   RECIPIENT: Interface
> bRequest: 00  
> 
> 
> TransferBuffer: 0x0000000c (12) length
> 0000: 28 00 00 00 00 00 00 00 80 00 00 00 

READ 10

> 11	out up	n/a	3.196	CONTROL_TRANSFER	-	0x00000000
> URB Header (length: 80)
> SequenceNumber: 11
> Function: 0008 (CONTROL_TRANSFER)
> PipeHandle: c1565d8c
> 
> SetupPacket:
> 0000: 21 00 00 00 00 00 0c 00 
> bmRequestType: 21
>   DIR: Host-To-Device
>   TYPE: Class
>   RECIPIENT: Interface
> bRequest: 00  
> 
> 
> No TransferBuffer
> 
> 12	in down	0x82	3.201	BULK_OR_INTERRUPT_TRANSFER	-	
> URB Header (length: 72)
> SequenceNumber: 12
> Function: 0009 (BULK_OR_INTERRUPT_TRANSFER)
> TransferFlags: 0x00000003
> 
> No TransferBuffer

-- Patrick Mansfield


-------------------------------------------------------
This SF.Net email sponsored by: ApacheCon 2003,
16-19 November in Las Vegas. Learn firsthand the latest
developments in Apache, PHP, Perl, XML, Java, MySQL,
WebDAV, and more! http://www.apachecon.com/
_______________________________________________
linux-usb-devel@lists.sourceforge.net
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel

Re: usb-storage and Sony Handycam

[Patrick Mansfield]

On Sat, Nov 08, 2003 at 10:47:26PM -0500, Alan Stern wrote:
> On Sat, 8 Nov 2003, Patrick Mansfield wrote:
> 
> > On Sat, Nov 08, 2003 at 11:28:37AM -0500, Alan Stern wrote:

> > A DID_ERROR causes scsi core to retry the command - it will not even check
> > the sense data. A DID_ABORT might be better, scsi core will not retry.
> > (We still have issue 1, where the host byte is not checked in
> > scsi_status_is_good.)
> 
> For CB and CBI devices, babble causes a DID_ERROR return.  For Bulk-only 
> it causes a Check-Condition status to be returned along with Invalid 
> Command sense data.  Like I said earlier, we haven't yet figured out the 
> best way to handle these errors.

The DID_ERROR should be changed to DID_ABORT for the babble case so scsi
core won't retry the command generating the babble.  

I don't know why scsi core retries on DID_ERROR, as code comments say
DID_ERROR is an internal error. DID_SOFT_ERROR is the same as DID_ERROR
(as far as scsi core cares), execept for a check for reservation conflict.

> > Dmitri's change (return USB_STOR_XFER_STALL instead of USB_STOR_XFER_LONG)
> > causes us to return USB_STOR_TRANSPORT_GOOD vs USB_STOR_TRANSPORT_ERROR in
> > usb_stor_CB_transport. 
> > 
> > Then in usb_stor_invoke_transport we issue a REQUEST SENSE. 
> > 
> > If for a USB_STOR_TRANSPORT_ERROR we also did a REQUEST SENSE, the
> > following command (TEST UNIT READY) would not get the ILLEGAL REQUEST.
> 
> That doesn't make sense.  USB_STOR_TRANSPORT_ERROR means we aren't able to 
> communicate with the device.  So how can we send it a REQUEST-SENSE?

No that should not be done, I was trying to figure out and explain why he
did not get the ILLEGAL REQUEST for TEST UNIT READY.

Has anyone looked at why the reset failed? I assume that would clear the
sense.

> > I don't know why the TEST UNIT READY caused an auto REQUEST SENSE to
> > be sent.
> 
> The CB transport doesn't include any means for the device to return
> status.  (Yes, it's stupid.)  The only way we can find out is to issue
> auto REQUEST-SENSE for virtually every command.
> 
> > Changing the sense stuff does not address the MODE SENSE failing, we still
> > have 3 separate issues that are all hit by this device.
> 
> If we simply avoided sending the MODE-SENSE in the first place, none of 
> the other issues would come up.  IMHO that's the best solution.

Yes.

We still have the missed check in scsi_status_is_good.

And, the sense coming back for a previous command returning babble is still
an issue.

-- Patrick Mansfield


-------------------------------------------------------
This SF.Net email sponsored by: ApacheCon 2003,
16-19 November in Las Vegas. Learn firsthand the latest
developments in Apache, PHP, Perl, XML, Java, MySQL,
WebDAV, and more! http://www.apachecon.com/
_______________________________________________
linux-usb-devel@lists.sourceforge.net
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel

Re: usb-storage and Sony Handycam

[Alan Stern]

On Mon, 10 Nov 2003, Patrick Mansfield wrote:

> On Sat, Nov 08, 2003 at 10:47:26PM -0500, Alan Stern wrote:
> > 
> > For CB and CBI devices, babble causes a DID_ERROR return.  For Bulk-only 
> > it causes a Check-Condition status to be returned along with Invalid 
> > Command sense data.  Like I said earlier, we haven't yet figured out the 
> > best way to handle these errors.
> 
> The DID_ERROR should be changed to DID_ABORT for the babble case so scsi
> core won't retry the command generating the babble.  
> 
> I don't know why scsi core retries on DID_ERROR, as code comments say
> DID_ERROR is an internal error. DID_SOFT_ERROR is the same as DID_ERROR
> (as far as scsi core cares), execept for a check for reservation conflict.

I'll pass that on to Matt Dharm.  It's not always clear to me which
DID_XXX code is best to use in a given situation for an emulated SCSI bus
like USB.

> Has anyone looked at why the reset failed? I assume that would clear the
> sense.

How could anyone attempt to find out why the reset failed?  The only 
externally visible state is the device's response to incoming USB 
requests.  IIRC, it simply didn't respond to the reset request.  (I'm not 
sure about that; I may be confusing this with other devices that failed in 
similar ways.)

> We still have the missed check in scsi_status_is_good.
> 
> And, the sense coming back for a previous command returning babble is still
> an issue.

Agreed.  I tend to ascribe things like that to buggy SCSI implementations 
in the device firmware.  These things are produced quickly and often 
without thorough testing.  On the other hand, even if it's true it doesn't 
suggest any good avenues of approach.

Alan Stern



-------------------------------------------------------
This SF.Net email sponsored by: ApacheCon 2003,
16-19 November in Las Vegas. Learn firsthand the latest
developments in Apache, PHP, Perl, XML, Java, MySQL,
WebDAV, and more! http://www.apachecon.com/
_______________________________________________
linux-usb-devel@lists.sourceforge.net
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel

Re: [linux-usb-devel] usb-storage and Sony Handycam

[Patrick Mansfield]

On Mon, Nov 10, 2003 at 01:46:44PM -0500, Alan Stern wrote:
> On Mon, 10 Nov 2003, Patrick Mansfield wrote:

> How could anyone attempt to find out why the reset failed?  The only 
> externally visible state is the device's response to incoming USB 
> requests.  IIRC, it simply didn't respond to the reset request.  (I'm not 
> sure about that; I may be confusing this with other devices that failed in 
> similar ways.)

I don't know, the bug report (and other logs) show:

Nov  7 00:12:19 [kernel] usb-storage: Command MODE_SENSE_10 (10 bytes)
Nov  7 00:12:19 [kernel] usb-storage: usb_stor_ctrl_transfer: rq=00 rqtype=21 value=0000 index=00 len=10
Nov  7 00:12:19 [kernel] usb-storage: Status code 0; transferred 10/10
Nov  7 00:12:19 [kernel] usb-storage: -- transfer complete
Nov  7 00:12:19 [kernel] usb-storage: Call to usb_stor_ctrl_transfer() returned 0
Nov  7 00:12:19 [kernel] usb-storage: usb_stor_bulk_transfer_buf: xfer 8 bytes
Nov  7 00:12:19 [kernel] usb-storage: Status code -75; transferred 8/8
Nov  7 00:12:19 [kernel] usb-storage: -- babble
Nov  7 00:12:19 [kernel] usb-storage: CB data stage result is 0x3
Nov  7 00:12:19 [kernel] usb-storage: -- transport indicates error, resetting
Nov  7 00:12:19 [kernel] usb-storage: usb_stor_CB_reset called
Nov  7 00:12:19 [kernel] usb-storage: usb_stor_control_msg: rq=00 rqtype=21 value=0000 index=00 len=12
Nov  7 00:12:19 [kernel] usb-storage: Soft reset failed: -32
Nov  7 00:12:19 [kernel] usb-storage: scsi cmd done, result=0x70000

-- Patrick Mansfield

Re: usb-storage and Sony Handycam

[Alan Stern]

On Mon, 10 Nov 2003, Patrick Mansfield wrote:

> On Mon, Nov 10, 2003 at 01:46:44PM -0500, Alan Stern wrote:
> > On Mon, 10 Nov 2003, Patrick Mansfield wrote:
> 
> > How could anyone attempt to find out why the reset failed?  The only 
> > externally visible state is the device's response to incoming USB 
> > requests.  IIRC, it simply didn't respond to the reset request.  (I'm not 
> > sure about that; I may be confusing this with other devices that failed in 
> > similar ways.)
> 
> I don't know, the bug report (and other logs) show:
> 
> Nov  7 00:12:19 [kernel] usb-storage: Command MODE_SENSE_10 (10 bytes)
> Nov  7 00:12:19 [kernel] usb-storage: usb_stor_ctrl_transfer: rq=00 rqtype=21 value=0000 index=00 len=10
> Nov  7 00:12:19 [kernel] usb-storage: Status code 0; transferred 10/10
> Nov  7 00:12:19 [kernel] usb-storage: -- transfer complete
> Nov  7 00:12:19 [kernel] usb-storage: Call to usb_stor_ctrl_transfer() returned 0
> Nov  7 00:12:19 [kernel] usb-storage: usb_stor_bulk_transfer_buf: xfer 8 bytes
> Nov  7 00:12:19 [kernel] usb-storage: Status code -75; transferred 8/8
> Nov  7 00:12:19 [kernel] usb-storage: -- babble
> Nov  7 00:12:19 [kernel] usb-storage: CB data stage result is 0x3
> Nov  7 00:12:19 [kernel] usb-storage: -- transport indicates error, resetting
> Nov  7 00:12:19 [kernel] usb-storage: usb_stor_CB_reset called
> Nov  7 00:12:19 [kernel] usb-storage: usb_stor_control_msg: rq=00 rqtype=21 value=0000 index=00 len=12
> Nov  7 00:12:19 [kernel] usb-storage: Soft reset failed: -32
> Nov  7 00:12:19 [kernel] usb-storage: scsi cmd done, result=0x70000

The -32 (-EPIPE) error code indicates a "protocol stall" -- the device is
saying that for some reason it can't accept or can't carry out the reset
command.  Maybe because it's busy still trying to finish up its broken
response to the MODE-SENSE command.

You would think that a reset would take precedence over a regular 
command-in-progress, but apparently not here.

Alan Stern




-------------------------------------------------------
This SF.Net email sponsored by: ApacheCon 2003,
16-19 November in Las Vegas. Learn firsthand the latest
developments in Apache, PHP, Perl, XML, Java, MySQL,
WebDAV, and more! http://www.apachecon.com/
_______________________________________________
linux-usb-devel@lists.sourceforge.net
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel

Re: usb-storage and Sony Handycam

[Sancho Dauskardt]

> > Has anyone looked at why the reset failed? I assume that would clear the
> > sense.
>
>How could anyone attempt to find out why the reset failed?

Just some ideas:
a) not supported - AFAIK many storage devices don't seem to support the 
bulk-reset, as the 'Other Popular OS' doesn't seem to issue them.
b) possibly VIA UHCI chipset which got killed by babble ?

- sda



-------------------------------------------------------
This SF.Net email sponsored by: ApacheCon 2003,
16-19 November in Las Vegas. Learn firsthand the latest
developments in Apache, PHP, Perl, XML, Java, MySQL,
WebDAV, and more! http://www.apachecon.com/
_______________________________________________
linux-usb-devel@lists.sourceforge.net
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel

Unaligned scatter-gather buffers and usb-storage

[Alan Stern]

Although most scatter-gather buffers are page aligned, occasionally one of 
them isn't.  This can happen when the sg and st drivers use direct I/O and 
the user's buffer doesn't start on a page boundary; the first s-g buffer 
in the list will coincide with the start of the user buffer even though 
all the later ones will start at page boundaries.

The usb-storage driver has trouble with unaligned buffers.  More 
accurately, it has trouble with s-g entries having bad lengths.  A buffer 
that occupies an entire page or is the _last_ entry in the s-g list is 
okay, but if the _first_ entry isn't aligned properly it is likely to have 
a bad length.

[For linux-scsi readers: This has to do with the way USB works.  Data
transfers are divided up into packets, and packets have a maximum size
(typically 64 or 512 bytes).  A packet can't span two s-g entries; hence
if an s-g entry's length isn't divisible by the maxpacket size then one of
the packets for that entry must be short, i.e., smaller than the maximum
size.  But USB specifies that short packets always indicate the end of a
transfer.  Thus if any s-g entry but the last has a bad length, the data
transfer will terminate prematurely.]

Alignment to a 512-byte boundary would suffice and page alignment would be
more than enough.  The cleanest way to fix the problem would be to have st
and sg refuse to do direct I/O when the user's buffer isn't page aligned.  
That's probably too draconian (but it would allow us to give the user
process an appropriate error code right away).

So instead I'm going to change the usb-storage driver to make it reject
any s-g transfer that doesn't meet the entry length requirements.  My
question is this: What's the best way to inform the SCSI midlayer about
what went wrong?  Return a DID_ABORT code?  Or would something else be
better?

Alan Stern

Re: Unaligned scatter-gather buffers and usb-storage

[Patrick Mansfield]

On Tue, Nov 18, 2003 at 10:20:17AM -0500, Alan Stern wrote:

> Alignment to a 512-byte boundary would suffice and page alignment would be
> more than enough.  The cleanest way to fix the problem would be to have st
> and sg refuse to do direct I/O when the user's buffer isn't page aligned.  
> That's probably too draconian (but it would allow us to give the user
> process an appropriate error code right away).

The above restriction sounds reasonable to me, but I do not have
experience in this area. I thought that the raw and direct block
interfaces already had such restrictions.

Jens or Doug G can you comment on this?

You could add a scsi_host fields or bits specifying alignment
restrictions, and enforce them as needed in upper level drivers.

> So instead I'm going to change the usb-storage driver to make it reject
> any s-g transfer that doesn't meet the entry length requirements.  My
> question is this: What's the best way to inform the SCSI midlayer about
> what went wrong?  Return a DID_ABORT code?  Or would something else be
> better?
> 
> Alan Stern

Assuming a printk is OK, DID_ABORT is OK.

If there is an existing appropriate sense code, you could generate that
and pass it back. I searched the SPC3 spec and did not find any that have
"align" but there are some with "invalid" that might be OK, though you
might not be able to tell for certain if the sense was from the device or
not, it is less vague than a DID_ABORT.

-- Patrick Mansfield

Re: Unaligned scatter-gather buffers and usb-storage

[Jens Axboe]

On Tue, Nov 18 2003, Patrick Mansfield wrote:
> On Tue, Nov 18, 2003 at 10:20:17AM -0500, Alan Stern wrote:
> 
> > Alignment to a 512-byte boundary would suffice and page alignment would be
> > more than enough.  The cleanest way to fix the problem would be to have st
> > and sg refuse to do direct I/O when the user's buffer isn't page aligned.  
> > That's probably too draconian (but it would allow us to give the user
> > process an appropriate error code right away).
> 
> The above restriction sounds reasonable to me, but I do not have
> experience in this area. I thought that the raw and direct block
> interfaces already had such restrictions.
> 
> Jens or Doug G can you comment on this?
> 
> You could add a scsi_host fields or bits specifying alignment
> restrictions, and enforce them as needed in upper level drivers.

The queue already has such a restriction embedded, see bio_map_user()
and queue_dma_alignment().

-- 
Jens Axboe

Re: [linux-usb-devel] Re: Unaligned scatter-gather buffers and usb-storage

[Oliver Neukum]

> > You could add a scsi_host fields or bits specifying alignment
> > restrictions, and enforce them as needed in upper level drivers.
> 
> The queue already has such a restriction embedded, see bio_map_user()
> and queue_dma_alignment().

Very well, as far as sd and sr are concerned, but it doesn't help for
sg and st, does it?

	Regards
		Oliver

Re: [linux-usb-devel] Re: Unaligned scatter-gather buffers and usb-storage

[Jens Axboe]

On Wed, Nov 19 2003, Oliver Neukum wrote:
> 
> > > You could add a scsi_host fields or bits specifying alignment
> > > restrictions, and enforce them as needed in upper level drivers.
> > 
> > The queue already has such a restriction embedded, see bio_map_user()
> > and queue_dma_alignment().
> 
> Very well, as far as sd and sr are concerned, but it doesn't help for
> sg and st, does it?

They both have a queue through the scsi device structure. But you could
put the value someplace else if you wanted.

-- 
Jens Axboe

Re: Re: Unaligned scatter-gather buffers and usb-storage

[James Bottomley]

On Wed, 2003-11-19 at 07:04, Jens Axboe wrote:
> On Wed, Nov 19 2003, Oliver Neukum wrote:
> > 
> > > > You could add a scsi_host fields or bits specifying alignment
> > > > restrictions, and enforce them as needed in upper level drivers.
> > > 
> > > The queue already has such a restriction embedded, see bio_map_user()
> > > and queue_dma_alignment().
> > 
> > Very well, as far as sd and sr are concerned, but it doesn't help for
> > sg and st, does it?
> 
> They both have a queue through the scsi device structure. But you could
> put the value someplace else if you wanted.

Actually, the sg case should be fine, because it shares the queue with
the other attachment (unless it's a processor or other unrecognised
device).

but, regardless, the queue is already set up (for all devices, including
st) in either slave_alloc, or slave_configure, so you could call the
alignment setting routine there.

Alternatively, we could add this as yet another parameter to the host
and template structures.  Any preferences?

James




-------------------------------------------------------
This SF.net email is sponsored by: SF.net Giveback Program.
Does SourceForge.net help you be more productive?  Does it
help you create better code?  SHARE THE LOVE, and help us help
YOU!  Click Here: http://sourceforge.net/donate/
_______________________________________________
linux-usb-devel@lists.sourceforge.net
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel

Re: Re: Unaligned scatter-gather buffers and usb-storage

[Jens Axboe]

On Wed, Nov 19 2003, James Bottomley wrote:
> On Wed, 2003-11-19 at 07:04, Jens Axboe wrote:
> > On Wed, Nov 19 2003, Oliver Neukum wrote:
> > > 
> > > > > You could add a scsi_host fields or bits specifying alignment
> > > > > restrictions, and enforce them as needed in upper level drivers.
> > > > 
> > > > The queue already has such a restriction embedded, see bio_map_user()
> > > > and queue_dma_alignment().
> > > 
> > > Very well, as far as sd and sr are concerned, but it doesn't help for
> > > sg and st, does it?
> > 
> > They both have a queue through the scsi device structure. But you could
> > put the value someplace else if you wanted.
> 
> Actually, the sg case should be fine, because it shares the queue with
> the other attachment (unless it's a processor or other unrecognised
> device).
> 
> but, regardless, the queue is already set up (for all devices, including
> st) in either slave_alloc, or slave_configure, so you could call the
> alignment setting routine there.

That was my point, yes.

> Alternatively, we could add this as yet another parameter to the host
> and template structures.  Any preferences?

I suppose you have to, if you want LLD to be able to pass it down.

-- 
Jens Axboe



-------------------------------------------------------
This SF.net email is sponsored by: SF.net Giveback Program.
Does SourceForge.net help you be more productive?  Does it
help you create better code?  SHARE THE LOVE, and help us help
YOU!  Click Here: http://sourceforge.net/donate/
_______________________________________________
linux-usb-devel@lists.sourceforge.net
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel

Re: Re: Unaligned scatter-gather buffers and usb-storage

[James Bottomley]

On Wed, 2003-11-19 at 08:39, Jens Axboe wrote:
> > but, regardless, the queue is already set up (for all devices, including
> > st) in either slave_alloc, or slave_configure, so you could call the
> > alignment setting routine there.
> 
> That was my point, yes.

I know, just driving it home.  I'll hand back the reins now...

> 
> > Alternatively, we could add this as yet another parameter to the host
> > and template structures.  Any preferences?
> 
> I suppose you have to, if you want LLD to be able to pass it down.

Well, what I was fishing for: Is this property useful enough to want
every device driver writer to think about it (i.e. document and place in
the template),  or is it rare enough that it should only be accessible
to those who know what they're doing (via the direct block queue
interfaces in slave_alloc/configure)?

James




-------------------------------------------------------
This SF.net email is sponsored by: SF.net Giveback Program.
Does SourceForge.net help you be more productive?  Does it
help you create better code?  SHARE THE LOVE, and help us help
YOU!  Click Here: http://sourceforge.net/donate/
_______________________________________________
linux-usb-devel@lists.sourceforge.net
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel

Re: [linux-usb-devel] Re: Unaligned scatter-gather buffers and usb-storage

[Jens Axboe]

On Wed, Nov 19 2003, James Bottomley wrote:
> > > Alternatively, we could add this as yet another parameter to the host
> > > and template structures.  Any preferences?
> > 
> > I suppose you have to, if you want LLD to be able to pass it down.
> 
> Well, what I was fishing for: Is this property useful enough to want
> every device driver writer to think about it (i.e. document and place in
> the template),  or is it rare enough that it should only be accessible
> to those who know what they're doing (via the direct block queue
> interfaces in slave_alloc/configure)?

As far as I'm concerned, requiring 512-byte alignment is 'just
easier'... Only a select few host adapters will set it differently
anyways, so you gain basically nothing.

-- 
Jens Axboe

Re: [linux-usb-devel] Re: Unaligned scatter-gather buffers and usb-storage

[Kai Makisara]

On Wed, 19 Nov 2003, Jens Axboe wrote:

> On Wed, Nov 19 2003, James Bottomley wrote:
> > > > Alternatively, we could add this as yet another parameter to the host
> > > > and template structures.  Any preferences?
> > >
> > > I suppose you have to, if you want LLD to be able to pass it down.
> >
> > Well, what I was fishing for: Is this property useful enough to want
> > every device driver writer to think about it (i.e. document and place in
> > the template),  or is it rare enough that it should only be accessible
> > to those who know what they're doing (via the direct block queue
> > interfaces in slave_alloc/configure)?
>
> As far as I'm concerned, requiring 512-byte alignment is 'just
> easier'... Only a select few host adapters will set it differently
> anyways, so you gain basically nothing.
>
As usual, I am against this 512-byte alignment (thinking as st
maintainer). Don't default to more strict aligment requirements than is
necessary. It creates hidden performance problems for character device
drivers.

-- 
Kai

Re: [linux-usb-devel] Re: Unaligned scatter-gather buffers and usb-storage

[Jens Axboe]

On Wed, Nov 19 2003, Kai Makisara wrote:
> On Wed, 19 Nov 2003, Jens Axboe wrote:
> 
> > On Wed, Nov 19 2003, James Bottomley wrote:
> > > > > Alternatively, we could add this as yet another parameter to the host
> > > > > and template structures.  Any preferences?
> > > >
> > > > I suppose you have to, if you want LLD to be able to pass it down.
> > >
> > > Well, what I was fishing for: Is this property useful enough to want
> > > every device driver writer to think about it (i.e. document and place in
> > > the template),  or is it rare enough that it should only be accessible
> > > to those who know what they're doing (via the direct block queue
> > > interfaces in slave_alloc/configure)?
> >
> > As far as I'm concerned, requiring 512-byte alignment is 'just
> > easier'... Only a select few host adapters will set it differently
> > anyways, so you gain basically nothing.
> >
> As usual, I am against this 512-byte alignment (thinking as st
> maintainer). Don't default to more strict aligment requirements than is
> necessary. It creates hidden performance problems for character device
> drivers.

But you have to honor hardware restrictions, though. Where is the
performance impact?

-- 
Jens Axboe

Re: [linux-usb-devel] Re: Unaligned scatter-gather buffers and usb-storage

[Kai Makisara]

On Wed, 19 Nov 2003, Jens Axboe wrote:

> On Wed, Nov 19 2003, Kai Makisara wrote:
...
> > As usual, I am against this 512-byte alignment (thinking as st
> > maintainer). Don't default to more strict aligment requirements than is
> > necessary. It creates hidden performance problems for character device
> > drivers.
>
> But you have to honor hardware restrictions, though. Where is the
> performance impact?
>
Yes, I agree that we must not try to do things the hardware does not
allow. On i386, most SCSI adapter accept any alignment. It is a waste to
require 512-byte alignment with these adapters just because USB requires
that. I think there should be a way for the high-level drivers to know
about the absolutely necessary alignment requirements for the actual
adapter being used with a device.

If the user buffer is not aligned according to the required rules, the
driver must copy the data into a temporary buffer for i/o. This consumes
bandwidth and with the current faster tape drives this leads to
significant waste of cpu cycles (according to measurements).

The software using tapes tends to malloc() the buffers and then the
alignment is not guaranteed to 512 byte boundaries. Other Unices don't
have alignment requirements for tape buffers and I think we should avoid
requiring this in Linux if possible.

Someone may think that this is theory. I checked one common low-end backup
program tar (from SuSE 9.0). In the first test (tar cb 64 xxx) the user
buffer address was 0806e508 except in one write it was 08051000. A test
with dd gave the address 0806e508. cpio used the addresses 0806e508 and
08057cd8.

-- 
Kai

Re: [linux-usb-devel] Re: Unaligned scatter-gather buffers and usb-storage

[Jens Axboe]

On Thu, Nov 20 2003, Kai Makisara wrote:
> On Wed, 19 Nov 2003, Jens Axboe wrote:
> 
> > On Wed, Nov 19 2003, Kai Makisara wrote:
> ..
> > > As usual, I am against this 512-byte alignment (thinking as st
> > > maintainer). Don't default to more strict aligment requirements than is
> > > necessary. It creates hidden performance problems for character device
> > > drivers.
> >
> > But you have to honor hardware restrictions, though. Where is the
> > performance impact?
> >
> Yes, I agree that we must not try to do things the hardware does not
> allow. On i386, most SCSI adapter accept any alignment. It is a waste to
> require 512-byte alignment with these adapters just because USB requires
> that. I think there should be a way for the high-level drivers to know
> about the absolutely necessary alignment requirements for the actual
> adapter being used with a device.
> 
> If the user buffer is not aligned according to the required rules, the
> driver must copy the data into a temporary buffer for i/o. This consumes
> bandwidth and with the current faster tape drives this leads to
> significant waste of cpu cycles (according to measurements).
> 
> The software using tapes tends to malloc() the buffers and then the
> alignment is not guaranteed to 512 byte boundaries. Other Unices don't
> have alignment requirements for tape buffers and I think we should avoid
> requiring this in Linux if possible.

Then you just cannot do zero-copy dma, if the buffer isn't aligned
properly. It's as simple as that. And surely that wont kill you
performance wise.

Doing bounce buffering to just maintain the use of mapping user pages
into the kernel ls out right silly.

So no, I still don't think we have to add anything. Just don't do_dio if
the alignment doesn't allow it. Ditto for sg.

-- 
Jens Axboe

Re: [linux-usb-devel] Re: Unaligned scatter-gather buffers and usb-storage

[Alan Stern]

On Thu, 20 Nov 2003, Jens Axboe wrote:

> Then you just cannot do zero-copy dma, if the buffer isn't aligned
> properly. It's as simple as that. And surely that wont kill you
> performance wise.
> 
> Doing bounce buffering to just maintain the use of mapping user pages
> into the kernel ls out right silly.
> 
> So no, I still don't think we have to add anything. Just don't do_dio if
> the alignment doesn't allow it. Ditto for sg.

The answer seems very simple.  There should be a host template entry for
dma buffer alignment (there's already a dma_boundary member).  It would be
copied into the device's request queue stucture if it is nonzero,
overriding the default value of 512.  sg and st should check the user
buffer against the request queue's dma_alignment mask and avoid doing
direct I/O if the alignment is wrong -- just fall back to normal I/O.

Any objections to this scheme?

Alan Stern

Re: [linux-usb-devel] Re: Unaligned scatter-gather buffers and usb-storage

[Jens Axboe]

On Thu, Nov 20 2003, Alan Stern wrote:
> On Thu, 20 Nov 2003, Jens Axboe wrote:
> 
> > Then you just cannot do zero-copy dma, if the buffer isn't aligned
> > properly. It's as simple as that. And surely that wont kill you
> > performance wise.
> > 
> > Doing bounce buffering to just maintain the use of mapping user pages
> > into the kernel ls out right silly.
> > 
> > So no, I still don't think we have to add anything. Just don't do_dio if
> > the alignment doesn't allow it. Ditto for sg.
> 
> The answer seems very simple.  There should be a host template entry for
> dma buffer alignment (there's already a dma_boundary member).  It would be
> copied into the device's request queue stucture if it is nonzero,
> overriding the default value of 512.  sg and st should check the user
> buffer against the request queue's dma_alignment mask and avoid doing
> direct I/O if the alignment is wrong -- just fall back to normal I/O.
> 
> Any objections to this scheme?

Well yes, that's what my objection is against - adding that member. Did
you not read any of my mails? And it's quite simple why - basically
noone will add it, so it'll end up being 512 anyways.

I just don't see the point. It's a miniscule optimization. If you need
that last bit of performance, then align your buffers and noone loses.
See?

-- 
Jens Axboe

Re: [linux-usb-devel] Re: Unaligned scatter-gather buffers and usb-storage

[Alan Stern]

On Thu, 20 Nov 2003, Jens Axboe wrote:

> Well yes, that's what my objection is against - adding that member. Did
> you not read any of my mails? And it's quite simple why - basically
> noone will add it, so it'll end up being 512 anyways.

All right, fine.  Host adapter drivers have the option of setting the 
dma_alignment mask in their slave_configure() routines.  Doing it there 
won't add anything to the host template, will leave the value at its 
default 512 for host drivers that don't care, and will let drivers that do 
care set it to the optimal value.

The important thing is that sg and st should check the alignment of the 
actual buffer against the dma_alignment mask, which they currently don't 
do.

> I just don't see the point. It's a miniscule optimization. If you need
> that last bit of performance, then align your buffers and noone loses.
> See?

I'm not sure -- was that "you" directed at me personally or at userspace 
program writers in general?  Telling me to align the user buffers won't 
help anything; it's the program writers who need to be informed of the 
restrictions.  And it's the host-driver writers who need to loosen those 
restrictions where applicable.

Alan Stern

Re: Re: Unaligned scatter-gather buffers and usb-storage

[Jens Axboe]

On Thu, Nov 20 2003, Alan Stern wrote:
> On Thu, 20 Nov 2003, Jens Axboe wrote:
> 
> > Well yes, that's what my objection is against - adding that member. Did
> > you not read any of my mails? And it's quite simple why - basically
> > noone will add it, so it'll end up being 512 anyways.
> 
> All right, fine.  Host adapter drivers have the option of setting the 
> dma_alignment mask in their slave_configure() routines.  Doing it there 
> won't add anything to the host template, will leave the value at its 
> default 512 for host drivers that don't care, and will let drivers that do 
> care set it to the optimal value.
> 
> The important thing is that sg and st should check the alignment of the 
> actual buffer against the dma_alignment mask, which they currently don't 
> do.

Sure fine. I still don't think it's worth anybodies time messing with.

> > I just don't see the point. It's a miniscule optimization. If you need
> > that last bit of performance, then align your buffers and noone loses.
> > See?
> 
> I'm not sure -- was that "you" directed at me personally or at userspace 
> program writers in general?  Telling me to align the user buffers won't 
> help anything; it's the program writers who need to be informed of the 
> restrictions.  And it's the host-driver writers who need to loosen those 
> restrictions where applicable.

You as in program writers, clearly you cannot do anything about it. Of
course sg/st/whatever cannot pass down badly aligned sg addresses,
that's a given. And I think they are currently broken that they don't
attempt to align to 512 bytes. I just think we've waisted way more time
discussion the issue than what will ever be saved in cpu time.

-- 
Jens Axboe



-------------------------------------------------------
This SF.net email is sponsored by: SF.net Giveback Program.
Does SourceForge.net help you be more productive?  Does it
help you create better code?  SHARE THE LOVE, and help us help
YOU!  Click Here: http://sourceforge.net/donate/
_______________________________________________
linux-usb-devel@lists.sourceforge.net
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel

Re: [linux-usb-devel] Re: Unaligned scatter-gather buffers and usb-storage

[Oliver Neukum]

 
> > The answer seems very simple.  There should be a host template entry for
> > dma buffer alignment (there's already a dma_boundary member).  It would be
> > copied into the device's request queue stucture if it is nonzero,
> > overriding the default value of 512.  sg and st should check the user
> > buffer against the request queue's dma_alignment mask and avoid doing
> > direct I/O if the alignment is wrong -- just fall back to normal I/O.
> > 
> > Any objections to this scheme?
> 
> Well yes, that's what my objection is against - adding that member. Did
> you not read any of my mails? And it's quite simple why - basically
> noone will add it, so it'll end up being 512 anyways.

But neither sg nor st currently honor the 512 byte alignment requirement.
Whether this really hurts st in terms of performance is another question.

	Regards
		Oliver

Re: [linux-usb-devel] Re: Unaligned scatter-gather buffers and usb-storage

[Kai Makisara]

On Thu, 20 Nov 2003, Oliver Neukum wrote:

...
> But neither sg nor st currently honor the 512 byte alignment requirement.
> Whether this really hurts st in terms of performance is another question.
>
It depends on hardware :-)

Before implementing direct i/o, I made some measurements with the hardware
available. The tape drive was HP DDS-4 and computer dual PIII 700 MHz. The
results with a test program using well-compressible data (zeroes):

Buffer length 32768 bytes, 32768 buffers written (1024.000 MB, 1073741824 bytes)
Variable block mode (writing 32768 byte blocks).
dio:
write: wall  60.436 user   0.015 (  0.0 %) system   0.485 (  0.8 %) speed 16.944 MB/s
read:  wall  64.580 user   0.014 (  0.0 %) system   0.500 (  0.8 %) speed 15.856 MB/s
no dio:
write: wall  61.373 user   0.024 (  0.0 %) system   2.897 (  4.7 %) speed 16.685 MB/s
read:  wall  66.435 user   0.055 (  0.1 %) system   6.347 (  9.6 %) speed 15.413 MB/s

The common high-end drives at that time streamed at 30-40 MB/s assuming
2:1 compression. It meant that about 20 % of cpu was needed for extra
copies when reading. Other people reported even higher percentages with
other hardware.

I just repeated the tests using the same drive but a 2.6 GHz PIV on a
Intel D875 motherboard and dual channel memory. The results were:

dio:
write: wall  58.668 user   0.013 (  0.0 %) system   0.217 (  0.4 %) speed  17.045 MB/s
read:  wall  63.425 user   0.011 (  0.0 %) system   0.245 (  0.4 %) speed  15.767 MB/s
no dio:
write: wall  58.713 user   0.017 (  0.0 %) system   0.534 (  0.9 %) speed  17.032 MB/s
read:  wall  63.265 user   0.020 (  0.0 %) system   0.805 (  1.3 %) speed  15.807 MB/s

Current ordinary drives stream up to 70 MB/s (assuming 2:1 compression).
This would lead to over 5 % overhead per drive when not doing dio.

-- 
Kai

Re: Re: Unaligned scatter-gather buffers and usb-storage

[Kai Makisara]

On Thu, 20 Nov 2003, Alan Stern wrote:

> On Thu, 20 Nov 2003, Jens Axboe wrote:
>
> > Then you just cannot do zero-copy dma, if the buffer isn't aligned
> > properly. It's as simple as that. And surely that wont kill you
> > performance wise.
> >
> > Doing bounce buffering to just maintain the use of mapping user pages
> > into the kernel ls out right silly.
> >
> > So no, I still don't think we have to add anything. Just don't do_dio if
> > the alignment doesn't allow it. Ditto for sg.
>
> The answer seems very simple.  There should be a host template entry for
> dma buffer alignment (there's already a dma_boundary member).  It would be
> copied into the device's request queue stucture if it is nonzero,
> overriding the default value of 512.  sg and st should check the user
> buffer against the request queue's dma_alignment mask and avoid doing
> direct I/O if the alignment is wrong -- just fall back to normal I/O.
>
> Any objections to this scheme?
>
My only objection is the default. It should be zero. If I read the
usb-storage sources correctly, the scsi_host_template is defined in one
file (scsiglue.c). If the mask is there set to 511, all USB storage
devices get 512-byte alignment and other get no constraints. All get
optimal result with minimal work. (Adding the check to st and sg is
trivial.)

-- 
Kai


-------------------------------------------------------
This SF.net email is sponsored by: SF.net Giveback Program.
Does SourceForge.net help you be more productive?  Does it
help you create better code?  SHARE THE LOVE, and help us help
YOU!  Click Here: http://sourceforge.net/donate/
_______________________________________________
linux-usb-devel@lists.sourceforge.net
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel

Re: [linux-usb-devel] Re: Unaligned scatter-gather buffers and usb-storage

[Kai Mäkisara]

Thus spake Kai Makisara (Kai.Makisara@kolumbus.fi):

> On Thu, 20 Nov 2003, Alan Stern wrote:
> 
...
> > The answer seems very simple.  There should be a host template entry for
> > dma buffer alignment (there's already a dma_boundary member).  It would be
> > copied into the device's request queue stucture if it is nonzero,
> > overriding the default value of 512.  sg and st should check the user
> > buffer against the request queue's dma_alignment mask and avoid doing
> > direct I/O if the alignment is wrong -- just fall back to normal I/O.
> >
> > Any objections to this scheme?
> >
> My only objection is the default. It should be zero. If I read the
> usb-storage sources correctly, the scsi_host_template is defined in one
> file (scsiglue.c). If the mask is there set to 511, all USB storage
> devices get 512-byte alignment and other get no constraints. All get
> optimal result with minimal work. (Adding the check to st and sg is
> trivial.)
> 
To make this a little more than talk, here is a patch for 2.6.0-test9 plus
csets up to today (compiled and tested but does not include the check for sg):
---8<---
diff -ur linux-2.6-cset/drivers/scsi/st.c linux-2.6-cset-k/drivers/scsi/st.c
--- linux-2.6-cset/drivers/scsi/st.c	2003-09-09 18:19:45.000000000 +0300
+++ linux-2.6-cset-k/drivers/scsi/st.c	2003-11-20 20:49:25.000000000 +0200
@@ -1267,7 +1267,8 @@
 		i = STp->try_dio && try_rdio;
 	else
 		i = STp->try_dio && try_wdio;
-	if (i) {
+
+	if (i && ((unsigned int)buf & STp->device->host->hostt->dma_align_mask) == 0) {
 		i = st_map_user_pages(&(STbp->sg[0]), STbp->use_sg,
 				      (unsigned long)buf, count, (is_read ? READ : WRITE),
 				      STp->max_pfn);
diff -ur linux-2.6-cset/drivers/usb/storage/scsiglue.c linux-2.6-cset-k/drivers/usb/storage/scsiglue.c
--- linux-2.6-cset/drivers/usb/storage/scsiglue.c	2003-10-25 22:35:58.000000000 +0300
+++ linux-2.6-cset-k/drivers/usb/storage/scsiglue.c	2003-11-20 19:34:56.000000000 +0200
@@ -312,6 +312,9 @@
 	/* lots of sg segments can be handled */
 	.sg_tablesize =			SG_ALL,
 
+	/* alignment to 512-byte required */
+	.dma_align_mask =		0x1ff,
+
 	/* merge commands... this seems to help performance, but
 	 * periodically someone should test to see which setting is more
 	 * optimal.
Only in linux-2.6-cset/include/asm: page.h~
Only in linux-2.6-cset/include/asm-i386: page.h~
diff -ur linux-2.6-cset/include/scsi/scsi_host.h linux-2.6-cset-k/include/scsi/scsi_host.h
--- linux-2.6-cset/include/scsi/scsi_host.h	2003-10-25 22:35:58.000000000 +0300
+++ linux-2.6-cset-k/include/scsi/scsi_host.h	2003-11-20 19:33:32.000000000 +0200
@@ -268,6 +268,12 @@
 	unsigned long dma_boundary;
 
 	/*
+	 * Alignment mask for scatter/gather segments. The offset and'ed
+	 * with the mask must be zero for segments.
+	 */
+	unsigned int dma_align_mask;
+
+	/*
 	 * This specifies "machine infinity" for host templates which don't
 	 * limit the transfer size.  Note this limit represents an absolute
 	 * maximum, and may be over the transfer limits allowed for
---8<---

-- 
Kai

PATCH: (as141) Unaligned scatter-gather buffers and usb-storage

[Alan Stern]

I'm not sure whether Jens is objecting to adding the extra member to the
host template or to making the default alignment something other than 512.  
Here's my attempt at a compromise patch, which boils the area of
disagreement between Kai and Jens down to a single line of code (plus a
comment).  It doesn't change the host template and it shouldn't slow down
any tape I/O programs when using a non-USB device.

If there aren't any more objections to this, I urge James to apply the
SCSI parts to a post-2.6.0-final tree, and likewise for Matt and the USB 
part.

Alan Stern


===== scsiglue.c 1.60 vs edited =====
--- 1.60/drivers/usb/storage/scsiglue.c	Fri Oct 24 14:53:38 2003
+++ edited/drivers/usb/storage/scsiglue.c	Fri Nov 21 12:45:21 2003
@@ -65,6 +65,16 @@
 
 static int slave_configure (struct scsi_device *sdev)
 {
+	/* Scatter-gather buffers (all but the last) must have a length
+	 * divisible by the bulk maxpacket size.  Otherwise a data packet
+	 * would end up being short, causing a premature end to the data
+	 * transfer.  Since high-speed bulk pipes have a maxpacket size
+	 * of 512, we'll use that as the scsi device queue's DMA alignment
+	 * mask.  Guaranteeing proper alignment of the first buffer will
+	 * have the desired effect because, except at the beginning and
+	 * the end, scatter-gather buffers follow page boundaries. */
+	blk_queue_dma_alignment(sdev->request_queue, (512 - 1));
+
 	/* this is to satisify the compiler, tho I don't think the 
 	 * return code is ever checked anywhere. */
 	return 0;
===== sg.c 1.48 vs edited =====
--- 1.48/drivers/scsi/sg.c	Fri Oct 24 14:53:37 2003
+++ edited/drivers/scsi/sg.c	Fri Nov 21 12:28:52 2003
@@ -1741,7 +1741,11 @@
 	int sg_tablesize = sfp->parentdp->sg_tablesize;
 	struct scatterlist *sgl;
 	int mx_sc_elems, res;
+	struct scsi_device *sdev = sfp->parentdp->device;
 
+	if (((unsigned long)hp->dxferp &
+			queue_dma_alignment(sdev->request_queue)) != 0)
+		return 1;
 	mx_sc_elems = sg_build_sgat(schp, sfp, sg_tablesize);
         if (mx_sc_elems <= 0) {
                 return 1;
===== st.c 1.45 vs edited =====
--- 1.45/drivers/scsi/st.c	Fri Sep  5 12:16:40 2003
+++ edited/drivers/scsi/st.c	Fri Nov 21 12:30:34 2003
@@ -1267,7 +1267,8 @@
 		i = STp->try_dio && try_rdio;
 	else
 		i = STp->try_dio && try_wdio;
-	if (i) {
+	if (i && ((unsigned int)buf & queue_dma_alignment(
+					STp->device->request_queue)) == 0) {
 		i = st_map_user_pages(&(STbp->sg[0]), STbp->use_sg,
 				      (unsigned long)buf, count, (is_read ? READ : WRITE),
 				      STp->max_pfn);
===== scsi_scan.c 1.52 vs edited =====
--- 1.52/drivers/scsi/scsi_scan.c	Fri Oct 24 14:53:37 2003
+++ edited/drivers/scsi/scsi_scan.c	Fri Nov 21 12:34:00 2003
@@ -231,6 +231,15 @@
 	sdev->request_queue->queuedata = sdev;
 	scsi_adjust_queue_depth(sdev, 0, sdev->host->cmd_per_lun);
 
+	/*
+	 * Set the queue's mask to require a mere 8-byte alignment for
+	 * DMA buffers, rather than the default 512.  This shouldn't
+	 * inconvenience any user programs and should be okay for most
+	 * host adapters.  A host driver can alter this mask in its
+	 * slave_alloc() or slave_configure() callback if necessary.
+	 */
+	blk_queue_dma_alignment(sdev->request_queue, (8 - 1));
+
 	if (shost->hostt->slave_alloc) {
 		if (shost->hostt->slave_alloc(sdev))
 			goto out_free_queue;

Re: PATCH: (as141) Unaligned scatter-gather buffers and usb-storage

[Kai Makisara]

On Fri, 21 Nov 2003, Alan Stern wrote:

> I'm not sure whether Jens is objecting to adding the extra member to the
> host template or to making the default alignment something other than 512.
> Here's my attempt at a compromise patch, which boils the area of
> disagreement between Kai and Jens down to a single line of code (plus a
> comment).  It doesn't change the host template and it shouldn't slow down
> any tape I/O programs when using a non-USB device.
>
Looks very good. Using existing mechanisms is better than adding a new
member to a struct.

I tested the patch with a tape and did not find any problems.

> If there aren't any more objections to this, I urge James to apply the
> SCSI parts to a post-2.6.0-final tree, and likewise for Matt and the USB
> part.
>
As far as I am concerned, please do.

-- 
Kai


-------------------------------------------------------
This SF.net email is sponsored by: SF.net Giveback Program.
Does SourceForge.net help you be more productive?  Does it
help you create better code?  SHARE THE LOVE, and help us help
YOU!  Click Here: http://sourceforge.net/donate/
_______________________________________________
linux-usb-devel@lists.sourceforge.net
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel

Re: PATCH: (as141) Unaligned scatter-gather buffers and usb-storage

[Matthew Dharm]

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

I'm going to hold on to this patch until I see the SCSI parts go in.  Alan,
try to remind me if you don't see this get merged after a while.

Matt

On Fri, Nov 21, 2003 at 01:03:05PM -0500, Alan Stern wrote:
> I'm not sure whether Jens is objecting to adding the extra member to the
> host template or to making the default alignment something other than 512.  
> Here's my attempt at a compromise patch, which boils the area of
> disagreement between Kai and Jens down to a single line of code (plus a
> comment).  It doesn't change the host template and it shouldn't slow down
> any tape I/O programs when using a non-USB device.
> 
> If there aren't any more objections to this, I urge James to apply the
> SCSI parts to a post-2.6.0-final tree, and likewise for Matt and the USB 
> part.
> 
> Alan Stern
> 
> 
> ===== scsiglue.c 1.60 vs edited =====
> --- 1.60/drivers/usb/storage/scsiglue.c	Fri Oct 24 14:53:38 2003
> +++ edited/drivers/usb/storage/scsiglue.c	Fri Nov 21 12:45:21 2003
> @@ -65,6 +65,16 @@
>  
>  static int slave_configure (struct scsi_device *sdev)
>  {
> +	/* Scatter-gather buffers (all but the last) must have a length
> +	 * divisible by the bulk maxpacket size.  Otherwise a data packet
> +	 * would end up being short, causing a premature end to the data
> +	 * transfer.  Since high-speed bulk pipes have a maxpacket size
> +	 * of 512, we'll use that as the scsi device queue's DMA alignment
> +	 * mask.  Guaranteeing proper alignment of the first buffer will
> +	 * have the desired effect because, except at the beginning and
> +	 * the end, scatter-gather buffers follow page boundaries. */
> +	blk_queue_dma_alignment(sdev->request_queue, (512 - 1));
> +
>  	/* this is to satisify the compiler, tho I don't think the 
>  	 * return code is ever checked anywhere. */
>  	return 0;
> ===== sg.c 1.48 vs edited =====
> --- 1.48/drivers/scsi/sg.c	Fri Oct 24 14:53:37 2003
> +++ edited/drivers/scsi/sg.c	Fri Nov 21 12:28:52 2003
> @@ -1741,7 +1741,11 @@
>  	int sg_tablesize = sfp->parentdp->sg_tablesize;
>  	struct scatterlist *sgl;
>  	int mx_sc_elems, res;
> +	struct scsi_device *sdev = sfp->parentdp->device;
>  
> +	if (((unsigned long)hp->dxferp &
> +			queue_dma_alignment(sdev->request_queue)) != 0)
> +		return 1;
>  	mx_sc_elems = sg_build_sgat(schp, sfp, sg_tablesize);
>          if (mx_sc_elems <= 0) {
>                  return 1;
> ===== st.c 1.45 vs edited =====
> --- 1.45/drivers/scsi/st.c	Fri Sep  5 12:16:40 2003
> +++ edited/drivers/scsi/st.c	Fri Nov 21 12:30:34 2003
> @@ -1267,7 +1267,8 @@
>  		i = STp->try_dio && try_rdio;
>  	else
>  		i = STp->try_dio && try_wdio;
> -	if (i) {
> +	if (i && ((unsigned int)buf & queue_dma_alignment(
> +					STp->device->request_queue)) == 0) {
>  		i = st_map_user_pages(&(STbp->sg[0]), STbp->use_sg,
>  				      (unsigned long)buf, count, (is_read ? READ : WRITE),
>  				      STp->max_pfn);
> ===== scsi_scan.c 1.52 vs edited =====
> --- 1.52/drivers/scsi/scsi_scan.c	Fri Oct 24 14:53:37 2003
> +++ edited/drivers/scsi/scsi_scan.c	Fri Nov 21 12:34:00 2003
> @@ -231,6 +231,15 @@
>  	sdev->request_queue->queuedata = sdev;
>  	scsi_adjust_queue_depth(sdev, 0, sdev->host->cmd_per_lun);
>  
> +	/*
> +	 * Set the queue's mask to require a mere 8-byte alignment for
> +	 * DMA buffers, rather than the default 512.  This shouldn't
> +	 * inconvenience any user programs and should be okay for most
> +	 * host adapters.  A host driver can alter this mask in its
> +	 * slave_alloc() or slave_configure() callback if necessary.
> +	 */
> +	blk_queue_dma_alignment(sdev->request_queue, (8 - 1));
> +
>  	if (shost->hostt->slave_alloc) {
>  		if (shost->hostt->slave_alloc(sdev))
>  			goto out_free_queue;

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

I want my GPFs!!!
					-- Stef
User Friendly, 11/9/1998

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

Re: PATCH: (as141) Unaligned scatter-gather buffers and usb-storage

[Matthew Dharm]

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

Is there any indication that the SCSI parts of this patch will get
accepted?

Matt

On Fri, Nov 21, 2003 at 01:03:05PM -0500, Alan Stern wrote:
> I'm not sure whether Jens is objecting to adding the extra member to the
> host template or to making the default alignment something other than 512.  
> Here's my attempt at a compromise patch, which boils the area of
> disagreement between Kai and Jens down to a single line of code (plus a
> comment).  It doesn't change the host template and it shouldn't slow down
> any tape I/O programs when using a non-USB device.
> 
> If there aren't any more objections to this, I urge James to apply the
> SCSI parts to a post-2.6.0-final tree, and likewise for Matt and the USB 
> part.
> 
> Alan Stern
> 
> 
> ===== scsiglue.c 1.60 vs edited =====
> --- 1.60/drivers/usb/storage/scsiglue.c	Fri Oct 24 14:53:38 2003
> +++ edited/drivers/usb/storage/scsiglue.c	Fri Nov 21 12:45:21 2003
> @@ -65,6 +65,16 @@
>  
>  static int slave_configure (struct scsi_device *sdev)
>  {
> +	/* Scatter-gather buffers (all but the last) must have a length
> +	 * divisible by the bulk maxpacket size.  Otherwise a data packet
> +	 * would end up being short, causing a premature end to the data
> +	 * transfer.  Since high-speed bulk pipes have a maxpacket size
> +	 * of 512, we'll use that as the scsi device queue's DMA alignment
> +	 * mask.  Guaranteeing proper alignment of the first buffer will
> +	 * have the desired effect because, except at the beginning and
> +	 * the end, scatter-gather buffers follow page boundaries. */
> +	blk_queue_dma_alignment(sdev->request_queue, (512 - 1));
> +
>  	/* this is to satisify the compiler, tho I don't think the 
>  	 * return code is ever checked anywhere. */
>  	return 0;
> ===== sg.c 1.48 vs edited =====
> --- 1.48/drivers/scsi/sg.c	Fri Oct 24 14:53:37 2003
> +++ edited/drivers/scsi/sg.c	Fri Nov 21 12:28:52 2003
> @@ -1741,7 +1741,11 @@
>  	int sg_tablesize = sfp->parentdp->sg_tablesize;
>  	struct scatterlist *sgl;
>  	int mx_sc_elems, res;
> +	struct scsi_device *sdev = sfp->parentdp->device;
>  
> +	if (((unsigned long)hp->dxferp &
> +			queue_dma_alignment(sdev->request_queue)) != 0)
> +		return 1;
>  	mx_sc_elems = sg_build_sgat(schp, sfp, sg_tablesize);
>          if (mx_sc_elems <= 0) {
>                  return 1;
> ===== st.c 1.45 vs edited =====
> --- 1.45/drivers/scsi/st.c	Fri Sep  5 12:16:40 2003
> +++ edited/drivers/scsi/st.c	Fri Nov 21 12:30:34 2003
> @@ -1267,7 +1267,8 @@
>  		i = STp->try_dio && try_rdio;
>  	else
>  		i = STp->try_dio && try_wdio;
> -	if (i) {
> +	if (i && ((unsigned int)buf & queue_dma_alignment(
> +					STp->device->request_queue)) == 0) {
>  		i = st_map_user_pages(&(STbp->sg[0]), STbp->use_sg,
>  				      (unsigned long)buf, count, (is_read ? READ : WRITE),
>  				      STp->max_pfn);
> ===== scsi_scan.c 1.52 vs edited =====
> --- 1.52/drivers/scsi/scsi_scan.c	Fri Oct 24 14:53:37 2003
> +++ edited/drivers/scsi/scsi_scan.c	Fri Nov 21 12:34:00 2003
> @@ -231,6 +231,15 @@
>  	sdev->request_queue->queuedata = sdev;
>  	scsi_adjust_queue_depth(sdev, 0, sdev->host->cmd_per_lun);
>  
> +	/*
> +	 * Set the queue's mask to require a mere 8-byte alignment for
> +	 * DMA buffers, rather than the default 512.  This shouldn't
> +	 * inconvenience any user programs and should be okay for most
> +	 * host adapters.  A host driver can alter this mask in its
> +	 * slave_alloc() or slave_configure() callback if necessary.
> +	 */
> +	blk_queue_dma_alignment(sdev->request_queue, (8 - 1));
> +
>  	if (shost->hostt->slave_alloc) {
>  		if (shost->hostt->slave_alloc(sdev))
>  			goto out_free_queue;

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

I need a computer?
					-- Customer
User Friendly, 2/19/1998

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

Re: PATCH: (as141) Unaligned scatter-gather buffers and usb-storage

[Jens Axboe]

On Sun, Jan 04 2004, Matthew Dharm wrote:
> Is there any indication that the SCSI parts of this patch will get
> accepted?

I'm fine with the patch, FWIW, just move it to scsi_alloc_queue()
instead?

> > ===== scsi_scan.c 1.52 vs edited =====
> > --- 1.52/drivers/scsi/scsi_scan.c	Fri Oct 24 14:53:37 2003
> > +++ edited/drivers/scsi/scsi_scan.c	Fri Nov 21 12:34:00 2003
> > @@ -231,6 +231,15 @@
> >  	sdev->request_queue->queuedata = sdev;
> >  	scsi_adjust_queue_depth(sdev, 0, sdev->host->cmd_per_lun);
> >  
> > +	/*
> > +	 * Set the queue's mask to require a mere 8-byte alignment for
> > +	 * DMA buffers, rather than the default 512.  This shouldn't
> > +	 * inconvenience any user programs and should be okay for most
> > +	 * host adapters.  A host driver can alter this mask in its
> > +	 * slave_alloc() or slave_configure() callback if necessary.
> > +	 */
> > +	blk_queue_dma_alignment(sdev->request_queue, (8 - 1));
> > +
> >  	if (shost->hostt->slave_alloc) {
> >  		if (shost->hostt->slave_alloc(sdev))
> >  			goto out_free_queue;

-- 
Jens Axboe



-------------------------------------------------------
This SF.net email is sponsored by: IBM Linux Tutorials.
Become an expert in LINUX or just sharpen your skills.  Sign up for IBM's
Free Linux Tutorials.  Learn everything from the bash shell to sys admin.
Click now! http://ads.osdn.com/?ad_id=1278&alloc_id=3371&op=click
_______________________________________________
linux-usb-devel@lists.sourceforge.net
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel

PATCH: (as141b) Unaligned scatter-gather buffers and usb-storage

[Alan Stern]

On Mon, 5 Jan 2004, Jens Axboe wrote:

> On Sun, Jan 04 2004, Matthew Dharm wrote:
> > Is there any indication that the SCSI parts of this patch will get
> > accepted?
> 
> I'm fine with the patch, FWIW, just move it to scsi_alloc_queue()
> instead?

Here's the revised patch.

Alan Stern


===== scsiglue.c 1.60 vs edited =====
--- 1.60/drivers/usb/storage/scsiglue.c	Fri Oct 24 14:53:38 2003
+++ edited/drivers/usb/storage/scsiglue.c	Fri Nov 21 12:45:21 2003
@@ -65,6 +65,16 @@
 
 static int slave_configure (struct scsi_device *sdev)
 {
+	/* Scatter-gather buffers (all but the last) must have a length
+	 * divisible by the bulk maxpacket size.  Otherwise a data packet
+	 * would end up being short, causing a premature end to the data
+	 * transfer.  Since high-speed bulk pipes have a maxpacket size
+	 * of 512, we'll use that as the scsi device queue's DMA alignment
+	 * mask.  Guaranteeing proper alignment of the first buffer will
+	 * have the desired effect because, except at the beginning and
+	 * the end, scatter-gather buffers follow page boundaries. */
+	blk_queue_dma_alignment(sdev->request_queue, (512 - 1));
+
 	/* this is to satisify the compiler, tho I don't think the 
 	 * return code is ever checked anywhere. */
 	return 0;
===== sg.c 1.48 vs edited =====
--- 1.48/drivers/scsi/sg.c	Fri Oct 24 14:53:37 2003
+++ edited/drivers/scsi/sg.c	Fri Nov 21 12:28:52 2003
@@ -1741,7 +1741,11 @@
 	int sg_tablesize = sfp->parentdp->sg_tablesize;
 	struct scatterlist *sgl;
 	int mx_sc_elems, res;
+	struct scsi_device *sdev = sfp->parentdp->device;
 
+	if (((unsigned long)hp->dxferp &
+			queue_dma_alignment(sdev->request_queue)) != 0)
+		return 1;
 	mx_sc_elems = sg_build_sgat(schp, sfp, sg_tablesize);
         if (mx_sc_elems <= 0) {
                 return 1;
===== st.c 1.45 vs edited =====
--- 1.45/drivers/scsi/st.c	Fri Sep  5 12:16:40 2003
+++ edited/drivers/scsi/st.c	Fri Nov 21 12:30:34 2003
@@ -1267,7 +1267,8 @@
 		i = STp->try_dio && try_rdio;
 	else
 		i = STp->try_dio && try_wdio;
-	if (i) {
+	if (i && ((unsigned int)buf & queue_dma_alignment(
+					STp->device->request_queue)) == 0) {
 		i = st_map_user_pages(&(STbp->sg[0]), STbp->use_sg,
 				      (unsigned long)buf, count, (is_read ? READ : WRITE),
 				      STp->max_pfn);
===== scsi_lib.c 1.49 vs edited =====
--- 1.49/drivers/scsi/scsi_lib.c	Sat Nov 22 11:20:45 2003
+++ edited/drivers/scsi/scsi_lib.c	Mon Jan  5 16:55:32 2004
@@ -1287,6 +1287,15 @@
 	blk_queue_max_sectors(q, shost->max_sectors);
 	blk_queue_bounce_limit(q, scsi_calculate_bounce_limit(shost));
 	blk_queue_segment_boundary(q, shost->dma_boundary);
+ 
+	/*
+	 * Set the queue's mask to require a mere 8-byte alignment for
+	 * DMA buffers, rather than the default 512.  This shouldn't
+	 * inconvenience any user programs and should be okay for most
+	 * host adapters.  A host driver can alter this mask in its
+	 * slave_alloc() or slave_configure() callback if necessary.
+	 */
+	blk_queue_dma_alignment(sdev->request_queue, (8 - 1));
 
 	if (!shost->use_clustering)
 		clear_bit(QUEUE_FLAG_CLUSTER, &q->queue_flags);

Re: PATCH: (as141b) Unaligned scatter-gather buffers and usb-storage

[Oliver Neukum]

 
>  static int slave_configure (struct scsi_device *sdev)
>  {
> +	/* Scatter-gather buffers (all but the last) must have a length
> +	 * divisible by the bulk maxpacket size.  Otherwise a data packet
> +	 * would end up being short, causing a premature end to the data
> +	 * transfer.  Since high-speed bulk pipes have a maxpacket size
> +	 * of 512, we'll use that as the scsi device queue's DMA alignment
> +	 * mask.  Guaranteeing proper alignment of the first buffer will
> +	 * have the desired effect because, except at the beginning and
> +	 * the end, scatter-gather buffers follow page boundaries. */
> +	blk_queue_dma_alignment(sdev->request_queue, (512 - 1));

What about the cacheline of the last buffer? It seems to me that we have
to worry about DMA coherency.

	Regards
		Oliver

Re: PATCH: (as141b) Unaligned scatter-gather buffers and usb-storage

[Alan Stern]

On Tue, 6 Jan 2004, Oliver Neukum wrote:

> > +	/* Scatter-gather buffers (all but the last) must have a length
> > +	 * divisible by the bulk maxpacket size.  Otherwise a data packet
> > +	 * would end up being short, causing a premature end to the data
> > +	 * transfer.  Since high-speed bulk pipes have a maxpacket size
> > +	 * of 512, we'll use that as the scsi device queue's DMA alignment
> > +	 * mask.  Guaranteeing proper alignment of the first buffer will
> > +	 * have the desired effect because, except at the beginning and
> > +	 * the end, scatter-gather buffers follow page boundaries. */
> > +	blk_queue_dma_alignment(sdev->request_queue, (512 - 1));
> 
> What about the cacheline of the last buffer? It seems to me that we have
> to worry about DMA coherency.

We don't have to worry about that, at least not here in the low-level
driver.  These buffers are passed from user processes.  Suppose a process
wants to write an amount that's smaller than a cacheline.  But that small
amount might be stored in part of a buffer that does end on a cacheline
boundary -- there's no way for the driver to know.  Ultimately the user
process must be responsible for insuring that DMA coherency is maintained.

Alan Stern



-------------------------------------------------------
This SF.net email is sponsored by: IBM Linux Tutorials.
Become an expert in LINUX or just sharpen your skills.  Sign up for IBM's
Free Linux Tutorials.  Learn everything from the bash shell to sys admin.
Click now! http://ads.osdn.com/?ad_id=1278&alloc_id=3371&op=click
_______________________________________________
linux-usb-devel@lists.sourceforge.net
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel

Re: PATCH: (as141b) Unaligned scatter-gather buffers and usb-storage

[James Bottomley]

On Mon, 2004-01-05 at 16:58, Alan Stern wrote:
> ===== st.c 1.45 vs edited =====
> --- 1.45/drivers/scsi/st.c	Fri Sep  5 12:16:40 2003
> +++ edited/drivers/scsi/st.c	Fri Nov 21 12:30:34 2003
> @@ -1267,7 +1267,8 @@
>  		i = STp->try_dio && try_rdio;
>  	else
>  		i = STp->try_dio && try_wdio;
> -	if (i) {
> +	if (i && ((unsigned int)buf & queue_dma_alignment(

This needs to be unsigned long, otherwise it will print a warning on a
64 bit compile.

> ===== scsi_lib.c 1.49 vs edited =====
> --- 1.49/drivers/scsi/scsi_lib.c	Sat Nov 22 11:20:45 2003
> +++ edited/drivers/scsi/scsi_lib.c	Mon Jan  5 16:55:32 2004
> @@ -1287,6 +1287,15 @@
>  	blk_queue_max_sectors(q, shost->max_sectors);
>  	blk_queue_bounce_limit(q, scsi_calculate_bounce_limit(shost));
>  	blk_queue_segment_boundary(q, shost->dma_boundary);
> + 
> +	/*
> +	 * Set the queue's mask to require a mere 8-byte alignment for
> +	 * DMA buffers, rather than the default 512.  This shouldn't
> +	 * inconvenience any user programs and should be okay for most
> +	 * host adapters.  A host driver can alter this mask in its
> +	 * slave_alloc() or slave_configure() callback if necessary.
> +	 */
> +	blk_queue_dma_alignment(sdev->request_queue, (8 - 1));

sdev->request_queue is NULL here (it's filled in by the return from this
function.  The argument should be q.

I'll make the changes and apply the patch.

James

Re: PATCH: (as141b) Unaligned scatter-gather buffers and usb-storage

[Alan Stern]

On 2 Feb 2004, James Bottomley wrote:

> On Mon, 2004-01-05 at 16:58, Alan Stern wrote:
> > ===== st.c 1.45 vs edited =====
> > --- 1.45/drivers/scsi/st.c	Fri Sep  5 12:16:40 2003
> > +++ edited/drivers/scsi/st.c	Fri Nov 21 12:30:34 2003
> > @@ -1267,7 +1267,8 @@
> >  		i = STp->try_dio && try_rdio;
> >  	else
> >  		i = STp->try_dio && try_wdio;
> > -	if (i) {
> > +	if (i && ((unsigned int)buf & queue_dma_alignment(
> 
> This needs to be unsigned long, otherwise it will print a warning on a
> 64 bit compile.

Really?  I'm surprised.  Although I know nothing about the details of 
using gcc on a 64-bit platform, it seems odd that an explicit cast would 
generate a warning.  Was that done deliberately, to try and catch 
inadvertent bugs in address arithmetic?

> > ===== scsi_lib.c 1.49 vs edited =====
> > --- 1.49/drivers/scsi/scsi_lib.c	Sat Nov 22 11:20:45 2003
> > +++ edited/drivers/scsi/scsi_lib.c	Mon Jan  5 16:55:32 2004
> > @@ -1287,6 +1287,15 @@
> >  	blk_queue_max_sectors(q, shost->max_sectors);
> >  	blk_queue_bounce_limit(q, scsi_calculate_bounce_limit(shost));
> >  	blk_queue_segment_boundary(q, shost->dma_boundary);
> > + 
> > +	/*
> > +	 * Set the queue's mask to require a mere 8-byte alignment for
> > +	 * DMA buffers, rather than the default 512.  This shouldn't
> > +	 * inconvenience any user programs and should be okay for most
> > +	 * host adapters.  A host driver can alter this mask in its
> > +	 * slave_alloc() or slave_configure() callback if necessary.
> > +	 */
> > +	blk_queue_dma_alignment(sdev->request_queue, (8 - 1));
> 
> sdev->request_queue is NULL here (it's filled in by the return from this
> function.  The argument should be q.

Whoops.  I cut and pasted the code from somewhere else and didn't notice 
that the variable needed to be changed.

> I'll make the changes and apply the patch.

Thank you.

Alan Stern

Re: PATCH: (as141b) Unaligned scatter-gather buffers and usb-storage

[James Bottomley]

On Tue, 2004-02-03 at 10:47, Alan Stern wrote:
> Really?  I'm surprised.  Although I know nothing about the details of 
> using gcc on a 64-bit platform, it seems odd that an explicit cast would 
> generate a warning.  Was that done deliberately, to try and catch 
> inadvertent bugs in address arithmetic?

Yes, it's specifically to detect incorrect casting.  The warning is
'cast from pointer to integer of different size'.  If you specifically
mean the truncation, you have to first cast to the correct integer size
and then truncate:

(unsigned int)((unsigned long)buf)

To tell the compiler you really, really mean it.

James

Re: PATCH: (as141b) Unaligned scatter-gather buffers and usb-storage

[Matthew Wilcox]

On Tue, Feb 03, 2004 at 10:47:38AM -0500, Alan Stern wrote:
> On 2 Feb 2004, James Bottomley wrote:
> > This needs to be unsigned long, otherwise it will print a warning on a
> > 64 bit compile.
> 
> Really?  I'm surprised.  Although I know nothing about the details of 
> using gcc on a 64-bit platform, it seems odd that an explicit cast would 
> generate a warning.  Was that done deliberately, to try and catch 
> inadvertent bugs in address arithmetic?

Yes, it's something like "Cast from pointer to integer of different
size".  If you really want to cast a pointer to an int, you have to do
(int)(long)ptr; or you can use an intermediate variable like "int foo =
(long)ptr;"

-- 
"Next the statesmen will invent cheap lies, putting the blame upon 
the nation that is attacked, and every man will be glad of those
conscience-soothing falsities, and will diligently study them, and refuse
to examine any refutations of them; and thus he will by and by convince 
himself that the war is just, and will thank God for the better sleep 
he enjoys after this process of grotesque self-deception." -- Mark Twain

Re: Unaligned scatter-gather buffers and usb-storage

[Alan Stern]

On Wed, 19 Nov 2003, Jens Axboe wrote:

> The queue already has such a restriction embedded, see bio_map_user()
> and queue_dma_alignment().

Searching through the 2.6 kernel source gave some interesting results.

queue_dma_alignment() does nothing but set the dma_alignment field of the
request queue structure.  The _only_ place this field is used is in
fs/bio.c by __bio_map_user(), called from bio_map_user().  In turn, the
_only_ place bio_map_user() is called is from drivers/block/scsi_ioctl.c,
and that's only for a certain specific type of SCSI ioctl.  It's not on
any main code path, and it wouldn't get invoked by anything using the st
or sg drivers.  In fact, it looks like scsi_ioctl.c:scsi_cmd_ioctl() is
only called from within the sd driver.

So something needs to be fixed up.  Is there some spot in the block layer 
that's supposed to be checking dma alignments but isn't doing so?

Alan Stern

Re: Unaligned scatter-gather buffers and usb-storage

[Jens Axboe]

On Wed, Nov 19 2003, Alan Stern wrote:
> On Wed, 19 Nov 2003, Jens Axboe wrote:
> 
> > The queue already has such a restriction embedded, see bio_map_user()
> > and queue_dma_alignment().
> 
> Searching through the 2.6 kernel source gave some interesting results.
> 
> queue_dma_alignment() does nothing but set the dma_alignment field of the
> request queue structure.  The _only_ place this field is used is in
> fs/bio.c by __bio_map_user(), called from bio_map_user().  In turn, the
> _only_ place bio_map_user() is called is from drivers/block/scsi_ioctl.c,
> and that's only for a certain specific type of SCSI ioctl.  It's not on

It's for SG_IO.

> any main code path, and it wouldn't get invoked by anything using the st
> or sg drivers.  In fact, it looks like scsi_ioctl.c:scsi_cmd_ioctl() is
> only called from within the sd driver.

It's invoked from user space. Don't mix block and scsi scsi_ioctl() up.
bio_map_user() would be used by a block layer sg driver too, for
instance.

> So something needs to be fixed up.  Is there some spot in the block layer 
> that's supposed to be checking dma alignments but isn't doing so?

The queue dma alignment was added to better cater to cdrecord, for
instance. We need better than 512b granularity there, or you cannot use
DMA on certain types of burns (lots of sector types are non-2kb aligned
there).

If you can 512-byte align the data and transfer from user space, _that
is enough_. The program just needs to be aware of this, it's not an odd
restriction. There are just scenarious where you cannot do this, and
that is why I added the queue dma alignment attribute. To me, it doesn't
sound like your case applies.

-- 
Jens Axboe

Re: Unaligned scatter-gather buffers and usb-storage

[Alan Stern]

On Wed, 19 Nov 2003, Jens Axboe wrote:

> > any main code path, and it wouldn't get invoked by anything using the st
> > or sg drivers.  In fact, it looks like scsi_ioctl.c:scsi_cmd_ioctl() is
> > only called from within the sd driver.
> 
> It's invoked from user space. Don't mix block and scsi scsi_ioctl() up.
> bio_map_user() would be used by a block layer sg driver too, for
> instance.

You've lost me.  scsi_cmd_ioctl() and bio_map_user() are _not_ used by the
sg driver -- did you mean that a hypothetical block layer sg driver would
call them?  That doesn't seem relevant to the point of this thread.  

Although come to think of it, you probably never saw the start of the
thread.  Take a look at

http://marc.theaimsgroup.com/?l=linux-usb-devel&m=106916943618090&w=2

to understand my problem.

> > So something needs to be fixed up.  Is there some spot in the block layer 
> > that's supposed to be checking dma alignments but isn't doing so?
> 
> The queue dma alignment was added to better cater to cdrecord, for
> instance. We need better than 512b granularity there, or you cannot use
> DMA on certain types of burns (lots of sector types are non-2kb aligned
> there).
> 
> If you can 512-byte align the data and transfer from user space, _that
> is enough_. The program just needs to be aware of this, it's not an odd
> restriction. There are just scenarious where you cannot do this, and
> that is why I added the queue dma alignment attribute. To me, it doesn't
> sound like your case applies.

This isn't a userspace problem as such; it's a problem of enforcing dma
alignment restrictions within the block layer or lower.  I'm not working
on a user program; I'm working on the usb-storage driver.  It too needs
512-byte alignment, but under the right circumstances the st and sg
drivers can submit scatter-gather buffers that aren't aligned.  You said
earlier that misaligned buffers would be caught in the block layer
queueing code, but I don't see any place where that happens.

Alan Stern




-------------------------------------------------------
This SF.net email is sponsored by: SF.net Giveback Program.
Does SourceForge.net help you be more productive?  Does it
help you create better code?  SHARE THE LOVE, and help us help
YOU!  Click Here: http://sourceforge.net/donate/
_______________________________________________
linux-usb-devel@lists.sourceforge.net
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel

Re: Unaligned scatter-gather buffers and usb-storage

[Jens Axboe]

On Wed, Nov 19 2003, Alan Stern wrote:
> On Wed, 19 Nov 2003, Jens Axboe wrote:
> 
> > > any main code path, and it wouldn't get invoked by anything using the st
> > > or sg drivers.  In fact, it looks like scsi_ioctl.c:scsi_cmd_ioctl() is
> > > only called from within the sd driver.
> > 
> > It's invoked from user space. Don't mix block and scsi scsi_ioctl() up.
> > bio_map_user() would be used by a block layer sg driver too, for
> > instance.
> 
> You've lost me.  scsi_cmd_ioctl() and bio_map_user() are _not_ used by the
> sg driver -- did you mean that a hypothetical block layer sg driver would
> call them?  That doesn't seem relevant to the point of this thread.  

Well you brought up bio_map_user(), so I'm telling where it's used and
why. The block layer provides SG_IO functionality for non-scsi devices,
that is its current use.

> Although come to think of it, you probably never saw the start of the
> thread.  Take a look at
> 
> http://marc.theaimsgroup.com/?l=linux-usb-devel&m=106916943618090&w=2
> 
> to understand my problem.

Ok I see. Then make sg/st return -EINVAL if the address isn't 512-byte
aligned would be the rigth fix, imo.

> > > So something needs to be fixed up.  Is there some spot in the block layer 
> > > that's supposed to be checking dma alignments but isn't doing so?
> > 
> > The queue dma alignment was added to better cater to cdrecord, for
> > instance. We need better than 512b granularity there, or you cannot use
> > DMA on certain types of burns (lots of sector types are non-2kb aligned
> > there).
> > 
> > If you can 512-byte align the data and transfer from user space, _that
> > is enough_. The program just needs to be aware of this, it's not an odd
> > restriction. There are just scenarious where you cannot do this, and
> > that is why I added the queue dma alignment attribute. To me, it doesn't
> > sound like your case applies.
> 
> This isn't a userspace problem as such; it's a problem of enforcing dma
> alignment restrictions within the block layer or lower.  I'm not working
> on a user program; I'm working on the usb-storage driver.  It too needs
> 512-byte alignment, but under the right circumstances the st and sg
> drivers can submit scatter-gather buffers that aren't aligned.  You said
> earlier that misaligned buffers would be caught in the block layer
> queueing code, but I don't see any place where that happens.

sg/st submissions would never pass through the block layer, even if they
use the queue as a transport between the scsi mid layer. They need to
impose this restrictions themselves. I still don't think this warrants
adding a host template attribute for this. 512-byte alignment is
perfectly fine.

-- 
Jens Axboe

Re: usb-storage and Sony Handycam

[Alan Stern]

Patrick:

For a usb-storage debugging trace of what appears to be exactly the same 
problem, look at

http://bugme.osdl.org/show_bug.cgi?id=1501

Alan Stern



-------------------------------------------------------
This SF.Net email sponsored by: ApacheCon 2003,
16-19 November in Las Vegas. Learn firsthand the latest
developments in Apache, PHP, Perl, XML, Java, MySQL,
WebDAV, and more! http://www.apachecon.com/
_______________________________________________
linux-usb-devel@lists.sourceforge.net
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel