Re: [PATCH v2 33/85] USB: serial: clean up usb-serial bus device removal

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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


[Index of Archives]     [Linux Media]     [Linux Input]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Old Linux USB Devel Archive]

  Powered by Linux