Re: [PATCH] usb-serial regression fix

[Jim Radford <radford@blackbean.org>]

On Tue, Mar 13, 2007 at 09:55:41AM -0400, Mark Lord wrote:
> Jim Radford wrote:
> >On Mon, Mar 12, 2007 at 09:55:14PM -0400, Mark Lord wrote:
> >
> >>So where does the memory get freed -- the structure pointed at
> >>by the serial->port[i] thingie ?  It's not a leak, is it?
> >
> >It gets free'd through device_unregister
> >
> >    for (i = 0; i < num_ports; ++i) {
> >       ...
> >       port->dev.release = &port_release;
> >       ...
> >       retval = device_register(&port->dev);
> >
> >which means that until all the drivers get converted to use
> >->port_probe() and ->port_remove() (which gets called by
> >device_unregister) and stop using the ->port[] array in ->shutdown()
> >we need to have ->shutdown() called before device_unregister.
> >
> >>>>>Look at changeset d9a7ecacac5f8274d2afce09aadcf37bdb42b93a in Linus's
> >>>>>tree from Jim Radford:
> >
> >>>>>http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commit;h=d9a7ecacac5f8274d2afce09aadcf37bdb42b93a

> >So, this patch should be reverted for now.

> Okay, so.. Jim, could you spell it out for us now?

> I'm confused.

> Based on the current 2.6.21-rc3-git*, tell us *exactly* what (if
> any) needs to be reverted, and *exactly* which (if any) of my
> suggested patches to apply ?

The simple change to not NULL the ->port[] array after unregister was
not quite enough, as ->shutdown() would, as hinted at by Mark, then
access kfree'd memory.

The best thing to do is to just revert
d9a7ecacac5f8274d2afce09aadcf37bdb42b93a and work out a better fix for
the next release.  My patches to fix ftdi_sio no longer require this
patch, so it is pointless at this late state if it breaks *anything*,
and it does.


This patch reverts d9a7ecacac5f8274d2afce09aadcf37bdb42b93a since it
breaks drivers that need to access the ->port[] array in shutdown
(most of them).

Signed-Off: Jim Radford <radford@blackbean.org>

--- b/drivers/usb/serial/usb-serial.c
+++ a/drivers/usb/serial/usb-serial.c
@@ -137,6 +135,11 @@
 
 	dbg("%s - %s", __FUNCTION__, serial->type->description);
 
+	serial->type->shutdown(serial);
+
+	/* return the minor range that this device had */
+	return_serial(serial);
+
 	for (i = 0; i < serial->num_ports; ++i)
 		serial->port[i]->open_count = 0;
 
@@ -147,12 +150,6 @@
 			serial->port[i] = NULL;
 		}
 
-	if (serial->type->shutdown)
-		serial->type->shutdown(serial);
-
-	/* return the minor range that this device had */
-	return_serial(serial);
-
 	/* If this is a "fake" port, we have to clean it up here, as it will
 	 * not get cleaned up in port_release() as it was never registered with
 	 * the driver core */