Re: 2.6.31-rc5 regression: Oops when USB Serial disconnected whilein use

[Alan Stern]

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-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/