On Tue, 14 Apr 2009, Alan Cox wrote:
> (Please direct tty layer patches to me as well as Greg as I'm the one
> currently trying to beat the tty layer into submission)
Sorry; I didn't realize this would interact quite so much with the tty
layer.
> > Similar patches will be submitted for the .stable kernels after this
> > one is accepted. Because of other changes, this one can't be applied
> > verbatim.
>
> This entire code area has been significantly revamped in the devel code
> to use the core tty_port_* helper functions.
Are there a lot of pending changes to usb-serial.c that aren't already
in Greg's tree?
> I'll go over this patch in more detail tomorrow however.
>
> > - usb_serial_put(port->serial);
> > + put_device(&port->dev);
> > +
> > + /* Mustn't dereference port any more */
> > + if (count == 0) {
>
> So why is the put_device just before this safe - it dereferences port ?
The comment means that as a result of the put_device, port is now a
stale pointer. Should I rephrase it (or omit it entirely)?
> > + * Stop all the traffic before cancelling the work, so that
> > + * nobody will restart it by calling usb_serial_port_softint.
> > + */
> > kill_traffic(port);
> > + cancel_work_sync(&port->work);
>
> If the work queue held a kref and the destruction was done off the kref
> destructor that wouldn't be a problem and indeed almost all the existing
> mess goes away if we treat the tty port kref as the anchor for all the
> upper levels (and the USB serial device level destructor can free a kref
> to the tty to hold it all together).
I'm very confused. By "the tty port kref" do you mean
port->dev.kobject.kref, or do you mean something else?
By "the USB serial device level destructor" do you mean the
destroy_serial() routine? (Just want to make sure I understand
correctly.)
Is "a kref to the tty" different from "the tty port kref"?
It sounds like you want to introduce a refcounting loop, with the
serial device holding a reference to one of its descendants! I _must_
have misunderstood.
In fact, I don't see how this cancel_work_sync() call can be an issue
at all. Is there anything wrong with using it in place of
flush_scheduled_work()?
> The easiest way to handle this is to kill the entire disc_mutex. It
> serves no real purpose except to serialize a one shot event and report
> it. You can do that with test_and_set_bit()
>
> In usb_serial_disconnect()
>
> if(test_and_set_bit(...))
> return;
>
> and test the bit in the disconnect path
I don't get it. What purpose does test_and_set_bit() serve if it
occurs in only one place?
> The other case it occurs in the old drivers is close() but this is a
> symptom of incomplete implementation and non-POSIX behaviour. Once you
> use the tty_port helpers then hangup() completes the physical close and
> the close() method for a port post hangup is a physical layer no-op. Thus
> the two flows you have are
>
> usb_serial_disconnect
> t&sb blocks recursion
> calls hangup
> kills hardware in the recursion blocked path
How does hangup go about killing the hardware? Does it somehow arrange
for usb_serial_disconnect() to be called again?
> close
> tests bit for usbautopm
>
> with usbautopm doing the disconnected device logic internally.
No doubt there's something going on in the tty layer here that I'm not
aware of.
The purpose of disc_mutex is to prevent the driver from doing I/O to a
device after they have been unbound. (It's important to remember that
a driver can be unbound from a device without the device being
unplugged.)
Without the mutex, there's a race between usb_serial_disconnect() and
the subdriver's open method. Does tty_hangup() resolve this race
somehow?
Alan Stern
--
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
