On Fri, Mar 15, 2013 at 08:02:45AM -0400, Peter Hurley wrote: > 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. It's conceptually cleaner, yes. But as a tty-ref can be held indefinitely (in particular, it can be held after having unregistered the device) and the wake up is not synchronous, I don't see how not doing so would make things any less robust, though. > > > 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. Either way, this series moves the wake-up from the individual driver's port remove to usb-serial disconnect and before unregistering the device from the bus. However, looking at this again now, I realise that the wake up must be done in hangup rather than disconnect (the common source for hangup in usb-serial) in order to fully fix the problem with processes not being woken up on hangup. (Perhaps, this is what you referred to with "robust".) I'll respin the series. Thanks, 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
