On Thu, 23 Apr 2009 tom.leiming@xxxxxxxxx wrote:
> From: Ming Lei <tom.leiming@xxxxxxxxx>
>
> This patch releases the usb_serial_port object after releasing
> usb_serial object, because .shutdown of usb_serial_driver often
> references port object and it is called in destroy_serial(),
> when it is touched, port object has been released.
>
> This bug is introduced by Alan's patch:
> usb-serial-fix-lifetime-and-locking-problems.patch.
>
> The bug is reproduced very easily by the following steps:
> 1, plug a usb serial port and unplug it;
> or
> 2,plug a use serial port, open it ,disconnect it, and close it.
>
> In each case, the oops will happen:
If I understand correctly, the real problem is that many of the serial
sub-drivers are badly written. :-(
The usb_serial_driver probe, attach, and shutdown methods are invoked
based on events affecting the entire serial device, not any particular
port. Hence the sub-drivers should not try to access any ports from
within those methods. The ports may not have been created yet, or they
may already have been destroyed (which is what happened here).
If there is port-specific work to be done, it should be carried out in
the port_probe and port_remove methods. That's what they're for. But
the sub-drivers don't use them properly.
Now, I don't relish the idea of going through a painstaking rewrite of
each USB serial sub-driver. But maybe we can fix things more easily by
moving the shutdown call from destroy_serial() into
usb_serial_disconnect(). That's what this patch does; it is intended
to go on top of my original patch.
Please, everyone try it out. If it works, Greg can merge the two
patches or I can resubmit the original one.
Alan Stern
P.S.: Whether or not the oops would occur depends on what actions are
taken by the shutdown method in the particular sub-driver you're using.
That explains why some people experienced it and others didn't.
Index: usb-2.6/drivers/usb/serial/usb-serial.c
===================================================================
--- usb-2.6.orig/drivers/usb/serial/usb-serial.c
+++ usb-2.6/drivers/usb/serial/usb-serial.c
@@ -137,8 +137,6 @@ static void destroy_serial(struct kref *
dbg("%s - %s", __func__, serial->type->description);
- serial->type->shutdown(serial);
-
/* return the minor range that this device had */
if (serial->minor != SERIAL_TTY_NO_MINOR)
return_serial(serial);
@@ -1067,6 +1065,10 @@ void usb_serial_disconnect(struct usb_in
serial->disconnected = 1;
mutex_unlock(&serial->disc_mutex);
+ /* Unfortunately, many of the sub-drivers expect the port structures
+ * to exist when their shutdown method is called, so we have to go
+ * through this awkward two-step unregistration procedure.
+ */
for (i = 0; i < serial->num_ports; ++i) {
port = serial->port[i];
if (port) {
@@ -1077,10 +1079,18 @@ void usb_serial_disconnect(struct usb_in
}
kill_traffic(port);
cancel_work_sync(&port->work);
- device_unregister(&port->dev);
+ device_del(&port->dev);
+ }
+ }
+ serial->type->shutdown(serial);
+ for (i = 0; i < serial->num_ports; ++i) {
+ port = serial->port[i];
+ if (port) {
+ put_device(&port->dev);
serial->port[i] = NULL;
}
}
+
/* let the last holder of this object
* cause it to be cleaned up */
usb_serial_put(serial);
--
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
