Messages in this thread | | | Date | Sat, 8 Mar 2008 13:21:48 -0800 | From | Matthew Dharm <> | Subject | Re: [PATCH] mass storage : emulation of sat scsi_pass_thru with ATACB |
| |
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.
> >Actually, why do you even have a separate 'dispatcher' function? Why not > >just one protocol handler function which checks the command at the top and > >calls invoke_transport there? > What do you means by having a separate 'dispatcher' function? > You means why I have 2 functions emulate_pass_thru_with_atacb and > usb_stor_transparent_scsi_command_atacb ? > I did 2 functions for having a code more clean. > > You suggest something like > void usb_stor_transparent_scsi_command_atacb(struct scsi_cmnd *srb, > struct us_data *us) > { > if (srb->cmnd[0] != ATA_16 && srb->cmnd[0] != ATA_12) { > usb_stor_invoke_transport(srb, us); > return; > } > copy emulate_pass_thru_with_atacb code here > }
Yes, modulo my comment above about calling the transparent scsi protocol handler instead of invoke_transport directly.
> >Also, unless ATACB is a new standard (and I don't think it is, as the > >Cypress datasheet uses the term 'vendor specific'), then your functions > >need renaming. Instead of 'emulate_pass_thru_with_atacb', how about > >something like 'cypress_atacb' -- since it's already a protocol handler, > >everyone already knows it's for passing commands. > But 'emulate_pass_thru_with_atacb' only handle ATA pass_thru scsi > commands. It doesn't translate all scsi commands to atacb like > 'cypress_atacb' could suggest. > That's why I put 'usb_stor_transparent_scsi_command_atacb' saying it is > transparent_scsi_command + atacb support.
Yes, but your name suggests that ATACB is a new industry standard which is implemented by more than a few chips from one specific vendor. That's not acceptable.
Try 'cypress_atacb_passthrough' instead?
Matt
-- Matthew Dharm Home: mdharm-usb@one-eyed-alien.net Maintainer, Linux USB Mass Storage Driver
P: Nine more messages in admin.policy. M: I know, I'm typing as fast as I can! -- Pitr and Mike User Friendly, 11/27/97 [unhandled content-type:application/pgp-signature] | |