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?
--
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
