On Sat, Jan 27, 2007 at 09:26:12PM +0100, Joris van Rantwijk wrote:
> Would it make sense to postpone setting up a new read URB until the
> tty layer has room available? It seems that some of the other drivers
> (digi_accelport for example) do this in response to a throttle message
> from the tty.
The attached patch does exactly this.
I tested it (single K6 processor, OHCI, full speed bulk transfers) and
it actually works under conditions that would otherwise cause data loss.
I added two fields to struct usb_serial_port to keep track of the
throttle state. Other usb-serial drivers typically use private data for
such things, but the generic driver can not really do that because some
of its code is also used by other drivers (which may have their own
private data needs).
As it is, I am not sure that this patch is useful in all scenarios.
It is certainly helpful for low-bandwidth devices that can hold their
data in response to throttling. But for devices that pump data in
real-time as fast as possible (webcam, A/D converter, etc), throttling
may actually cause more data loss.
There is an unrelated issue with the generic driver that worries me:
Received data is pushed to the tty layer in interrupt context (from the
urb callback). However, the tty_flip_buffer_push() code explicitly says
that it must not be called from interrupt context with the low_latency
flag set.
Is this wrong? Should we use a tasklet instead, like the cdc-acm driver?
Joris.
diff -ur -U5 linux-2.6.19.2-orig/drivers/usb/serial/generic.c
linux-2.6.19.2/drivers/usb/serial/generic.c
--- linux-2.6.19.2-orig/drivers/usb/serial/generic.c 2007-01-10
20:10:37.000000000 +0100
+++ linux-2.6.19.2/drivers/usb/serial/generic.c 2007-01-31 18:45:48.000000000
+0100
@@ -44,10 +44,12 @@
.num_interrupt_in = NUM_DONT_CARE,
.num_bulk_in = NUM_DONT_CARE,
.num_bulk_out = NUM_DONT_CARE,
.num_ports = 1,
.shutdown = usb_serial_generic_shutdown,
+ .throttle = usb_serial_generic_throttle,
+ .unthrottle = usb_serial_generic_unthrottle,
};
/* we want to look at all devices, as the vendor/product id can change
* depending on the command line argument */
static struct usb_device_id generic_serial_ids[] = {
@@ -108,20 +110,27 @@
int usb_serial_generic_open (struct usb_serial_port *port, struct file *filp)
{
struct usb_serial *serial = port->serial;
int result = 0;
+ unsigned long flags;
dbg("%s - port %d", __FUNCTION__, port->number);
/* force low_latency on so that our tty_push actually forces the data
through,
otherwise it is scheduled, and with high data rates (like with OHCI)
data
can get lost. */
if (port->tty)
port->tty->low_latency = 1;
- /* if we have a bulk interrupt, start reading from it */
+ /* clear the throttle flags */
+ spin_lock_irqsave(&port->lock, flags);
+ port->throttled = 0;
+ port->throttle_req = 0;
+ spin_unlock_irqrestore(&port->lock, flags);
+
+ /* if we have a bulk endpoint, start reading from it */
if (serial->num_bulk_in) {
/* Start reading from the device */
usb_fill_bulk_urb (port->read_urb, serial->dev,
usb_rcvbulkpipe(serial->dev,
port->bulk_in_endpointAddress),
port->read_urb->transfer_buffer,
@@ -246,35 +255,26 @@
dbg("%s - returns %d", __FUNCTION__, chars);
return (chars);
}
-void usb_serial_generic_read_bulk_callback (struct urb *urb)
+/* Push data to tty layer and resubmit the bulk read URB */
+static void flush_and_resubmit_read_urb (struct usb_serial_port *port)
{
- struct usb_serial_port *port = (struct usb_serial_port *)urb->context;
struct usb_serial *serial = port->serial;
- struct tty_struct *tty;
- unsigned char *data = urb->transfer_buffer;
+ struct urb *urb = port->read_urb;
+ struct tty_struct *tty = port->tty;
int result;
- dbg("%s - port %d", __FUNCTION__, port->number);
-
- if (urb->status) {
- dbg("%s - nonzero read bulk status received: %d", __FUNCTION__,
urb->status);
- return;
- }
-
- usb_serial_debug_data(debug, &port->dev, __FUNCTION__,
urb->actual_length, data);
-
- tty = port->tty;
+ /* Push data to tty */
if (tty && urb->actual_length) {
tty_buffer_request_room(tty, urb->actual_length);
- tty_insert_flip_string(tty, data, urb->actual_length);
- tty_flip_buffer_push(tty);
+ tty_insert_flip_string(tty, urb->transfer_buffer,
urb->actual_length);
+ tty_flip_buffer_push(tty); /* is this allowed from an URB
callback ? */
}
- /* Continue trying to always read */
+ /* Continue reading from device */
usb_fill_bulk_urb (port->read_urb, serial->dev,
usb_rcvbulkpipe (serial->dev,
port->bulk_in_endpointAddress),
port->read_urb->transfer_buffer,
port->read_urb->transfer_buffer_length,
@@ -283,10 +283,44 @@
usb_serial_generic_read_bulk_callback), port);
result = usb_submit_urb(port->read_urb, GFP_ATOMIC);
if (result)
dev_err(&port->dev, "%s - failed resubmitting read urb, error
%d\n", __FUNCTION__, result);
}
+
+void usb_serial_generic_read_bulk_callback (struct urb *urb)
+{
+ struct usb_serial_port *port = (struct usb_serial_port *)urb->context;
+ unsigned char *data = urb->transfer_buffer;
+ int is_throttled;
+ unsigned long flags;
+
+ dbg("%s - port %d", __FUNCTION__, port->number);
+
+ if (urb->status) {
+ dbg("%s - nonzero read bulk status received: %d", __FUNCTION__,
urb->status);
+ return;
+ }
+
+ usb_serial_debug_data(debug, &port->dev, __FUNCTION__,
urb->actual_length, data);
+
+ /* Throttle the device if requested by tty */
+ if (urb->actual_length) {
+ spin_lock_irqsave(&port->lock, flags);
+ is_throttled = port->throttled = port->throttle_req;
+ spin_unlock_irqrestore(&port->lock, flags);
+ if (is_throttled) {
+ /* Let the received data linger in the read URB;
+ * usb_serial_generic_unthrottle() will pick it
+ * up later. */
+ dbg("%s - throttling device", __FUNCTION__);
+ return;
+ }
+ }
+
+ /* Handle data and continue reading from device */
+ flush_and_resubmit_read_urb(port);
+}
EXPORT_SYMBOL_GPL(usb_serial_generic_read_bulk_callback);
void usb_serial_generic_write_bulk_callback (struct urb *urb)
{
struct usb_serial_port *port = (struct usb_serial_port *)urb->context;
@@ -301,10 +335,42 @@
usb_serial_port_softint(port);
}
EXPORT_SYMBOL_GPL(usb_serial_generic_write_bulk_callback);
+void usb_serial_generic_throttle (struct usb_serial_port *port)
+{
+ unsigned long flags;
+
+ dbg("%s - port %d", __FUNCTION__, port->number);
+
+ /* Set the throttle request flag. It will be picked up
+ * by usb_serial_generic_read_bulk_callback(). */
+ spin_lock_irqsave(&port->lock, flags);
+ port->throttle_req = 1;
+ spin_unlock_irqrestore(&port->lock, flags);
+}
+
+void usb_serial_generic_unthrottle (struct usb_serial_port *port)
+{
+ int was_throttled;
+ unsigned long flags;
+
+ dbg("%s - port %d", __FUNCTION__, port->number);
+
+ /* Clear the throttle flags */
+ spin_lock_irqsave(&port->lock, flags);
+ was_throttled = port->throttled;
+ port->throttled = port->throttle_req = 0;
+ spin_unlock_irqrestore(&port->lock, flags);
+
+ if (was_throttled) {
+ /* Handle pending data and resume reading from device */
+ flush_and_resubmit_read_urb(port);
+ }
+}
+
void usb_serial_generic_shutdown (struct usb_serial *serial)
{
int i;
dbg("%s", __FUNCTION__);
diff -ur -U5 linux-2.6.19.2-orig/include/linux/usb/serial.h
linux-2.6.19.2/include/linux/usb/serial.h
--- linux-2.6.19.2-orig/include/linux/usb/serial.h 2007-01-10
20:10:37.000000000 +0100
+++ linux-2.6.19.2/include/linux/usb/serial.h 2007-01-31 12:02:40.000000000
+0100
@@ -52,10 +52,12 @@
* @bulk_out_endpointAddress: endpoint address for the bulk out pipe for this
* port.
* @write_wait: a wait_queue_head_t used by the port.
* @work: work queue entry for the line discipline waking up.
* @open_count: number of times this port has been opened.
+ * @throttled: nonzero if the read urb is inactive to throttle the device
+ * @throttle_req: nonzero if the tty wants to throttle us
*
* This structure is used by the usb-serial core and drivers for the specific
* ports of a device.
*/
struct usb_serial_port {
@@ -86,10 +88,12 @@
__u8 bulk_out_endpointAddress;
wait_queue_head_t write_wait;
struct work_struct work;
int open_count;
+ char throttled;
+ char throttle_req;
struct device dev;
};
#define to_usb_serial_port(d) container_of(d, struct usb_serial_port, dev)
/* get and set the port private data pointer helper functions */
@@ -262,10 +266,12 @@
extern void usb_serial_generic_close (struct usb_serial_port *port, struct
file *filp);
extern int usb_serial_generic_write_room (struct usb_serial_port *port);
extern int usb_serial_generic_chars_in_buffer (struct usb_serial_port *port);
extern void usb_serial_generic_read_bulk_callback (struct urb *urb);
extern void usb_serial_generic_write_bulk_callback (struct urb *urb);
+extern void usb_serial_generic_throttle (struct usb_serial_port *port);
+extern void usb_serial_generic_unthrottle (struct usb_serial_port *port);
extern void usb_serial_generic_shutdown (struct usb_serial *serial);
extern int usb_serial_generic_register (int debug);
extern void usb_serial_generic_deregister (void);
extern int usb_serial_bus_register (struct usb_serial_driver *device);
-------------------------------------------------------------------------
Using Tomcat but need to do more? Need to support web services, security?
Get stuff done quickly with pre-integrated technology to make your job easier.
Download IBM WebSphere Application Server v.1.0.1 based on Apache Geronimo
http://sel.as-us.falkag.net/sel?cmd=lnk&kid=120709&bid=263057&dat=121642
_______________________________________________
[email protected]
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel
