Re: [SeaBIOS] [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