Hi Alan, It seems we have come across a bug in the tty-layer where dtr_rts is called after vhangup as well as tty_device_unregister has returned (both called from usb-serial disconnect). Do you think you can provide some insight into the apparent hangup race described below? Furthermore, are we correct to assume that no further calls to port shutdown are supposed to be made once vhangup (or tty_device_unregister) returns so that we could get rid of the disconnected checks in several usb-serial close functions? Thanks, Johan On Wed, May 30, 2012 at 12:20:02PM +0200, Johan Hovold wrote: > On Fri, May 25, 2012 at 01:46:48PM -0400, Alan Stern wrote: > > On Fri, 25 May 2012, Johan Hovold wrote: > > > > > > Well tty-core shouldn't handle disconnect (which is usb-serial- > > > > specific) > > > > > > Just to clarify: the tty layer could learn to deal with disconnect > > > (useful also for cdc-acm for example), but currently it is handled in > > > usb-serial core so moving it there first for dtr_rts makes sense. > > > > The tty layer may not know how to handle disconnect, but it certainly > > does know about handling tty_unregister_device(). Since that routine > > gets called by the usb-serial core as part of disconnect processing, > > what more is really needed? > > You're right, and this is indeed a bug in the tty layer. In fact, there > are three different issues interacting here which are related to the > oops Jan discovered: > > 1. hangup race in tty > 2. access to interface after disconnect > 3. io after disconnect (e.g. during close) > > > 1. hangup race in tty > --------------------- > First the call to lower dtr/rts should never happen after hangup -- > tty_close_start checks tty_hung_up_p(filp) and returns if hung up. > > I'm able to reproduce the race (and oops) reliably here using the pl2303 > driver. The bug is triggered when an open occurs after vhangup has been > called from disconnect. That open fails since the device has been > disconnected (-ENODEV from serial_install), causing tty_open to call > tty_release which ends up proceeding with port close despite having a > received a hangup and still having ports open (normally dtr/rts is only > lowered on the last close). Here a log from such a run: > > [ t0 ] tty_open - filp = f50c2d80 > > open the device once, then open and close in a loop and finally > disconnect the device > > ... > > [ 293.705143] tty_open - filp = f5100a80 > [ 293.705173] tty_open: opening ttyUSB0... > [ 293.705186] pl2303_dtr_rts - on = 1 > [ 293.705249] ttyUSB0 vhangup... > > hangup due to disconnect > > [ 293.705254] tty_release: ttyUSB0 (tty count=2)... > [ 293.705259] tty_port_close > [ 293.705297] __tty_hangup - setting hung_up_tty_fops, filp = f50c2d80 > [ 293.705302] tty_open - filp = f5100f00 > > open after hangup -- get's the normal tty_fops... > > [ 293.705423] tty_open: opening ttyUSB0... > [ 293.705441] tty_open: error -19 in opening ttyUSB0... > [ 293.705470] tty_release: ttyUSB0 (tty count=2)... > > open fails -- calls release which calls close > > [ 293.705478] tty_port_close > [ 293.705489] tty_port_close_start - closing, filp = f5100f00 > [ 293.705499] tty_port_close_start - drain_delay > > close proceeds despite hangup -- > schedule_timeout_interruptible() because drain delay is set for > pl2303 > > [ 293.705795] pl2303 ttyUSB0: pl2303 converter now disconnected from ttyUSB0 > [ 293.705865] usbserial_generic 6-1:1.0: device disconnected > > usb_serial_disconnect returns > > [ 293.971228] tty_port_close_start - dropping dtr_rts > [ 293.971254] pl2303_dtr_rts - on = 0 > > dropping dtr/rts after hangup, disconnect, and > tty_device_unregister (and with open ports) > > [ 293.971860] tty_release: ttyUSB0 (tty count=1)... > [ 293.971935] tty_port_close > [ 293.971946] tty_port_close_start - hung up > > last port (opened at t0) closes (without touching dtr/rts) > > [ 293.971963] tty_release: freeing tty structure... > > > 2. access to interface after disconnect > --------------------------------------- > The oops occurs when dereferencing serial->interface->cur_altsetting. In > my setup even the interface condition has a strange value, e.g.: > > [ 1005.517927] pl2303_dtr_rts - serial = f53b8540, intf = f53f3000, cur_alt = 00766564 > [ 1005.517938] pl2303_dtr_rts - condition = 559, unreg = 1 > > Documentation clearly states that we cannot use the interface after > returning from disconnect. In fact there's no reason for option to be > messing around with the interface in send_setup. I'm preparing a patch > which determines whether to use send_setup (check for blacklisting) and > store the interface number at probe instead. > > This will make the oops go away, even with the tty race still there. > > > 3. io after disconnect (e.g. during close) > ------------------------------------------ > You asked in reply to my patch why dtr_rts was special and needed to be > protected against disconnect. It's treated with extra care by several > drivers since Oliver introduced the disconnected flag: > > http://www.mail-archive.com/linux-usb@xxxxxxxxxxxxxxx/msg00207.html > > It used to be handled in driver close() but was later factored out to to > dtr_rts(). My patch simply refactored this protection to usb-serial > core. > > But as far as I can see, with the modifications that has since been done > to the tty layer, if the tty layer handled hangup in cases such as the > above we could get rid of this special handling as well? > > In fact, it seems to me we can even get rid of the disconnected checks > in close as vhangup will either call driver close (shutdown) or not > return until an ongoing close has finished. This should be enough to > uphold the rule: "You are not allowed any IO to a device after returning > from this [disconnect] callback" which was what the disconnected checks > in close was introduced for, right? > > Johan -- 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
