On Wed, 22 Apr 2009 16:31:39 -0400 (EDT)
Alan Stern <stern@xxxxxxxxxxxxxxxxxxx> wrote:
> 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.
I use ch341 serial port, and this patch does fix oops for me.
Thanks.
>
> 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);
>
--
Lei Ming
--
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
