Hi,
as this driver has a certain example function, I've gone through it with
a fine comb. Here are the bugs with extensive description.
- conformance to coding style
- an URB's status must be read before it is declared non busy
otherwise the URB may have been submitted again and you are
reading a false status.
- a dangerous spinlock bug leading to deadlocks
If you take a lock in task context, bottom halves and interrupt context,
you must always (except in interrupt context) switch off interrupts, even
if the data touched in task context is touched only in bottom halves.
Otherwise the kernel may deadlock.
Regards
Oliver
Signed-off-by: Oliver Neukum <[EMAIL PROTECTED]>
--
--- a/drivers/usb/serial/generic.c 2007-03-29 14:13:19.000000000 +0200
+++ b/drivers/usb/serial/generic.c 2007-03-29 14:13:29.000000000 +0200
@@ -82,7 +82,7 @@
}
#endif
-int usb_serial_generic_register (int _debug)
+int usb_serial_generic_register(int _debug)
{
int retval = 0;
@@ -93,7 +93,7 @@
generic_device_ids[0].match_flags = USB_DEVICE_ID_MATCH_VENDOR |
USB_DEVICE_ID_MATCH_PRODUCT;
/* register our generic driver with ourselves */
- retval = usb_serial_register (&usb_serial_generic_device);
+ retval = usb_serial_register(&usb_serial_generic_device);
if (retval)
goto exit;
retval = usb_register(&generic_driver);
@@ -113,11 +113,10 @@
#endif
}
-int usb_serial_generic_open (struct usb_serial_port *port, struct file *filp)
+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);
@@ -128,15 +127,15 @@
port->tty->low_latency = 1;
/* clear the throttle flags */
- spin_lock_irqsave(&port->lock, flags);
+ spin_lock_irq(&port->lock);
port->throttled = 0;
port->throttle_req = 0;
- spin_unlock_irqrestore(&port->lock, flags);
+ spin_unlock_irq(&port->lock);
/* 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_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,
@@ -153,7 +152,7 @@
}
EXPORT_SYMBOL_GPL(usb_serial_generic_open);
-static void generic_cleanup (struct usb_serial_port *port)
+static void generic_cleanup(struct usb_serial_port *port)
{
struct usb_serial *serial = port->serial;
@@ -171,12 +170,13 @@
void usb_serial_generic_close (struct usb_serial_port *port, struct file *
filp)
{
dbg("%s - port %d", __FUNCTION__, port->number);
- generic_cleanup (port);
+ generic_cleanup(port);
}
int usb_serial_generic_write(struct usb_serial_port *port, const unsigned char
*buf, int count)
{
struct usb_serial *serial = port->serial;
+ unsigned long flags;
int result;
unsigned char *data;
@@ -184,23 +184,23 @@
if (count == 0) {
dbg("%s - write request of 0 bytes", __FUNCTION__);
- return (0);
+ return 0;
}
/* only do something if we have a bulk out endpoint */
if (serial->num_bulk_out) {
- spin_lock_bh(&port->lock);
+ spin_lock_irqsave(&port->lock, flags);
if (port->write_urb_busy) {
- spin_unlock_bh(&port->lock);
+ spin_unlock_irqrestore(&port->lock, flags);
dbg("%s - already writing", __FUNCTION__);
return 0;
}
port->write_urb_busy = 1;
- spin_unlock_bh(&port->lock);
+ spin_unlock_irqrestore(&port->lock, flags);
count = (count > port->bulk_out_size) ? port->bulk_out_size :
count;
- memcpy (port->write_urb->transfer_buffer, buf, count);
+ memcpy(port->write_urb->transfer_buffer, buf, count);
data = port->write_urb->transfer_buffer;
usb_serial_debug_data(debug, &port->dev, __FUNCTION__, count,
data);
@@ -214,7 +214,6 @@
usb_serial_generic_write_bulk_callback),
port);
/* send the data out the bulk port */
- port->write_urb_busy = 1;
result = usb_submit_urb(port->write_urb, GFP_ATOMIC);
if (result) {
dev_err(&port->dev, "%s - failed submitting write urb,
error %d\n", __FUNCTION__, result);
@@ -230,7 +229,7 @@
return 0;
}
-int usb_serial_generic_write_room (struct usb_serial_port *port)
+int usb_serial_generic_write_room(struct usb_serial_port *port)
{
struct usb_serial *serial = port->serial;
int room = 0;
@@ -243,7 +242,7 @@
}
dbg("%s - returns %d", __FUNCTION__, room);
- return (room);
+ return room;
}
int usb_serial_generic_chars_in_buffer (struct usb_serial_port *port)
@@ -259,7 +258,7 @@
}
dbg("%s - returns %d", __FUNCTION__, chars);
- return (chars);
+ return chars;
}
/* Push data to tty layer and resubmit the bulk read URB */
@@ -278,7 +277,7 @@
}
/* Continue reading from device */
- usb_fill_bulk_urb (port->read_urb, serial->dev,
+ usb_fill_bulk_urb(port->read_urb, serial->dev,
usb_rcvbulkpipe (serial->dev,
port->bulk_in_endpointAddress),
port->read_urb->transfer_buffer,
@@ -296,7 +295,6 @@
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);
@@ -309,9 +307,9 @@
/* Throttle the device if requested by tty */
if (urb->actual_length) {
- spin_lock_irqsave(&port->lock, flags);
+ spin_lock(&port->lock);
is_throttled = port->throttled = port->throttle_req;
- spin_unlock_irqrestore(&port->lock, flags);
+ spin_unlock(&port->lock);
if (is_throttled) {
/* Let the received data linger in the read URB;
* usb_serial_generic_unthrottle() will pick it
@@ -332,12 +330,12 @@
dbg("%s - port %d", __FUNCTION__, port->number);
- port->write_urb_busy = 0;
if (urb->status) {
dbg("%s - nonzero write bulk status received: %d",
__FUNCTION__, urb->status);
return;
}
-
+ smp_mb(); /* we are busy until we have read our status */
+ port->write_urb_busy = 0;
usb_serial_port_softint(port);
}
EXPORT_SYMBOL_GPL(usb_serial_generic_write_bulk_callback);
-------------------------------------------------------------------------
Take Surveys. Earn Cash. Influence the Future of IT
Join SourceForge.net's Techsay panel and you'll get the chance to share your
opinions on IT & business topics through brief surveys-and earn cash
http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV
_______________________________________________
linux-usb-devel@lists.sourceforge.net
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel
