Re: [PATCH] usb-serial regression fix

Jim Radford
2007-03-13T15:36:51Z

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 */