Re: [PATCH] mass storage : emulation of sat scsi_pass_thru with ATACB

From: Matthew Dharm
Date: Sun Mar 09 2008 - 21:20:30 EST


On Sun, Mar 09, 2008 at 10:42:50PM +0100, matthieu castet wrote:
> Hi
>
> here an update version of the patch

We're getting very close here.

> + /* this value can change, but most(all ?) manufacturers keep the
> cypress
> + * default : 0x24
> + */

Line wrap problem. This happens a few times in your patch, but this was
the first one I noticed.

> + srb->cmnd[0] = 0x24;
> + srb->cmnd[1] = 0x24;
> + srb->cmnd[2] = 0;
> +
> + srb->cmnd[3] = 0xff - 1;
> + srb->cmnd[4] = 1;

What are these magic constants? Symbolic names or comments would be
apropriate here.

> + if (srb->cmnd[12] == ATA_CMD_ID_ATA || srb->cmnd[12] ==
> ATA_CMD_ID_ATAPI)
> + srb->cmnd[2] |= (1<<7);

What does this block do? In general, you could use some more comments
about the code flow...

> + /* if the device doesn't support ATACB
> + * abort and register usb_stor_transparent_scsi_command
> + * callback
> + */
> + if (srb->result == SAM_STAT_CHECK_CONDITION &&
> + memcmp(srb->sense_buffer, usb_stor_sense_invalidCDB,
> + sizeof(usb_stor_sense_invalidCDB)) == 0) {
> + us->subclass = US_SC_SCSI;
> + us->proto_handler = usb_stor_transparent_scsi_command;
> + goto end;
> + }

This is an interesting section of code. Do you have reason to believe that
some of the units you've claimed via unusual_devs.h do not support this
protocol, or are you just being forward-looking?

Either way, some debug printout would be good here...

> --- linux-2.6.24.2.orig/include/linux/usb_usual.h 2008-03-09
> 21:15:01.000000000 +0100
> +++ linux-2.6.24.2/include/linux/usb_usual.h 2008-03-09
> 21:24:12.000000000 +0100
> @@ -81,8 +81,9 @@
> #define US_SC_8070 0x05 /* Removable media */
> #define US_SC_SCSI 0x06 /* Transparent */
> #define US_SC_ISD200 0x07 /* ISD200 ATA */
> +#define US_SC_CYP_ATACB 0x08 /* Cypress ATACB */
> #define US_SC_MIN US_SC_RBC
> -#define US_SC_MAX US_SC_ISD200
> +#define US_SC_MAX US_SC_CYP_ATACB
>
> #define US_SC_DEVICE 0xff /* Use device's value */

I thought we changed this so that the non-spec protocols started from 0xfe
and worked their way down?

Matt

--
Matthew Dharm Home: mdharm-usb@xxxxxxxxxxxxxxxxxx
Maintainer, Linux USB Mass Storage Driver

C: They kicked your ass, didn't they?
S: They were cheating!
-- The Chief and Stef
User Friendly, 11/19/1997

Attachment: pgp00000.pgp
Description: PGP signature