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
