Hi, Looking through the USB serial driver code one is quite quickly struck by the amount of code which could have been shared but instead is reimplemented (or even copied more or less verbatim) in many drivers. The various custom fifo-implementations are obvious examples, but there is even more that can be be done in terms of code reuse. I've worked with three drivers that have somewhat different characteristics: cp210x a generic driver which uses the generic read and write (fifo) implementations ftdi_sio custom read and write implementations, where the latter allocates urbs and buffer on the fly pl2303 custom read and write implementations, where the latter uses a custom fifo implementation My plan was to improve the generic driver and its interfaces and to make ftdi_sio and pl2303 use the generic implementations as well, while using cp210x as a baseline for my changes to the generic driver. It turns out that the amount of code that can be shared is quite high: drivers/usb/serial/cp210x.c | 16 +-- drivers/usb/serial/ftdi_sio.c | 375 +++++------------------------------ drivers/usb/serial/generic.c | 280 +++++++++++--------------- drivers/usb/serial/pl2303.c | 421 +++------------------------------------ drivers/usb/serial/usb-serial.c | 2 + drivers/usb/serial/usb_debug.c | 4 +- include/linux/usb/serial.h | 19 ++- 7 files changed, 208 insertions(+), 909 deletions(-) As a further example of the potential of the interface changes I propose below, I also turned the aircable driver into a generic one: drivers/usb/serial/aircable.c | 498 ++++------------------------------------- 1 files changed, 46 insertions(+), 452 deletions(-) The patches listed below can be divided into sub-series that are fairly independent but I decided to submit them all at once. I would claim that most of them are fairly uncontroversial but I'm specifically asking for comments on the prepare_write_buffer interface. The patches are against 2.6.34-rc1 with the patches from Greg's tree applied. Note that they also depend on the patch series of fixes I recently submitted separately (USB: serial: generic driver fixes and bulk buffer feature). Here is a summary: 1. Read patches These patches clean up the generic driver's read implementation and generalises it by adding a new function pointer to usb_serial_driver called process_read_urb. The idea is that drivers should not have to reimplement read urb handling (submission, re-submission, throttling etc) but rather be handed the received bulk data to be processed as needed before pushing it to the line discipline. Both pl2303 and ftdi_sio handles overruns, breaks, etc here, whereas cp210x uses the default implementation which simply pushes to ldisc. The ftdi_sio (and aircable) also takes care of headers in incoming data here. 2. Fifo write patches This sub-series starts off with a couple of clean-ups. The custom pl2303 fifo-based write implementation is then thrown out and replaced by the generic kfifo-based one. This patch was actually submitted a couple of days ago (and this is the reason why I did not merge it with the following patch -- "use generic close"). 3. Multi-urb write patches There are currently two write implementations in the generic driver -- one uses a kfifo and the other allocates urbs and buffer on the fly. The latter is similar to what the ftdi_sio is doing but it is still not quite the same. The current generic multi-urb implementation splits an incoming write request into multiple urbs and endpoint-sized buffers (e.g. 64b) instead of allocating a single urb and large-enough buffer (e.g. 2k) and let the host controller to the partitioning to fit end-point size which is more efficient. Based on this observation I reimplement multi-urb writes so that only one buffer allocation and one urb allocation is done per write request. Before changing this I decided to let the usb_debug driver -- the only current user of multi-urb writes -- use the kfifo-based write-implementation (which wasn't available when multi-urb writes was added to usb_debug). It seems to make more sense to use a fifo with a small bulk_out_size (the device seem to require 8 byte buffers) than to allow up to 4000 urbs and buffers to be allocated and dispatched to the host stack. 4. Write prepare Now we have all the infrastructure in place to turn most drivers into generic ones: either using fifo-based or multi-urb-based writes. The rationale for keeping both schemes is that some devices require more than a single urb to achieve maximum throughput at high baudrates (e.g. ftdi_sio at 3Mbaud). There is however a category of devices which can't easily be turned into generic ones -- devices that require headers to be added to the bulk out data before it is sent. The legacy FTDI SIO device is an example of this. I propose an interface to let drivers process the outgoing data by adding a new function prototype to usb_serial_driver called prepare_write_buffer which will be called from the generic write implementations. Unfortunately, things get a little more complicated than in the read case as we have two generic write implementations. The solution I've chosen (after some iteration), and which I'm asking for feedback on, is to have one function being able to handle both schemes. The prototype looks like this: int (*prepare_write_buffer)(struct usb_serial_port *port, void **dest, size_t size, const void *src, size_t count); With rules such as "allocate *dest if NULL", and "use port->write_fifo if src is NULL" both schemes can be incorporated. Perhaps two different prototypes could be used instead (prepare_write_buffer_fifo/prepare_write_buffer_multi). The generic prepare_write_buffer implementation handles both cases, but drivers could choose to only support only one mode (e.g. only fifo-based writes). I use the prepare_write feature to finally turn ftdi_sio into a fully generic driver as well. 5. Example To further exemplify the power and usefulness of prepare_write_urb and process_read_urb I also convert the aircable driver into a generic one. This drivers does not handle line status, but the aircable protocol uses headers for both incoming and outgoing bulk data. Note however that this final patch has been compile-only tested. Thanks, Johan Johan Hovold (20): 1. read USB: serial: refactor read urb submission in generic driver USB: serial: remove unnecessary re-initialisation of generic urbs USB: cp210x: use generic submit_read_urb at open USB: serial: clean up read processing in generic driver USB: serial: generalise generic read implementation USB: pl2303: switch to generic read implementation USB: serial: export generic throttle and unthrottle USB: ftdi_sio: switch to generic read implementation 2. write-kfifo USB: serial: clean up some error and debug messages in generic driver USB: serial: clean up generic write start busy test USB: pl2303: switch to generic write implementation USB: pl2303: use generic close 3. write-multi USB: usb_debug: use the generic kfifo-based write implementation USB: serial: allow custom multi-urb write bulk callbacks USB: serial: re-implement multi-urb writes in generic driver 4. write-prepare USB: serial: generalise write buffer preparation USB: ftdi_sio: fix some coding style issues USB: ftdi_sio: switch to generic write implementation USB: ftdi_sio: clean up SIO write support 5. example USB: aircable: rewrite using generic read and write implementations drivers/usb/serial/aircable.c | 498 ++++----------------------------------- drivers/usb/serial/cp210x.c | 16 +- drivers/usb/serial/ftdi_sio.c | 375 ++++-------------------------- drivers/usb/serial/generic.c | 280 +++++++++------------- drivers/usb/serial/pl2303.c | 421 ++------------------------------- drivers/usb/serial/usb-serial.c | 2 + drivers/usb/serial/usb_debug.c | 4 +- include/linux/usb/serial.h | 19 ++- 8 files changed, 254 insertions(+), 1361 deletions(-) -- 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
