Re: [SeaBIOS] usb-msc.c: TEST_UNIT_READY needs USB_DIR_OUT?

[Peter Stuge]

Steve Goodrich wrote:
> the USB/SD adapter would start enumeration, but would fail out
> during TEST_UNIT_READY because the device came back with a STALL
> (this via a LeCroy/CACT USB analyzer).  I compared the transactions
> to those done during USB enumeration/configuration in Linux, and I
> noticed that the Direction bit in bmCBWFlags is *cleared* for the
> TEST_UNIT_READY command in Linux, but *set* in coreboot.
>
> In both cases, dCBWDataTransferLength is zero.
>
> The USB MSC spec that I'm using states: 
>    If [dCBWDataTransferLength] is zero, the device and the host 
>    shall transfer no data between the CBW and the associated CSW, 
>    and the device shall ignore the value of the Direction bit in 
>    bmCBWFlags.
>
> Obviously, this device is not ignoring the Direction flag.  So...
> what should it be?  I modified Paolo's patch (usb-msc.c,
> usb_cmd_data() function) from 27 Feb 2012 to look like this:
>     cbw.dCBWDataTransferLength = bytes;
>     if ((cbw.CBWCB[0] == CDB_CMD_WRITE_10) ||
>    	(cbw.CBWCB[0] == CDB_CMD_TEST_UNIT_READY)) {
>    	cbw.bmCBWFlags =  USB_DIR_OUT;
>     } else {
>    	cbw.bmCBWFlags =  USB_DIR_IN;
>     }
>     cbw.bCBWLUN = 0; // XXX

It would have helped to see a patch rather than only the new code.
> This change allows the device to work, but:
> Is this the right way to do this?

It's not more right or wrong than the other way around.
> Will this break anything else?

I'm sure it will break plenty of other non-compliant USB MSC devices
which also do not ignore the direction bit and require it to be 1.
> Are there other commands that should be set for OUT instead of IN?

Hm, commands? Not sure what you mean here.

The only other thing you could do is to make the code detect the
STALL, clear the stall, and then try with the direction bit inverted.

This will work with devices which are not too broken to support
clearing the stall. This may be a subset or a superset of those with
broken MSC bulk-only implementations. Lots of fun when engineers are
too stupid to follow specifications. :(

The behavior might actually depend on the SD card rather than the
USB-to-SD converter chip. This may or may not be relevant for you
to investigate.

//Peter

usb-msc.c: TEST_UNIT_READY needs USB_DIR_OUT?

[Paolo Bonzini]

Il 14/03/2012 21:53, Peter Stuge ha scritto:
> > Will this break anything else?
>
> I'm sure it will break plenty of other non-compliant USB MSC devices
> which also do not ignore the direction bit and require it to be 1.

Linux FWIW always sets it to 0 for DMA_NONE SCSI commands.  So I think
Steve's (implied) patch should be good.

Paolo

usb-msc.c: TEST_UNIT_READY needs USB_DIR_OUT?

[Steve Goodrich]

> > Are there other commands that should be set for OUT instead of IN?
>
> Hm, commands? Not sure what you mean here.

Commands like:
    CDB_CMD_WRITE_10
    CDB_CMD_TEST_UNIT_READY
    CDB_CMD_INQUIRY
    CDB_CMD_MODE_SENSE
    etc.

If it is easier to review, here is the patch I generated:
> From 4b0a7d2587e4476cee9a1703f591033e5d874bac Mon Sep 17 00:00:00 2001

From: Steve Goodrich 
Date: Tue, 13 Mar 2012 11:12:44 -0600
Subject: [PATCH] TEST UNIT READY must be OUT, not IN

---
 src/usb-msc.c |    7 ++++++-
 1 files changed, 6 insertions(+), 1 deletions(-)

diff --git a/src/usb-msc.c b/src/usb-msc.c
index e143401..efe90e8 100644
--- a/src/usb-msc.c
+++ b/src/usb-msc.c
@@ -77,7 +77,12 @@ usb_cmd_data(struct disk_op_s *op, void *cdbcmd, u16
blocksize)
     cbw.dCBWSignature = CBW_SIGNATURE;
     cbw.dCBWTag = 999; // XXX
     cbw.dCBWDataTransferLength = bytes;
-    cbw.bmCBWFlags = (cbw.CBWCB[0] == CDB_CMD_WRITE_10) ? USB_DIR_OUT :
USB_DIR_IN;
+    if ((cbw.CBWCB[0] == CDB_CMD_WRITE_10) ||
+		(cbw.CBWCB[0] == CDB_CMD_TEST_UNIT_READY)) {
+		cbw.bmCBWFlags =  USB_DIR_OUT;
+    } else {
+		cbw.bmCBWFlags =  USB_DIR_IN;
+    }
     cbw.bCBWLUN = 0; // XXX
     cbw.bCBWCBLength = USB_CDB_SIZE;