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

[matthieu castet <castet.matthieu@free.fr>]

Matthew Dharm wrote:
> On Sat, Mar 08, 2008 at 09:08:26PM +0100, matthieu castet wrote:
>> Hi Matthew,
>>
>> thanks for your comments
>>
>> Matthew Dharm wrote:
>>> Why are you using an initializer instead of a new protocol code?
>> Because using a new protocol code means I need to patch all the place 
>> where there is a comparison between us->subclass and US_SC_SCSI.
>> After all I am US_SC_SCSI with a special case for ATA12 & ATA16 commands.
>> I don't translate all scsi to atacb (that's what does US_SC_ISD200).
> 
> Yet, you call invoke_transport directly, just like any other protocol
> handler.
> 
> The proper way to do this is as a separate protocol handler.  If you want
> to make it clear that you are only intercepting a couple of command types,
> then don't call invoke_transport() directly, call the transparent scsi
> protocol handler (which, of course, does the same thing but provides
> clearer layering).
> 
> Oh, and you should add some "unlikely" tags to these if() conditionals.
Hum, may be to avoid confusion with a new protocol handler, I can add my 
hook in usb_stor_control_thread with a new flag.

Something like :
[...]
  /* Handle those devices which need us to fake
          * their inquiry data */
         else if ((us->srb->cmnd[0] == INQUIRY) &&
                 (us->flags & US_FL_FIX_INQUIRY)) {
[...]
	else if (( (us->srb->cmnd[0] == ATA_12) || (us->srb->cmnd[0] == ATA_16)) &&
                 (us->flags & US_FL_CYPRESS_ATACB)) {
         US_DEBUGP("emulating ATA pass thru\n");
		call to emulate_pass_thru_with_atacb code
         }

         /* we've got a command, let's do it! */
         else {
             US_DEBUG(usb_stor_show_command(us->srb));
             us->proto_handler(us->srb, us);
         }

Does it sound better ?

Matthieu