Messages in this thread | | | Date | Fri, 21 Aug 2009 16:52:28 -0400 (EDT) | From | Alan Stern <> | Subject | Re: 2.6.31-rc5 regression: Oops when USB Serial disconnected while in use |
| |
On Fri, 21 Aug 2009, Alan Cox wrote:
> > serial_open() /* first user */ > > tty_port_block_til_ready() /* returns immediately */ > > serial->type->open() /* initializes the hardware */ > > > > serial_open() /* second user */ > > tty_port_block_til_ready() /* blocks */ > > If the first user succeeded then second should open immediately as the use > count is >= 1 already. It is re-opening the open device, unless the > hangup occurs first.
What about protecting the use counter? In tty_port.c it's always protected by port->lock, but not in serial_open(). Is that a mistake?
How are hangups synchronized with opens? Do you rely on the BKL? Suppose a hangup occurs, and do_tty_hangup() marks all the existing file references with hung_up_tty_fops. But before it gets around to calling tty->ops->hangup(), another open occurs. I can't imagine the BKL will prevent this; do_tty_hangup() is so complex it must sleep somewhere. Thus it's possible for __tty_open() to call tty->ops->open() before tty->ops->hangup() is called, which means the open will succeed.
The when the driver's hangup routine finally gets around to calling tty_port_hangup(), port->count will be set back to 0. So now we've got an uncounted open file.
> Most drivers tend to look like > > open > test ASYNC_INITIALIZED > init hardware > [either in full or clean up partial] > set ASYNC_INITIALIZED) > any other alloc/counter magic > tty->private_data = my stuff
tty->driver_data, right?
> block_til_ready > return ok/error > > close > if (hung_up) > return > if (tty->driver_data == NULL) > return > counts
Is "counts" shorthand for:
if (tty_port_close_start(...) == 0) return ?
> clean up resources > if (last && test_clear INITIALIZED)
How do you check for "last"? Doesn't the fact that we are here mean that there are no remaining open references?
> deinit-hardware > return ok/error > > hangup > if (initialized & test_clear INITIALIZED) {
What is "initialized" supposed to be? Isn't INITIALIZED enough?
> deinit hardware > } > > which is where I was trying to get the USB code.
It doesn't look as though we're too far away. Thanks for the detailed explanations.
Alan Stern
P.S.: Consider this code in tty_port_block_til_ready():
/* if non-blocking mode is set we can pass directly to open unless the port has just hung up or is in another error state */ if ((filp->f_flags & O_NONBLOCK) || (tty->flags & (1 << TTY_IO_ERROR))) { port->flags |= ASYNC_NORMAL_ACTIVE; return 0; }
The comment doesn't agree with the logic of the test. Which is wrong?
| |