On Thu, 1 Apr 2010, Matthew Dharm wrote: > On Thu, Apr 01, 2010 at 10:38:51AM -0700, Hrant Dalalyan wrote: > > Implementation of USB Attached SCSI Protocol per UASP Specification > > (Rev.1, July 9, 2008). Below is the list of the enhancements made to > > the usb-storage driver. > > I've only started reviewing this. See comments inline. > > One overarching comment -- I think you're overreaching with this single > patch. Implementing UASP support should be done first. Then, we can look > at adding multiple commands in-flight separately. I disagree with part of this. The main advantage of UASP over the older protocols is that allows concurrent execution of multiple commands. If you're only going to do one command at a time, you almost might as well not bother. The thing is, concurrent execution affects the whole design of the driver. It can't work like usb-storage does, where we send off an URB, wait for the response, send off a buffer of data, wait for the response, etc. It has to be event-driven, because the device will send back replies in whatever order it wants, not in the same order as the original URBs were sent. So even if you don't want to support concurrent execution initially, you have to design for it in advance. Otherwise, adding it would require the entire driver to be rewritten anyhow. And once you have a good design, adding the support is a pretty easy change. > Most specifically, having both multi-command-queueing AND support for UASP > added in a single mega-patch makes it very very difficult to review. It > looks like there is a LOT of code duplication to avoid disturbing existing > code paths; that's an admirable goal, but I think this is NOT the way to go > about doing it. > > If the existing infrastructure of the usb-storage driver, with a single > protocol and transport pointer isn't sufficient, then let's enhance the > framework. Otherwise, this driver might as well as be completely separate > from usb-storage, as just about the only parts it shares with the other > protocol/transport drivers is the top-level interface into the SCSI core. Yes, that's the way it should end up going -- separate from usb-storage. > This either needs to exist in a unified framework with all the other > sub-drivers, or should just be a separate driver completely. I would > prefer the former. It could be done as a sub-driver, but my guess is that it wouldn't end up using much of the existing core usb-storage code. Which means that it might just as well be a whole new driver. Alan Stern -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
