On Fri, 3 Apr 2009, Hal Murray wrote: > It worked in 2.6.26.5. It's broken in 2.6.27.9 > > I'm slightly surprised it's still broken and/or I don't find more commentary > via google. It dies every time for me. Maybe my code is doing something > different. Maybe the garmin driver doesn't get used much. (I sure was happy > to find it when I wanted it a year or two ago.) > > I did find this: > http://bugs.archlinux.org/task/12079 > Which mostly confirms that there is a problem and suggests switching to > libusb. > > Here is what I've found: > > garmin_close does > mutex_lock(&port->serial->disc_mutex); > > then it (maybe) calls: > process_resetdev_request (in garmin_gps) > > that calls usb_reset_device > I haven't found where that locks the mutex. > > It works for me if I move the unlock to before that call. As I understand > it, that mutex is only to make sure multiple instances of open and close > don't trip over eachother. I'm not doing that. I don't know if I could > cause troubles if I tried to write some nasty code. > > > I don't know my way around this area. I was expecting all the serial drivers > to be similar and simple. The garmin driver has a lot of code that I > expected to be in user space. (I didn't study it carefully.) > > The pl2303 driver doesn't lock the mutex or call usb_reset_device. > > ftdi_close in ftdi_sio.c locks the mutex so it can disable flow control and > drop RTS and DTR. I don't see anything that looks tricky in there. It > doesn't call usb_reset_device. > > I can easily test things if somebody who understands this area wants to take > a look at it and propose a clean/official fix. Here's a patch that I'm planning to submit, back-ported to 2.6.27. It may fix your problem. Alan Stern Index: 2.6.27.19/drivers/usb/serial/usb-serial.c =================================================================== --- 2.6.27.19.orig/drivers/usb/serial/usb-serial.c +++ 2.6.27.19/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,16 +187,20 @@ 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 || 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; if (mutex_lock_interruptible(&port->mutex)) { retval = -ERESTARTSYS; - goto bailout_kref_put; + goto bailout_port_put; } ++port->port.count; @@ -248,7 +242,9 @@ bailout_mutex_unlock: tty->driver_data = NULL; port->port.tty = NULL; mutex_unlock(&port->mutex); -bailout_kref_put: +bailout_port_put: + put_device(&port->dev); +bailout_serial_put: usb_serial_put(serial); return retval; } @@ -256,6 +252,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; @@ -263,6 +262,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); @@ -273,7 +274,7 @@ static void serial_close(struct tty_stru if (port->port.count == 0) /* only call the device specific close if this * port is being closed by the last owner */ - port->serial->type->close(tty, port, filp); + serial->type->close(tty, port, filp); if (port->port.count == (port->console? 1 : 0)) { if (port->port.tty) { @@ -283,16 +284,22 @@ static void serial_close(struct tty_stru } } - if (port->port.count == 0) { - 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); + count = port->port.count; + mutex_unlock(&port->mutex); + 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); - mutex_unlock(&port->mutex); - usb_serial_put(port->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, @@ -544,7 +551,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); @@ -553,7 +566,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); } @@ -1037,17 +1049,21 @@ 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) { if (port->port.tty) tty_hangup(port->port.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
