Messages in this thread | | | Date | Wed, 29 Jul 2009 12:07:04 +0100 | From | Alan Cox <> | Subject | Re: [PATCH] kdesu broken |
| |
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.
| |