Re: [SeaBIOS] [PATCH 1/2] usb attached scsi boot support

[Kevin O'Connor]

On Thu, Jul 19, 2012 at 06:15:02PM +0200, Gerd Hoffmann wrote:
> This patch adds support for booting from UAS (usb
> attached scsi) devices.
>
> For now only usb 2.0 support is there.  On usb 3.0 the
> UAS protocol uses streams, so changes will be required
> to make usb 3.0 devices fly once we have a xhci host
> controller driver.
>
> So far the driver has been tested on qemu-emulated
> virtual hardware only.  In theory should just work on
> bare metal too.

Thanks.  Looks good to me.  I have a few minor comments below.

[...]
> --- a/src/block.c
> +++ b/src/block.c
> @@ -280,7 +280,7 @@ map_floppy_drive(struct drive_s *drive_g)
>  static int
>  process_scsi_op(struct disk_op_s *op)
>  {
> if (!CONFIG_VIRTIO_SCSI && !CONFIG_USB_MSC)
> if (!CONFIG_VIRTIO_SCSI && !CONFIG_USB_MSC && !CONFIG_USB_UAS)

I think I'll submit a patch to drop this config check - it's not
really needed.

[...]
> --- a/src/blockcmd.c
> +++ b/src/blockcmd.c

[...]
> int
>  scsi_init_drive(struct drive_s *drive, const char *s, int prio)
>  {
> if (!CONFIG_USB_MSC && !CONFIG_VIRTIO_SCSI)
> if (!CONFIG_USB_UAS && !CONFIG_USB_MSC && !CONFIG_VIRTIO_SCSI)
>     return 0;

Same here.

[...]
> +int
> +usb_uas_init(struct usbdevice_s *usbdev)
> +{
> if (!CONFIG_USB_UAS)
>    return -1;
> // Verify right kind of device
> struct usb_interface_descriptor *iface = usbdev->iface;
> if (iface->bInterfaceSubClass != 0x06 ||
>    iface->bInterfaceProtocol != 0x62) {

It would be preferable to #define constants for these numbers.

[...]
> while (desc) {
>    desc += desc[0];
>    switch (desc[1]) {
>    case USB_DT_ENDPOINT:
>        ep = (void*)desc;
>        break;
>    case 0x24:

Same here.

[...]
> --- /dev/null
> +++ b/src/usb-uas.h
> @@ -0,0 +1,2 @@
> +int uas_cmd_data(struct disk_op_s *op, void *cdbcmd, u16 blocksize);
> +int usb_uas_init(struct usbdevice_s *usbdev);

Due to SeaBIOS' use of -fcombine during the build, all headers really
should have #ifdef guards on them.

[...]
> else if (iface->bInterfaceClass == USB_CLASS_MASS_STORAGE) {
>    if (iface->bInterfaceProtocol == 0x50)
>        ret = usb_msc_init(usbdev);
>    if (iface->bInterfaceProtocol == 0x62)
>        ret = usb_uas_init(usbdev);

Would prefer #define names.

-Kevin