On Thu, 24 Sep 2009, Johan Hovold wrote:
> Speaking of that, isn't there a race in the current usb serial_open:
>
> /* Do the device-specific open only if the hardware isn't
> * already initialized.
> */
> if (!test_bit(ASYNCB_INITIALIZED, &port->port.flags)) {
> if (mutex_lock_interruptible(&port->mutex))
> return -ERESTARTSYS;
> mutex_lock(&serial->disc_mutex);
> if (serial->disconnected)
> retval = -ENODEV;
> else
> retval = port->serial->type->open(tty, port);
> mutex_unlock(&serial->disc_mutex);
> mutex_unlock(&port->mutex);
> if (retval)
> return retval;
> set_bit(ASYNCB_INITIALIZED, &port->port.flags);
> }
>
> Shouldn't it be something more along the lines of:
>
> if (mutex_lock_interruptible(&port->mutex))
> return -ERESTARTSYS;
> if (!test_bit(ASYNCB_INITIALIZED, &port->port.flags)) {
> mutex_lock(&serial->disc_mutex);
> if (serial->disconnected)
> retval = -ENODEV;
> else
> retval = port->serial->type->open(tty, port);
> mutex_unlock(&serial->disc_mutex);
> if (retval) {
> mutex_unlock(&port->mutex);
> return retval;
> }
> set_bit(ASYNCB_INITIALIZED, &port->port.flags);
> }
> mutex_unlock(&port->mutex);
In theory, at least, this should be serialized by the TTY core. In
practice it probably isn't, since the core relies so heavily on the
BKL. Still, I would prefer to fix the core rather than change
usb-serial.
Locking in the TTY layer is a mess. Open races with just about
everything else, and in addition hangup races with close. Not to
mention races involving the I/O and line discipline paths or the serial
attributes (baud rate and so on). Until this is fixed properly,
fiddling around with lower-level drivers will be of very limited use.
Alan Stern
--
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
