Hi,
this is John's patch for fixing the last disconnect/IO races in usb-serial.
I ported it to Greg's current tree.
To explain how it works, I quote him:
----------------------------
This is a fix for race condition in usb-serial.c
between serial_open and usb_serial_disconnect
(kernel version 2.6.21-rc3)
Prior to this patch instances of usb_serial were not
ref-counted consistently, which caused serial_open
to reuse already deleted instance.
For more details search Web for
"Possible race condition in usb-serial.c"
discussion in Dec 2006.
The present patch ensures that instances
of usb_serial are ref-counted consistently
by calling kref_get in get_free_serial
and usb_serial_put in return_serial.
Another problem was that the code prior to
this patch called serial->type->shutdown(serial)
from destroy_serial, which may execute after
usb_serial_disconnect (if the serial device was opened).
This was an error because no IO should be done
on a USB device after usb_serial_disconnect returns.
To fix that the present patch moved call to
serial->type->shutdown(serial)
from destroy_serial to usb_serial_disconnect.
Also it introduces functions
serial_lock, serial_unlock and
serial_lock_and_wait_before_shutdown.
serial_lock and serial_unlock are called
from each serial_* routine and
serial_lock_and_wait_before_shutdown
is called from usb_serial_disconnect
to ensure that all serial_* calls exit before
usb_serial_disconnect calls
serial->type->shutdown(serial),
Signed-off-by: John Green <[EMAIL PROTECTED]>
Signed-off-by: Oliver Neukum <[EMAIL PROTECTED]>
--
--- a/drivers/usb/serial/usb-serial.c 2007-05-03 11:12:01.000000000 +0200
+++ b/drivers/usb/serial/usb-serial.c 2007-05-03 12:26:19.000000000 +0200
@@ -61,9 +61,41 @@ static struct usb_driver usb_serial_driv
static int debug;
static struct usb_serial *serial_table[SERIAL_TTY_MINORS]; /* initially
all NULL */
-static spinlock_t table_lock;
+static unsigned long ports_reserved[SERIAL_TTY_MINORS/BITS_PER_LONG];
+static spinlock_t table_lock; /* protects serial_table and ports_reserved */
static LIST_HEAD(usb_serial_driver_list);
+/*
+ usb_serial objects are ref counted. This is necessary because pointers
to an object
+ may be stored in several places. Each time a pointer is duplicated, the
ref count
+ should be incremented. This done using kref_get(&serial->kref). When
pointer is
+ destroyed, the count should be decremented using usb_serial_put(serial).
+ A typical life cycle of usb_serial:
+
+ 1. usb_serial_probe creates usb_serial (ref count set to 1) and stores
the pointer
+ in the usb_interface using usb_set_intfdata.
+ 2. usb_serial_probe calls get_free_serial, which copies the pointer one
or more times
+ to serial_table indexed by TTY minor index (ref count is incremeted
for each copy).
+ 3. serial_open takes this pointer from serial_table (ref count
incremented) and passes it to TTY
+ (this is done indirectly: tty->driver_data = port, but port->serial
= serial).
+ 4. serial_close decrements the ref count (because tty object is
destroyed).
+ 5. usb_serial_disconnect calls return_serial, which erases all pointers
to the given serial
+ from serial_table (decrementing the ref count).
+ 6. usb_serial_disconnect destroys the pointer, which was stored in
usb_interface
+ (decrementing the ref count).
+ At this point the ref count normally reaches 0 and the object
destroyed.
+ Alternatively, serial_close may be called after usb_serial_disconnect
and the object destroyed
+ at that time.
+
+ ports_reserved is an array of bits parallel to usb_serial.
+ get_free_serial uses this array to locate a range of TTY devices, which
are not yet registered.
+ Then get_free_serial reserves the found range by setting bits.
+ After that usb_serial_probe registers TTY devices in this range using
device_register.
+ A bit is cleared by destroy_serial after it unregisters devices using
device_unregister.
+*/
+
+void usb_serial_put(struct usb_serial *serial);
+
struct usb_serial *usb_serial_get_by_index(unsigned index)
{
struct usb_serial *serial;
@@ -92,7 +124,7 @@ static struct usb_serial *get_free_seria
good_spot = 1;
for (j = 1; j <= num_ports-1; ++j)
- if ((i+j >= SERIAL_TTY_MINORS) || (serial_table[i+j])) {
+ if ((i+j >= SERIAL_TTY_MINORS) || (test_bit(i+j,
ports_reserved))) {
good_spot = 0;
i += j;
break;
@@ -104,7 +136,9 @@ static struct usb_serial *get_free_seria
j = 0;
dbg("%s - minor base = %d", __FUNCTION__, *minor);
for (i = *minor; (i < (*minor + num_ports)) && (i <
SERIAL_TTY_MINORS); ++i) {
+ kref_get(&serial->kref);
serial_table[i] = serial;
+ __set_bit(i, ports_reserved);
serial->port[j++]->number = i;
}
spin_unlock(&table_lock);
@@ -126,10 +160,82 @@ static void return_serial(struct usb_ser
spin_lock(&table_lock);
for (i = 0; i < serial->num_ports; ++i) {
serial_table[serial->minor + i] = NULL;
+ usb_serial_put(serial);
}
spin_unlock(&table_lock);
}
+static int usb_serial_check_reserved(unsigned index)
+{
+ int ret;
+
+ spin_lock(&table_lock);
+ ret = test_bit(index, ports_reserved);
+ spin_unlock(&table_lock);
+ return ret;
+}
+
+static void usb_serial_unreserve(unsigned index, unsigned num_ports)
+{
+ int i;
+
+ spin_lock(&table_lock);
+ for (i = 0; i < num_ports; ++i, ++index) {
+ __clear_bit(index, ports_reserved);
+ }
+ spin_unlock(&table_lock);
+}
+
+/*
+ * serial_lock expects a valid usb_serial * serial pointer.
+ * serial_lock returns 0 if usb_serial is still active and the caller can use
it.
+ * In this case the caller must call serial_unlock.
+ * serial_lock returns -ENODEV if disconnection routine was already called on
the device.
+ * In this case the caller should not perform any IO operations, but
should return an error code.
+ */
+int serial_lock(struct usb_serial * serial)
+{
+ int ret;
+ unsigned long flags;
+
+ spin_lock_irqsave(&serial->lock, flags);
+ if (serial->shutdown_called) {
+ ret = -ENODEV;
+ } else {
+ serial->lock_count++;
+ ret = 0;
+ }
+ spin_unlock_irqrestore(&serial->lock, flags);
+ return ret;
+}
+
+void serial_unlock(struct usb_serial * serial)
+{
+ int wakeup = 0;
+ unsigned long flags;
+
+ spin_lock_irqsave(&serial->lock, flags);
+ serial->lock_count--;
+ if (serial->shutdown_called && serial->lock_count == 0) {
+ wakeup = 1;
+ }
+ spin_unlock_irqrestore(&serial->lock, flags);
+
+ if(wakeup) {
+ wake_up(&serial->shutdown_wait);
+ }
+}
+
+void serial_lock_and_wait_before_shutdown(struct usb_serial * serial)
+{
+ unsigned long flags;
+
+ spin_lock_irqsave(&serial->lock, flags);
+ serial->shutdown_called = 1;
+ spin_unlock_irqrestore(&serial->lock, flags);
+ wait_event(serial->shutdown_wait, serial->lock_count == 0);
+}
+
static void destroy_serial(struct kref *kref)
{
struct usb_serial *serial;
@@ -140,10 +246,8 @@ static void destroy_serial(struct kref *
dbg("%s - %s", __FUNCTION__, serial->type->description);
- serial->type->shutdown(serial);
-
/* return the minor range that this device had */
- return_serial(serial);
+ usb_serial_unreserve(serial->minor, serial->num_ports);
for (i = 0; i < serial->num_ports; ++i)
serial->port[i]->open_count = 0;
@@ -193,7 +297,11 @@ static int serial_open (struct tty_struc
/* get the serial object associated with this tty pointer */
serial = usb_serial_get_by_index(tty->index);
if (!serial) {
- tty->driver_data = NULL;
+ /* There may be other files open for this port after device
disconnection.
+ in this case don't erase tty->driver_data */
+ if (!usb_serial_check_reserved(tty->index)) {
+ tty->driver_data = NULL;
+ }
return -ENODEV;
}
@@ -204,9 +312,13 @@ static int serial_open (struct tty_struc
goto bailout_kref_put;
}
+ retval = serial_lock(serial);
+ if (retval)
+ goto bailout_kref_put;
+
if (mutex_lock_interruptible(&port->mutex)) {
retval = -ERESTARTSYS;
- goto bailout_kref_put;
+ goto bailout_kref_put_unlock;
}
++port->open_count;
@@ -234,6 +346,7 @@ static int serial_open (struct tty_struc
}
mutex_unlock(&port->mutex);
+ serial_unlock(serial);
return 0;
bailout_module_put:
@@ -243,6 +356,8 @@ bailout_mutex_unlock:
tty->driver_data = NULL;
port->tty = NULL;
mutex_unlock(&port->mutex);
+bailout_kref_put_unlock:
+ serial_unlock(serial);
bailout_kref_put:
usb_serial_put(serial);
return retval;
@@ -299,8 +414,13 @@ static int serial_write (struct tty_stru
goto exit;
}
+ retval = serial_lock(port->serial);
+ if (retval)
+ goto exit;
+
/* pass on to the driver specific version of this function */
retval = port->serial->type->write(port, buf, count);
+ serial_unlock(port->serial);
exit:
return retval;
@@ -321,9 +441,12 @@ static int serial_write_room (struct tty
goto exit;
}
+ retval = serial_lock(port->serial);
+ if (retval)
+ goto exit;
/* pass on to the driver specific version of this function */
retval = port->serial->type->write_room(port);
-
+ serial_unlock(port->serial);
exit:
return retval;
}
@@ -343,8 +466,12 @@ static int serial_chars_in_buffer (struc
goto exit;
}
+ retval = serial_lock(port->serial);
+ if (retval)
+ goto exit;
/* pass on to the driver specific version of this function */
retval = port->serial->type->chars_in_buffer(port);
+ serial_unlock(port->serial);
exit:
return retval;
@@ -364,9 +491,13 @@ static void serial_throttle (struct tty_
return;
}
+ if(serial_lock(port->serial))
+ return;
/* pass on to the driver specific version of this function */
if (port->serial->type->throttle)
port->serial->type->throttle(port);
+ serial_unlock(port->serial);
+
}
static void serial_unthrottle (struct tty_struct * tty)
@@ -383,9 +514,13 @@ static void serial_unthrottle (struct tt
return;
}
+ if(serial_lock(port->serial))
+ return;
/* pass on to the driver specific version of this function */
if (port->serial->type->unthrottle)
port->serial->type->unthrottle(port);
+ serial_unlock(port->serial);
+
}
static int serial_ioctl (struct tty_struct *tty, struct file * file, unsigned
int cmd, unsigned long arg)
@@ -403,11 +538,16 @@ static int serial_ioctl (struct tty_stru
goto exit;
}
+ retval = serial_lock(port->serial);
+ if (retval)
+ goto exit;
/* pass on to the driver specific version of this function if it is
available */
if (port->serial->type->ioctl)
retval = port->serial->type->ioctl(port, file, cmd, arg);
else
retval = -ENOIOCTLCMD;
+ serial_unlock(port->serial);
+
exit:
return retval;
@@ -427,9 +567,13 @@ static void serial_set_termios (struct t
return;
}
+ if(serial_lock(port->serial))
+ return;
/* pass on to the driver specific version of this function if it is
available */
if (port->serial->type->set_termios)
port->serial->type->set_termios(port, old);
+ serial_unlock(port->serial);
+
}
static void serial_break (struct tty_struct *tty, int break_state)
@@ -446,9 +590,12 @@ static void serial_break (struct tty_str
return;
}
+ if(serial_lock(port->serial))
+ return;
/* pass on to the driver specific version of this function if it is
available */
if (port->serial->type->break_ctl)
port->serial->type->break_ctl(port, break_state);
+ serial_unlock(port->serial);
}
static int serial_read_proc (char *page, char **start, off_t off, int count,
int *eof, void *data)
@@ -465,6 +612,8 @@ static int serial_read_proc (char *page,
serial = usb_serial_get_by_index(i);
if (serial == NULL)
continue;
+ if(serial_lock(serial))
+ continue;
length += sprintf (page+length, "%d:", i);
if (serial->type->driver.owner)
@@ -481,6 +630,7 @@ static int serial_read_proc (char *page,
length += sprintf (page+length, "\n");
if ((length + begin) > (off + count)) {
+ serial_unlock(serial);
usb_serial_put(serial);
goto done;
}
@@ -488,6 +638,7 @@ static int serial_read_proc (char *page,
begin += length;
length = 0;
}
+ serial_unlock(serial);
usb_serial_put(serial);
}
*eof = 1;
@@ -501,6 +652,7 @@ done:
static int serial_tiocmget (struct tty_struct *tty, struct file *file)
{
struct usb_serial_port *port = tty->driver_data;
+ int retval;
if (!port)
return -ENODEV;
@@ -512,16 +664,23 @@ static int serial_tiocmget (struct tty_s
return -ENODEV;
}
+ retval = serial_lock(port->serial);
+ if(retval)
+ goto exit;
if (port->serial->type->tiocmget)
- return port->serial->type->tiocmget(port, file);
-
- return -EINVAL;
+ retval = port->serial->type->tiocmget(port, file);
+ else
+ retval = -EINVAL;
+ serial_unlock(port->serial);
+exit:
+ return retval;
}
static int serial_tiocmset (struct tty_struct *tty, struct file *file,
unsigned int set, unsigned int clear)
{
struct usb_serial_port *port = tty->driver_data;
+ int retval;
if (!port)
return -ENODEV;
@@ -533,10 +692,17 @@ static int serial_tiocmset (struct tty_s
return -ENODEV;
}
+ retval = serial_lock(port->serial);
+ if (retval)
+ goto exit;
if (port->serial->type->tiocmset)
- return port->serial->type->tiocmset(port, file, set, clear);
+ retval = port->serial->type->tiocmset(port, file, set, clear);
+ else
+ retval = -EINVAL;
+ serial_unlock(port->serial);
- return -EINVAL;
+exit:
+ return retval;
}
/*
@@ -614,6 +780,9 @@ static struct usb_serial * create_serial
serial->interface = interface;
kref_init(&serial->kref);
+ spin_lock_init(&serial->lock);
+ init_waitqueue_head(&serial->shutdown_wait);
+
return serial;
}
@@ -1057,6 +1226,9 @@ void usb_serial_disconnect(struct usb_in
usb_set_intfdata (interface, NULL);
if (serial) {
+
+ return_serial(serial); /* erase pointers to serial from
serial_table */
+
for (i = 0; i < serial->num_ports; ++i) {
port = serial->port[i];
if (port) {
@@ -1065,8 +1237,20 @@ void usb_serial_disconnect(struct usb_in
kill_traffic(port);
}
}
- /* let the last holder of this object
- * cause it to be cleaned up */
+
+ /* The following call will block if any serial_* routine (on
our serial)
+ * is still in progress. It will wait for all such routines to
exit.
+ * After that all further calls to serial_* routines will
return immediately
+ * without performing any IO or touching the content of serial
or port.
+ * Therefore, we can cleanup serial and ports.
+ */
+ serial_lock_and_wait_before_shutdown(serial);
+ serial->type->shutdown(serial);
+
+ /* Now we destroy the pointer to serial, which was associated
with usb_interface.
+ * This is likely to delete serial object, unless some TTY
files are still open.
+ * In that case the object will be deleted from serial_close.
+ */
usb_serial_put(serial);
}
dev_info(dev, "device disconnected\n");
--- a/include/linux/usb/serial.h 2007-03-23 15:51:38.610525100 -0800
+++ b/include/linux/usb/serial.h 2007-03-23 17:16:58.524274200 -0800
@@ -119,6 +119,11 @@ static inline void usb_set_serial_port_d
* @num_bulk_in: number of bulk in endpoints we have
* @num_bulk_out: number of bulk out endpoints we have
* @port: array of struct usb_serial_port structures for the different ports.
+ * @kref: refcount for storing the number of pointers to usb_serial
+ * @lock: spinlock to protect access to lock_count and shutdown_called
+ * @lock_count: number of serial_* routines, which locked usb_serial and are
using it.
+ * @shutdown_called: If this flag is set, serial_* routines must not use other
fields of usb_serial.
+ * @shutdown_wait: used by usb_serial_disconnect to block until all serial_*
routines exit.
* @private: place to put any driver specific information that is needed. The
* usb-serial driver is required to manage this data, the usb-serial core
* will not touch this. Use usb_get_serial_data() and
@@ -136,7 +141,11 @@ struct usb_serial {
char num_bulk_in;
char num_bulk_out;
struct usb_serial_port * port[MAX_NUM_PORTS];
- struct kref kref;
+ struct kref kref;
+ spinlock_t lock;
+ int lock_count;
+ int shutdown_called;
+ wait_queue_head_t shutdown_wait;
void * private;
};
#define to_usb_serial(d) container_of(d, struct usb_serial, kref)
-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/
_______________________________________________
linux-usb-devel@lists.sourceforge.net
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel
