On Wed, 2013-02-13 at 15:25 +0100, Johan Hovold wrote:
> On Fri, Feb 01, 2013 at 10:44:25AM +0800, Chris Ruehl wrote:
> > I file a report for you, please have a look when you have time.
>
> Thanks for the bug report, Chris.
>
> You have come across what looks like a known issue, which since it's
> discovery last summer has been made worse by an unrelated change.
>
> A similar oops was reported and its cause identified in this thread:
>
> http://marc.info/?l=linux-usb&m=133837337927749&w=2
>
> It turns out that the tty-layer may call the driver's dtr_rts even after
> the device has been disconnected and the tty device unregistered. Since
> last summer another change has made the problem worse by setting the
> port data to NULL which results in even more drivers hitting the
> problem.
The tty driver's close() routine must be called even if the open()
failed because the tty layer doesn't know if the driver left unfinished
business and is expecting to receive a close() to cleanup even if it
failed the open(). This behavior was just recently documented in
include/linux/tty_driver.h (ie., is in linux-next).
> While waiting for input from the tty-guru Alan Cox, and as the immediate
> cause of that oops was remedied (by moving the offending interface
> access in the driver in question), the problem was unfortunately
> forgotten (or rather down-prioritised) until now.
Looks to me like a bug the usb serial mini-port interface design.
A usb bus disconnect has the pl2303 (and every other) mini-port freeing
the private data (before unregistering with tty anyway -- not that
putting that first would fix the problem).
static int usb_serial_device_remove(struct device *dev)
{
struct usb_serial_driver *driver;
struct usb_serial_port *port;
int retval = 0;
int minor;
port = to_usb_serial_port(dev);
if (!port)
return -ENODEV;
/* make sure suspend/resume doesn't race against port_remove */
usb_autopm_get_interface(port->serial->interface);
device_remove_file(&port->dev, &dev_attr_port_number);
driver = port->serial->type;
if (driver->port_remove)
====> retval = driver->port_remove(port);
minor = port->number;
tty_unregister_device(usb_serial_tty_driver, minor);
dev_info(dev, "%s converter now disconnected from ttyUSB%d\n",
driver->description, minor);
usb_autopm_put_interface(port->serial->interface);
return retval;
}
The pl2303 mini-port dutifully:
static int pl2303_port_remove(struct usb_serial_port *port)
{
struct pl2303_private *priv;
priv = usb_get_serial_port_data(port);
===> kfree(priv);
return 0;
}
while the tty layer still has outstanding references to the port.
static void pl2303_dtr_rts(struct usb_serial_port *port, int on)
{
=====> struct pl2303_private *priv = usb_get_serial_port_data(port);
unsigned long flags;
u8 control;
[...]
The tty layer (and its port implementation) can't protect the pl2303
mini-port from this behavior/interface design.
It's the glue layer's responsibility to correctly manage the differing
lifetimes of its bus devices with tty devices (and tty_ports, if used).
Meaning, that if the physical device disconnects from the bus, the ports
must simply become in-operative; they can't disappear.
BTW, just fixing this one part won't work because the usb serial driver
is also abruptly deleting the usb_serial_port device as well:
static void usb_serial_disconnect(struct usb_interface *interface)
{
[...]
for (i = 0; i < serial->num_ports; ++i) {
port = serial->port[i];
if (port) {
struct tty_struct *tty = tty_port_tty_get(&port->port);
if (tty) {
tty_vhangup(tty);
tty_kref_put(tty);
}
kill_traffic(port);
cancel_work_sync(&port->work);
if (device_is_registered(&port->dev))
========> device_del(&port->dev);
}
}
[...]
}
Ummm, wasn't that where the port private data pointer was?
Regards,
Peter Hurley
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
