Booting from USB cdrom device doesn't work

[Sven Schnelle]

Hi List,

i have seabios running on a Thinkpad T60 with coreboot and seabios as
Payload. When trying to boot from a USB cdrom, i get the following errors:

Booting from DVD/CD...
wait_td error - status=184007ff
uhci_send_bulk failed
USB transmission failed
wait_td error - status=184007ff
uhci_send_bulk failed
USB transmission failed
WARNING - Timeout at wait_td:427!
uhci_send_bulk failed
USB transmission failed
WARNING - Timeout at wait_td:427!
uhci_send_bulk failed
USB transmission failed
read capacity failed
atapi_is_ready returned -1
WARNING - Timeout at wait_td:427!
uhci_send_bulk failed
USB transmission failed
Boot failed: Could not read from CDROM (code 0003)

I have no knowledge about USB, so if anybody could
help me in interpreting those messages, or has an
other idea what the problem is, that would be great.

Thanks,

Sven

[Kevin O'Connor]

On Sat, Oct 15, 2011 at 06:46:22PM +0200, Sven Schnelle wrote:
> Hi List,
>
> i have seabios running on a Thinkpad T60 with coreboot and seabios as
> Payload. When trying to boot from a USB cdrom, i get the following errors:
>
> Booting from DVD/CD...
> wait_td error - status=184007ff
> uhci_send_bulk failed
> USB transmission failed

The device has responded with a STALL condition.  Unfortunately, the
current SeaBIOS code doesn't have the ability to clear the stall.  I'm
not sure why the device responds with a stall, but SeaBIOS should be
able to handle it.

Do other USB devices work okay?

-Kevin

[Sven Schnelle]

Hi Kevin,

On 10/15/2011 09:43 PM, Kevin O'Connor wrote:
> On Sat, Oct 15, 2011 at 06:46:22PM +0200, Sven Schnelle wrote:
> > Hi List,
> >
> > i have seabios running on a Thinkpad T60 with coreboot and seabios as
> > Payload. When trying to boot from a USB cdrom, i get the following errors:
> >
> > Booting from DVD/CD...
> > wait_td error - status=184007ff
> > uhci_send_bulk failed
> > USB transmission failed
>
> The device has responded with a STALL condition.  Unfortunately, the
> current SeaBIOS code doesn't have the ability to clear the stall.  I'm
> not sure why the device responds with a stall, but SeaBIOS should be
> able to handle it.
>
> Do other USB devices work okay?

Yes. I've investigated that a bit further in the meantime, and the
problem is that seabios doesn't send a START STOP UNIT command before
accessing the device. As soon as i send that command, i can successfully
boot from that USB stick.

I'll prepare a patch for that, but i'm not sure wether i should issue
a TEST UNIT READY command before, or if it's safe to just send that
command in any case.

Regards,

Sven

[PATCH] USB MSC: send START/STOP UNIT command before accessing device

[Sven Schnelle]

Some USB Mass storage devices need this command before being
accessed. request sense in error case and retry the last command
in case it says 'Unit attention'

Signed-off-by: Sven Schnelle 
---
 src/blockcmd.c |   12 ++++++++++++
 src/blockcmd.h |   11 +++++++++++
 src/usb-msc.c  |   44 ++++++++++++++++++++++++++++++++++----------
 3 files changed, 57 insertions(+), 10 deletions(-)

diff --git a/src/blockcmd.c b/src/blockcmd.c
index c9c6845..571f7cf 100644
--- a/src/blockcmd.c
+++ b/src/blockcmd.c
@@ -56,6 +56,18 @@ cdb_get_sense(struct disk_op_s *op, struct cdbres_request_sense *data)
     return cdb_cmd_data(op, &cmd, sizeof(*data));
 }

+int
+cdb_start_stop_unit(struct disk_op_s *op, int stop, int start)
+{
+    struct cdb_start_stop_unit cmd;
+    memset(&cmd, 0, sizeof(cmd));
+    cmd.command = CDB_CMD_START_STOP_UNIT;
+    cmd.loj_start = ((stop & 1) << 1) | (start & 1);
+    op->buf_fl = NULL;
+    op->count = 0;
+    return cdb_cmd_data(op, &cmd, 0);
+}
+
 // Request capacity
 int
 cdb_read_capacity(struct disk_op_s *op, struct cdbres_read_capacity *data)
diff --git a/src/blockcmd.h b/src/blockcmd.h
index 903c435..ebc1161 100644
--- a/src/blockcmd.h
+++ b/src/blockcmd.h
@@ -34,6 +34,7 @@ struct cdbres_read_capacity {

#define CDB_CMD_INQUIRY 0x12
 #define CDB_CMD_REQUEST_SENSE 0x03
+#define CDB_CMD_START_STOP_UNIT 0x1b

struct cdb_request_sense {
     u8 command;
@@ -44,6 +45,16 @@ struct cdb_request_sense {
     u8 pad[10];
 } PACKED;

+struct cdb_start_stop_unit {
+    u8 command;
+    u8 lun;
+    u8 reserved_02;
+    u8 reserved_03;
+    u8 loj_start;
+    u8 control;
+    u8 pad[10];
+} PACKED;
+
 struct cdbres_request_sense {
     u8 errcode;
     u8 segment;
diff --git a/src/usb-msc.c b/src/usb-msc.c
index 13ef93e..deccf35 100644
--- a/src/usb-msc.c
+++ b/src/usb-msc.c
@@ -53,12 +53,14 @@ usb_cmd_data(struct disk_op_s *op, void *cdbcmd, u16 blocksize)
     if (!CONFIG_USB_MSC)
         return 0;

+restart:
     dprintf(16, "usb_cmd_data id=%p write=%d count=%d bs=%d buf=%p\n"
             , op->drive_g, 0, op->count, blocksize, op->buf_fl);
     struct usbdrive_s *udrive_g = container_of(
         op->drive_g, struct usbdrive_s, drive);
     struct usb_pipe *bulkin = GET_GLOBAL(udrive_g->bulkin);
     struct usb_pipe *bulkout = GET_GLOBAL(udrive_g->bulkout);
+    struct cdbres_request_sense sense;

// Setup command block wrapper.
     u32 bytes = blocksize * op->count;
@@ -78,24 +80,42 @@ usb_cmd_data(struct disk_op_s *op, void *cdbcmd, u16 blocksize)
     if (ret)
         goto fail;

-    // Transfer data from device.
-    ret = usb_send_bulk(bulkin, USB_DIR_IN, op->buf_fl, bytes);
-    if (ret)
-        goto fail;
-
+    if (blocksize) {
+	    // Transfer data from device.
+	    ret = usb_send_bulk(bulkin, USB_DIR_IN, op->buf_fl, bytes);
+	    if (ret)
+		    goto fail;
+    }
     // Transfer csw info.
     struct csw_s csw;
     ret = usb_send_bulk(bulkin, USB_DIR_IN
                         , MAKE_FLATPTR(GET_SEG(SS), &csw), sizeof(csw));
     if (ret)
-        goto fail;
+	    goto fail;

-    if (!csw.bCSWStatus)
+    switch(csw.bCSWStatus) {
+    case 0:
         return DISK_RET_SUCCESS;
-    if (csw.bCSWStatus == 2)
-        goto fail;
+    case 2:
+        break;
+    case 1:
+        if (!cdb_get_sense(op, &sense)) {
+            dprintf(1, "SENSE: error %x, key %x, asc %x ascq %x\n",
+                    sense.errcode, sense.flags & 0x0f, sense.asc, sense.ascq);
+
+            switch(sense.flags & 0x0f) {
+                case 6: /* Unit Attention */
+                    goto restart;
+                default:
+                    goto fail;
+            }
+        }
+        break;
+    };

-    op->count -= csw.dCSWDataResidue / blocksize;
+    if(blocksize)
+	    op->count -= csw.dCSWDataResidue / blocksize;
+    dprintf(1, "EBADTRACK\n");
     return DISK_RET_EBADTRACK;

fail:
@@ -226,6 +246,10 @@ usb_msc_init(struct usb_pipe *pipe
     int ret = cdb_get_inquiry(&dop, &data);
     if (ret)
         goto fail;
+
+    if (cdb_start_stop_unit(&dop, 0, 1))
+        goto fail;
+
     char vendor[sizeof(data.vendor)+1], product[sizeof(data.product)+1];
     char rev[sizeof(data.rev)+1];
     strtcpy(vendor, data.vendor, sizeof(vendor));

[PATCH] USB MSC: send START/STOP UNIT command before accessing device

[Kevin O'Connor]

On Sun, Oct 16, 2011 at 11:57:27AM +0200, Sven Schnelle wrote:
> Some USB Mass storage devices need this command before being
> accessed. request sense in error case and retry the last command
> in case it says 'Unit attention'

Hi Sven,

I was looking through your patch and noticed the following:

[...]
> +int
> +cdb_start_stop_unit(struct disk_op_s *op, int stop, int start)
> +{
> struct cdb_start_stop_unit cmd;
> memset(&cmd, 0, sizeof(cmd));
> cmd.command = CDB_CMD_START_STOP_UNIT;

I don't see this command as required in the usb_msc_boot_1.0 spec -
are drives guarenteed to support this command?

[...]
> @@ -53,12 +53,14 @@ usb_cmd_data(struct disk_op_s *op, void *cdbcmd, u16 blocksize)
>      if (!CONFIG_USB_MSC)
>          return 0;
>  
> +restart:
>      dprintf(16, "usb_cmd_data id=%p write=%d count=%d bs=%d buf=%p\n"
>              , op->drive_g, 0, op->count, blocksize, op->buf_fl);

[...]
> case 1:
>    if (!cdb_get_sense(op, &sense)) {
>        dprintf(1, "SENSE: error %x, key %x, asc %x ascq %x\n",
>                sense.errcode, sense.flags & 0x0f, sense.asc, sense.ascq);
>        switch(sense.flags & 0x0f) {
>            case 6: /* Unit Attention */
>                goto restart;

This doesn't look correct - the cdb_get_sense will alter "op", so I
don't think the command can be restarted.  Also, should the
cdb_get_sense fail, it could cause an infinite loop.

Otherwise, both patches look okay to me.  (I still need to boot test
them though.)

-Kevin

[PATCH] USB MSC: send START/STOP UNIT command before accessing device

[Paolo Bonzini]

On 10/16/2011 11:57 AM, Sven Schnelle wrote:
> Some USB Mass storage devices need this command before being
> accessed. request sense in error case and retry the last command
> in case it says 'Unit attention'

If it says UNIT ATTENTION you do not need to do START STOP UNIT. 
Sending REQUEST SENSE will clear the unit attention condition automatically.

That said, sending START STOP UNIT at startup can indeed make sense for 
real hardware, especially a START STOP UNIT command with 
(start=1,loej=1) which would try closing the tray of a CD drive.
> Signed-off-by: Sven Schnelle
>
> src/blockcmd.c |   12 ++++++++++++
>   src/blockcmd.h |   11 +++++++++++
>   src/usb-msc.c  |   44 ++++++++++++++++++++++++++++++++++----------
>   3 files changed, 57 insertions(+), 10 deletions(-)
>
> diff --git a/src/blockcmd.c b/src/blockcmd.c
> index c9c6845..571f7cf 100644
> --- a/src/blockcmd.c
> +++ b/src/blockcmd.c
> @@ -56,6 +56,18 @@ cdb_get_sense(struct disk_op_s *op, struct cdbres_request_sense *data)
>       return cdb_cmd_data(op,&cmd, sizeof(*data));
>   }
>
> +int
> +cdb_start_stop_unit(struct disk_op_s *op, int stop, int start)
> +{
> struct cdb_start_stop_unit cmd;
> memset(&cmd, 0, sizeof(cmd));
> cmd.command = CDB_CMD_START_STOP_UNIT;

Please set bit 0 of byte 1 ("Immed") to 1.  Otherwise, the command might 
take several seconds to complete on a CD and you might get a timeout in 
usb-{u,e}hci.c.
> cmd.loj_start = ((stop&  1)<<  1) | (start&  1);
> op->buf_fl = NULL;
> op->count = 0;
> return cdb_cmd_data(op,&cmd, 0);
>
> +}

Here, bit 0 means "start" and bit 1 means "load/eject" (not "stop"). 
The meaning is:

start=0, loej=0        spin down the drive
     start=0, loej=1        eject the disk (open the tray)
     start=1, loej=0        spin up the drive
     start=1, loej=1        load the disk (close the tray)

So what you are sending below is "start without loading".  As I 
mentioned above, I think it's better if instead you just send the 
command once at startup, with start=loej=1.

The function scsi_is_ready (formerly atapi_is_ready, but now used also 
for hard drives in my series that Kevin just committed) is perfect for 
this, because it is already equipped to poll for the device becoming 
ready.  It would go somewhat like this:

send TEST UNIT READY
     if successful, break
     send REQUEST SENSE
     if error, continue
     key = sense.flags & 0x0f;
     if (key == UNIT_ATTENTION) {
         /* TEST UNIT READY+REQUEST SENSE should have cleared the UNIT
            ATTENTION condition (e.g. POWER ON OCCURRED, or MEDIUM MAY
            HAVE CHANGED).  Retry immediately.  */
         continue;
     } else if (key != NOT_READY) {
         printf ("Drive returned sense %x asc=%x ascq=%x\n",
                 key, sense.asc, sense.ascq);
         return -1;
     } else if (in_progress) {
         /* Do not hit the drive continuously.  */
         usleep(200);
     } else {
         if (sense.asc == 0x3A && sense.ascq == 0x02)
             send START STOP UNIT(Immed=1,LoEj=1,Start=1);
         else if (sense.asc != 0x04 || sense.ascq != 0x01)
             continue;

dprintf(1, "Waiting for device to detect medium... ");
         end = calc_future_tsc(30000);
         in_progress = 1;
     }

What do you think?

Paolo