Matthew Dharm wrote:
> 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.
Ha, may be my mailer is broken with inline patch. I will attach the next 
one.

> 
>> +	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.
Ok I will change that
> 
>> +	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...
It's a bit that should be set for these commands. I will add a comment
about that.
> 
>> +	/* 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?
when I did this code, I was wondering where to plug the hook and was 
thinking about auto-discovering of atacb device.
I think I can remove it now and replace it with a message.
But I should find a way to discover other devices supporting atacb. May 
be I will do a userspace tool for that.

> 
> 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?
Oh, I suppose I need to port this to lastest git.

- I updated the patch to git version (I have to change some stuff for 
scsi) and I follow the above comments.


Matthieu

PS : I don't have a running kernel git version. So I only build the mass 
storage driver with this patch, I didn't run it.
----