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

Dave Frodin dave at se-eng.com
Fri Mar 16 22:20:24 CET 2012


I tested this (the combined patches) with several of the different USB
thumbdrives that I have and didn't have any problems.
In fact it actually increased the pass rate of my failing Ubuntu booting
problem from ~0% to ~50%, but I'll address that in that email stream.

dave

----- Original Message -----
> From: "Paolo Bonzini" <pbonzini at redhat.com>
> To: "Kevin O'Connor" <kevin at koconnor.net>
> Cc: seabios at seabios.org
> Sent: Friday, March 16, 2012 11:54:46 AM
> Subject: Re: [SeaBIOS] usb-msc.c: TEST_UNIT_READY needs USB_DIR_OUT?
> 
> 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 <kevin at koconnor.net>
> > 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 <kevin at koconnor.net>
> > ---
> >  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
> 
> _______________________________________________
> SeaBIOS mailing list
> SeaBIOS at seabios.org
> http://www.seabios.org/mailman/listinfo/seabios
> 



More information about the SeaBIOS mailing list