Am Donnerstag, 24. September 2009 17:19:11 schrieb Johan Hovold:
> On Thu, Sep 24, 2009 at 03:21:25PM +0200, Oliver Neukum 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);
AFAICT BKL saves us here.
> > The completion handlers care only about an open or closing adapter
> > as we have no running URBs unless they have been submitted, but
> > drivers use tasklets which need a flag to safely cancel.
>
> Agree on open, but is such a flag for cancelling the tasklets always
> necessary?
Yes :-)
> --- a/drivers/usb/serial/ftdi_sio.c
> +++ b/drivers/usb/serial/ftdi_sio.c
> @@ -1787,12 +1787,12 @@ static void ftdi_close(struct usb_serial_port
> *port)
>
> dbg("%s", __func__);
>
> + /* shutdown our bulk read */
> + usb_kill_urb(port->read_urb);
And here's the window that screws things up. If you hit that window
the read_urb isn't submitted, usb_kill_urb() is a nop,
but after it has run,the tasklet can submit the urb again.
> /* cancel any scheduled reading */
> cancel_delayed_work_sync(&priv->rx_work);
You can, if you wish to hide what's going on, poison the urb. The
correct sequence is:
1. poison the urb
2. cancel_delayed_work_sync
3. unpoison the urb
or
3. free the urb
4. allocate a replacement
Step 1 sets a flag.
Regards
Oliver
--
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
