On Fri, 2013-02-22 at 12:35 -0500, Alan Stern wrote:
> On Fri, 22 Feb 2013, Johan Hovold wrote:
>
> > > > So we end up with an unregistered device still possibly referenced by
> > > > tty instead, and I suspect we can't do much else than deal with any
> > > > post-disconnect callbacks. [ These should be few, especially with my
> > > > latest TTY-patches applied. ]
> > > >
> > > > Alan, do you see any way around this? It's not possible (or desirable)
> > > > to pin the parent device (interface) until the last reference is
> > > > dropped, is it?
> > >
> > > On the contrary, it is customary to pin data structures until the last
> > > reference to them is gone. That's what krefs are for.
> >
> > I was referring to the usb device in the device hierarchy, which
> > apparently is not pinned despite the outstanding reference we have in
> > struct usb_serial.
>
> Are you talking about the usb_device or the usb_interface? The
> usb_serial structure _does_ pin the usb_device structure. But it
> doesn't pin the usb_interface. I don't know why things were done this
> way; maybe it's a mistake.
>
> Anyway, keeping a pointer to a non-pinned data structure after
> unregistration is okay, provided you know you will never dereference
> the pointer. If you don't know this in the case of the usb_interface,
> pinning is acceptable -- depending on _how_ the usb_interface is
> accessed. For example, no URBs should be submitted for any of the
> interface's endpoints.
>
> > There is an unconditional call to device_del in usb_disconnect which
> > unlinks the parent usb device from the device hierarchy resulting in the
> > broken devpaths above if we do not unregister the usb-serial port (and
> > tty device) in disconnect.
>
> Sure. But unregistering is different from deallocation. It's not
> clear what your point is.
>
> > > On the other hand, the port private data was owned entirely by the
> > > serial sub-driver. Neither the serial core nor the tty layer is able
> > > to use it meaningfully -- they don't even know what type of structure
> > > it is.
> > >
> > > Therefore freeing the structure when the port is removed should be
> > > harmless -- unless the subdriver is called after the structure is
> > > deallocated.
> >
> > Which could happen (and is happening), unless we defer port unregister
> > until the last tty reference is dropped -- but then we get the broken
> > uevents.
>
> Unregistration should not be deferred. We mustn't have those broken
> devpaths.
>
> > > This means there still is one bug remaining. In
> > > usb_serial_device_remove(), the call to tty_unregister_device() should
> > > occur _before_ the call to driver->port_remove(), not _after_. Do you
> > > think changing the order cause any new problems?
> >
> > Yes, Peter noticed that one too. Changing the order shouldn't cause any
> > new issues as far as I can see. I'll cook up a patch for this one as
> > well, but just to be clear: this is not directly related to the problem
> > discussed above as there may be outstanding tty references long after
> > both functions return (not that anyone has claimed anything else).
>
> This is related to the problem of the port's private data being
> accessed after it is deallocated. The only way that can happen is if
> the tty layer calls the subdriver after the private data structure is
> freed -- and you said above that this does happen.
>
> But if change things so that the structure isn't freed until after the
> port is unregistered from the tty layer, this would mean that the tty
> layer is trying to do stuff to an unregistered port. That would be a
> bug in the tty layer.
>
> I'm not saying such bugs don't exist. However, if they do exist then
> the tty layer needs to be fixed, not the usb-serial layer.
ISTM the usb_serial_port private data should not be freed until
port_release().
If usb traffic isn't halted until port_release() ...
static void port_release(struct device *dev)
{
struct usb_serial_port *port = to_usb_serial_port(dev);
int i;
dev_dbg(dev, "%s\n", __func__);
/*
* 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);
then what happens here if ->port_remove() has already been called?
static void garmin_write_bulk_callback(struct urb *urb)
{
struct usb_serial_port *port = urb->context;
if (port) {
struct garmin_data *garmin_data_p =
usb_get_serial_port_data(port);
if (GARMIN_LAYERID_APPL == getLayerId(urb->transfer_buffer)) {
====> if (garmin_data_p->mode == MODE_GARMIN_SERIAL) {
gsp_send_ack(garmin_data_p,
((__u8 *)urb->transfer_buffer)[4]);
}
--
To unsubscribe from this list: send the line "unsubscribe linux-serial" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
