> + /* send the data out the bulk port */
> + result = usb_submit_urb(port->write_urb, GFP_ATOMIC);
> + if (result) {
> + dev_err(&port->dev,
> + "%s - failed submitting write urb, error %d\n",
> + __func__, result);
> + /* don't have to grab the lock here, as we will
> + retry if != 0 */
> + port->write_urb_busy = 0;
> + status = result;
This looks deficient. If the first part of a transmission fails,
the fifo's remaining content should be discarded and if possible
an error returned to user space.
[..]
> @@ -487,8 +515,8 @@ void usb_serial_generic_write_bulk_callback(struct urb
> *urb) port->urbs_in_flight = 0;
> spin_unlock_irqrestore(&port->lock, flags);
> } else {
> - /* Handle the case for single urb mode */
> port->write_urb_busy = 0;
> + usb_serial_generic_write_start(port, 0);
This is a problem. This may fail due to a system suspend.
In that case you cannot depend on the next write restarting
IO. You need to restart IO in resume()
> @@ -970,6 +971,11 @@ int usb_serial_probe(struct usb_interface *interface,
> dev_err(&interface->dev, "No free urbs available\n");
> goto probe_error;
> }
> + port->write_fifo = kfifo_alloc(PAGE_SIZE, GFP_KERNEL,
> + &port->write_fifo_lock);
Whence do you take the fifo's size? What does this do to latency?
[..]
> @@ -96,6 +98,8 @@ struct usb_serial_port {
> unsigned char *bulk_out_buffer;
> int bulk_out_size;
> struct urb *write_urb;
> + struct kfifo *write_fifo;
> + spinlock_t write_fifo_lock;
Do you really need a separate lock?
Regards
Oliver
--
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
