On Thu, 2013-03-14 at 19:30 +0100, Johan Hovold wrote: > On Thu, Mar 14, 2013 at 01:39:39PM -0400, Peter Hurley wrote: > > On Thu, 2013-03-14 at 16:23 +0100, Johan Hovold wrote: > > > Make sure to unregister the tty-device before calling subdriver > > > port_remove. > > > > > > This way remove will reverse probe, and specifically any port data > > > released in port_remove will be available throughout tty unregister. > > > > > > Note that the order currently does not matter as the tty-layer can make > > > callbacks also after the device has been unregistered. This is > > > handled in usb-serial core using the disconnected flag, which is > > > already set when usb-serial bus device remove is called. > > > > > > Cc: Peter Hurley <peter@xxxxxxxxxxxxxxxxxx> > > > Reported-by: Peter Hurley <peter@xxxxxxxxxxxxxxxxxx> > > > Signed-off-by: Johan Hovold <jhovold@xxxxxxxxx> > > > > Looks fine to me. > > > > The thing I did notice is that the ftdi usb serial mini-driver has its > > own ioctl() handler for TIOCMIWAIT which can be holding the tty > > reference open after the unregister. > > Yes, this is expected and perfectly fine. User-space may hold any > tty-ref indefinitely, but we must unregister the tty-device at > disconnect and then deal with it using the disconnected flag. > > > Its port_remove() method, ftdi_sio_port_remove(), does the wake up to > > kick it off the loop. > > Yes, all is good (except for another use-after free bug in that very > implementation that I fixed elsewhere). We hang up the port at > disconnect, unregister the device from the bus, which calls ftdi port > remove and wakes up any processes waiting on the delta_msr_wait queue, > which then return immediately based on the disconnected flag (this last > bit wasn't the case before the fix mentioned above but has nothing to > do with tty unregister). > > > Now that the call order is reversed, that will need to be fixed. > > No, the call order in usb-serial bus remove does not change anything > really. Any process doing TIOCMIWAIT will only be woken up slightly > later (and again there is nothing wrong in keeping a tty ref after tty > unregister). I didn't mean it needed fixing because it would blow up; I just meant it needed fixing because it's not robust. Generally, if a tty driver needs to perform tty device-related cleanup, it should do so in the either the hangup() or close() call chain, not after it has knowingly unregistered the tty device. For the usb serial mini-drivers, this should be in their close() methods. > > A quick review of the usage seems like the whole ioctl() should be > > torn out along with the private delta_msr_wait. > > Most usb-serial TIOCIMWAIT-implementations, including the ftdi one, are > replaced by a generic implementation by this very series. Ok. I have a look at those. Regards, Peter Hurley -- 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
