On Sun, 5 Apr 2009, Oliver Neukum wrote:
> Am Sonntag 05 April 2009 16:40:03 schrieb Alan Stern:
> > On Sat, 4 Apr 2009, Oliver Neukum wrote:
> > > > In any case, shouldn't the generic layer contain pre_reset and
> > > > post_reset routines? And shouldn't these routines be listed in the
> > > > usb_driver structure for each of the subdrivers?
> > >
> > > What could generic methods do?
> >
> > Not much, I guess. Check whether the device file is open and call
> > kill_traffic() if it is? But I'm not familiar with all the ins and
> > outs of these drivers.
>
> We couldn't limit this to the case of an opened interface,
> as some drivers schedule interrupt URBs always and we'd
> need a way to signal drivers why traffic is killed, lest they
> start error handling procedures.
All right, so the subdrivers should have their own pre_reset and
post_reset methods.
But that's not the answer to the problem in garmin_gps. Along with
everything else, it calls usb_reset_device() without holding the
required lock. It would be best to have the original author take a
look at the problem and figure out how to fix it properly.
> > BTW, I'm concerned about that usb-serial.c patch posted earlier. In
> > particular, does serial_open() hold disc_mutex long enough? What
> > happens if a disconnect occurs while serial_open() is waiting to
> > acquire port->mutex? Do you think the calls to
> > usb_autopm_get_interface() and serial->type->open() should also be
> > protected by disc_mutex?
>
> Yes. usb_autopm_get_interface() must be protected or the counters get out of
> sync.
Here's an updated version of that patch. See what you think.
Alan Stern
Index: usb-2.6/drivers/usb/serial/usb-serial.c
===================================================================
--- usb-2.6.orig/drivers/usb/serial/usb-serial.c
+++ usb-2.6/drivers/usb/serial/usb-serial.c
@@ -142,16 +142,6 @@ static void destroy_serial(struct kref *
if (serial->minor != SERIAL_TTY_NO_MINOR)
return_serial(serial);
- for (i = 0; i < serial->num_ports; ++i)
- serial->port[i]->port.count = 0;
-
- /* the ports are cleaned up and released in port_release() */
- for (i = 0; i < serial->num_ports; ++i)
- if (serial->port[i]->dev.parent != NULL) {
- device_unregister(&serial->port[i]->dev);
- serial->port[i] = NULL;
- }
-
/* If this is a "fake" port, we have to clean it up here, as it will
* not get cleaned up in port_release() as it was never registered with
* the driver core */
@@ -186,7 +176,7 @@ static int serial_open (struct tty_struc
struct usb_serial *serial;
struct usb_serial_port *port;
unsigned int portNumber;
- int retval;
+ int retval = 0;
dbg("%s", __func__);
@@ -197,21 +187,23 @@ static int serial_open (struct tty_struc
return -ENODEV;
}
+ mutex_lock(&serial->disc_mutex);
portNumber = tty->index - serial->minor;
port = serial->port[portNumber];
- if (!port) {
+ if (!port || serial->disconnected)
retval = -ENODEV;
- goto bailout_kref_put;
- }
-
- if (port->serial->disconnected) {
- retval = -ENODEV;
- goto bailout_kref_put;
- }
+ else
+ get_device(&port->dev);
+ mutex_unlock(&serial->disc_mutex);
+ if (retval)
+ goto bailout_serial_put;
+ /* Note: locking order requirement forces port->mutex to be
+ * acquired while serial->disc_mutex is not held.
+ */
if (mutex_lock_interruptible(&port->mutex)) {
retval = -ERESTARTSYS;
- goto bailout_kref_put;
+ goto bailout_port_put;
}
++port->port.count;
@@ -231,9 +223,15 @@ static int serial_open (struct tty_struc
goto bailout_mutex_unlock;
}
- retval = usb_autopm_get_interface(serial->interface);
+ mutex_lock(&serial->disc_mutex);
+ if (serial->disconnected)
+ retval = -ENODEV;
+ else
+ retval = usb_autopm_get_interface(serial->interface);
+ mutex_unlock(&serial->disc_mutex);
if (retval)
goto bailout_module_put;
+
/* only call the device specific open if this
* is the first time the port is opened */
retval = serial->type->open(tty, port, filp);
@@ -253,7 +251,9 @@ bailout_mutex_unlock:
tty->driver_data = NULL;
tty_port_tty_set(&port->port, NULL);
mutex_unlock(&port->mutex);
-bailout_kref_put:
+bailout_port_put:
+ put_device(&port->dev);
+bailout_serial_put:
usb_serial_put(serial);
return retval;
}
@@ -261,6 +261,9 @@ bailout_kref_put:
static void serial_close(struct tty_struct *tty, struct file *filp)
{
struct usb_serial_port *port = tty->driver_data;
+ struct usb_serial *serial;
+ struct module *owner;
+ int count;
if (!port)
return;
@@ -268,6 +271,8 @@ static void serial_close(struct tty_stru
dbg("%s - port %d", __func__, port->number);
mutex_lock(&port->mutex);
+ serial = port->serial;
+ owner = serial->type->driver.owner;
if (port->port.count == 0) {
mutex_unlock(&port->mutex);
@@ -280,7 +285,7 @@ static void serial_close(struct tty_stru
* this before we drop the port count. The call is protected
* by the port mutex
*/
- port->serial->type->close(tty, port, filp);
+ serial->type->close(tty, port, filp);
if (port->port.count == (port->console ? 2 : 1)) {
struct tty_struct *tty = tty_port_tty_get(&port->port);
@@ -294,17 +299,23 @@ static void serial_close(struct tty_stru
}
}
- if (port->port.count == 1) {
- mutex_lock(&port->serial->disc_mutex);
- if (!port->serial->disconnected)
- usb_autopm_put_interface(port->serial->interface);
- mutex_unlock(&port->serial->disc_mutex);
- module_put(port->serial->type->driver.owner);
- }
--port->port.count;
-
+ count = port->port.count;
mutex_unlock(&port->mutex);
- usb_serial_put(port->serial);
+ put_device(&port->dev);
+
+ /* Mustn't dereference port any more */
+ if (count == 0) {
+ mutex_lock(&serial->disc_mutex);
+ if (!serial->disconnected)
+ usb_autopm_put_interface(serial->interface);
+ mutex_unlock(&serial->disc_mutex);
+ }
+ usb_serial_put(serial);
+
+ /* Mustn't dereference serial any more */
+ if (count == 0)
+ module_put(owner);
}
static int serial_write(struct tty_struct *tty, const unsigned char *buf,
@@ -553,7 +564,13 @@ static void kill_traffic(struct usb_seri
static void port_free(struct usb_serial_port *port)
{
+ /*
+ * Stop all the traffic before cancelling the work, so that
+ * nobody will restart it by calling usb_serial_port_softint.
+ */
kill_traffic(port);
+ cancel_work_sync(&port->work);
+
usb_free_urb(port->read_urb);
usb_free_urb(port->write_urb);
usb_free_urb(port->interrupt_in_urb);
@@ -562,7 +579,6 @@ static void port_free(struct usb_serial_
kfree(port->bulk_out_buffer);
kfree(port->interrupt_in_buffer);
kfree(port->interrupt_out_buffer);
- flush_scheduled_work(); /* port->work */
kfree(port);
}
@@ -1047,6 +1063,8 @@ void usb_serial_disconnect(struct usb_in
usb_set_intfdata(interface, NULL);
/* must set a flag, to signal subdrivers */
serial->disconnected = 1;
+ mutex_unlock(&serial->disc_mutex);
+
for (i = 0; i < serial->num_ports; ++i) {
port = serial->port[i];
if (port) {
@@ -1056,11 +1074,13 @@ void usb_serial_disconnect(struct usb_in
tty_kref_put(tty);
}
kill_traffic(port);
+ cancel_work_sync(&port->work);
+ device_unregister(&port->dev);
+ serial->port[i] = NULL;
}
}
/* let the last holder of this object
* cause it to be cleaned up */
- mutex_unlock(&serial->disc_mutex);
usb_serial_put(serial);
dev_info(dev, "device disconnected\n");
}
--
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
