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

[Paolo Bonzini]

Il 15/03/2012 02:31, Kevin O'Connor ha scritto:
> On Wed, Mar 14, 2012 at 01:02:39PM -0600, Steve Goodrich wrote:
> > I've been working with coreboot and SeaBIOS lately to try to get a platform
> > working, including having boot capability using an SD-to-USB adapter.  I got
> > stuck on this last part: 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.
>
> If Linux always clears the direction bit for zero length transfers,
> then I think it should be safe to do that in SeaBIOS also.  How about
> the patch below?
>
> -Kevin
>
> From 1fd9a89082b807a4bb4ab6ce1285df474cb75746 Mon Sep 17 00:00:00 2001
> From: Kevin O'Connor 
> Date: Wed, 14 Mar 2012 21:27:45 -0400
> Subject: [PATCH] Use OUT mode for all zero byte "scsi" transfers.
>
> Some devices can get confused if asked to "read" data during a zero
> byte transfer, so consider these transfers as "writes".  (Reported by
> Steve Goodrich.)
>
> Also, extract out the code to determine the transfer direction into
> cdb_is_read().
> Signed-off-by: Kevin O'Connor
>
> src/blockcmd.c    |    7 +++++++
>  src/blockcmd.h    |    1 +
>  src/usb-msc.c     |    4 ++--
>  src/virtio-scsi.c |    7 ++++---
>  4 files changed, 14 insertions(+), 5 deletions(-)
>
> diff --git a/src/blockcmd.c b/src/blockcmd.c
> index 8d881a5..2dda04d 100644
> --- a/src/blockcmd.c
> +++ b/src/blockcmd.c
> @@ -35,6 +35,13 @@ cdb_cmd_data(struct disk_op_s *op, void *cdbcmd, u16 blocksize)
>      }
>  }
>
> +// Determine if the command is a request to pull data from the device
> +int
> +cdb_is_read(u8 *cdbcmd, u16 blocksize)
> +{
> return blocksize && cdbcmd[0] != CDB_CMD_WRITE_10;
>
> +}
>
> int
>  scsi_is_ready(struct disk_op_s *op)
>  {
> diff --git a/src/blockcmd.h b/src/blockcmd.h
> index ccfeeaf..8459d3e 100644
> --- a/src/blockcmd.h
> +++ b/src/blockcmd.h
> @@ -100,6 +100,7 @@ struct cdbres_mode_sense_geom {
>  } PACKED;
>
> // blockcmd.c
> +int cdb_is_read(u8 *cdbcmd, u16 blocksize);
>  int cdb_get_inquiry(struct disk_op_s *op, struct cdbres_inquiry *data);
>  int cdb_get_sense(struct disk_op_s *op, struct cdbres_request_sense *data);
>  int cdb_test_unit_ready(struct disk_op_s *op);
> diff --git a/src/usb-msc.c b/src/usb-msc.c
> index dadb9ca..c53a1f5 100644
> --- a/src/usb-msc.c
> +++ b/src/usb-msc.c
> @@ -77,13 +77,13 @@ 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;
>
> cbw.bmCBWFlags = cdb_is_read(cdbcmd, blocksize) ? USB_DIR_IN : USB_DIR_OUT;
> cbw.bCBWLUN = 0; // XXX
> cbw.bCBWCBLength = USB_CDB_SIZE;
>
> // Transfer cbw to device.
> int ret = usb_msc_send(udrive_g, USB_DIR_OUT
>                        , MAKE_FLATPTR(GET_SEG(SS), &cbw), sizeof(cbw));
>                       , MAKE_FLATPTR(GET_SEG(SS), &cbw), sizeof(cbw));
> if (ret)
>        goto fail;
>
> diff --git a/src/virtio-scsi.c b/src/virtio-scsi.c
> index 3c59438..50339d7 100644
> --- a/src/virtio-scsi.c
> +++ b/src/virtio-scsi.c
> @@ -31,7 +31,7 @@ struct virtio_lun_s {
>
> static int
>  virtio_scsi_cmd(u16 ioaddr, struct vring_virtqueue *vq, struct disk_op_s *op,
>            void *cdbcmd, u16 target, u16 lun, u32 len)
>            void *cdbcmd, u16 target, u16 lun, u16 blocksize)
>
> {
>      struct virtio_scsi_req_cmd req;
>      struct virtio_scsi_resp_cmd resp;
> @@ -44,7 +44,8 @@ virtio_scsi_cmd(u16 ioaddr, struct vring_virtqueue *vq, struct disk_op_s *op,
>      req.lun[3] = (lun & 0xff);
>      memcpy(req.cdb, cdbcmd, 16);
> int datain = (req.cdb[0] != CDB_CMD_WRITE_10);
> u32 len = op->count * blocksize;
> int datain = cdb_is_read(cdbcmd, blocksize);

The patch changes the way TEST_UNIT_READY is composed in the
buffers and breaks virtio-scsi.  Now I remember why I did it
this way. :)

You can squash this in:

diff --git a/src/virtio-scsi.c b/src/virtio-scsi.c
index 76c5f29..d03a562 100644
--- a/src/virtio-scsi.c
+++ b/src/virtio-scsi.c
@@ -46,9 +46,8 @@ virtio_scsi_cmd(u16 ioaddr, struct vring_virtqueue *vq, struct disk_op_s *op,

u32 len = op->count * blocksize;
     int datain = cdb_is_read(cdbcmd, blocksize);
-    int data_idx = (datain ? 2 : 1);
-    int out_num = (datain ? 1 : 2);
-    int in_num = (len ? 3 : 2) - out_num;
+    int in_num = (datain ? 2 : 1);
+    int out_num = (len ? 3 : 2) - in_num;

sg[0].addr   = MAKE_FLATPTR(GET_SEG(SS), &req);
     sg[0].length = sizeof(req);
@@ -56,8 +56,11 @@ virtio_scsi_cmd(u16 ioaddr, struct vring_virtqueue *vq, struct disk_op_s *op,
     sg[out_num].addr   = MAKE_FLATPTR(GET_SEG(SS), &resp);
     sg[out_num].length = sizeof(resp);

-    sg[data_idx].addr   = op->buf_fl;
-    sg[data_idx].length = len;
+    if (len) {
+        int data_idx = (datain ? 2 : 1);
+        sg[data_idx].addr   = op->buf_fl;
+        sg[data_idx].length = len;
+    }

/* Add to virtqueue and kick host */
     vring_add_buf(vq, sg, out_num, in_num, 0, 0);

and apply the patch.

Thanks!

Paolo