Re: [PATCH] kdesu broken

[Alan Cox <alan@lxorguk.ukuu.org.uk>]

On Tue, 28 Jul 2009 21:49:05 -0700 (PDT)
Linus Torvalds <torvalds@linux-foundation.org> wrote:

> 
> 
> On Tue, 28 Jul 2009, Gene Heskett wrote:
> > 
> > It doesn't seem to do anything for bz 13841 here.  Sorry, I wish it did.
> 
> No, bz 13841 isn't about pty's, it's about usb serial. The patches in this 
> thread would mainly be pty-related, and would affect other tty drivers 
> (like your usb one) only purely by chance.
> 
> could you bisect the 13841 behavior?

See the thread in 13821 on linux-usb list which extends from the "why do
we leak ttyUSB numbers" into this as well. Its been pinned down and the in
tree fix is confirmed to do the job for that case. Not perfectly because
it needs a further small fix as

a) it should call drv->close() within the mutex on the error path if the
tty_block_til_ready() fails without which you get a leak on a few drivers
but nothing too harmful. (Noted by Alan Stern inspecting the patch)

b) it fails the 'variable frequency generator on carrier pin' test - but
that isn't a regression as such as all 2.6 kernels I've tried crash
during that test with USB.  (Running my 'extreme violence' test setup)

I suspect that doing

        retval = tty_port_block_til_ready(&port->port, tty, filp);
        if (retval == 0) {
                if (!first)
                        usb_serial_put(serial);
                return 0;
        }
        mutex_lock(&port->mutex);


	if (tty_hung_up_p(filp)) {
		mutex_unlock(&port->mutex);
		return retval;
	}
	dev->close(port);

	...

fixes both. The race basically is

		open
		drop mutex so we can wait for the port
		wait for the port
						[hangup]
						[serial_do_down]
		block_til_ready reports an error
		take mutex
			duplicate serial_do_down


Most serial drivers don't try and do open clean up in open() instead they
do it in close() which is called by the tty layer on an open failure
(always has been) and which turns out to be a useful design
decision as it means the driver doesn't have to tie itself in knots and
tty_port_close_start() handles all the mechanics. Plus they use
ASYNC_INITIALIZED as flag to avoid double shutdowns on a device.

Simply deleting most of the clean up from serial_open and letting close()
do its job would I think clean that up no end but not in an -rc perhaps.