On Fri, 19 Sep 2003, Greg KH wrote:
> On Fri, Sep 19, 2003 at 05:17:22PM -0400, Alan Stern wrote:
> >
> > ===== config.c 1.25 vs edited =====
> > --- 1.25/drivers/usb/core/config.c Mon Sep 15 10:14:46 2003
> > +++ edited/drivers/usb/core/config.c Fri Sep 19 17:07:29 2003
> > @@ -237,9 +237,6 @@
> > memset(interface, 0, sizeof(struct usb_interface));
> > interface->dev.release = usb_release_intf;
> > device_initialize(&interface->dev);
> > -
> > - /* put happens in usb_destroy_configuration */
> > - get_device(&interface->dev);
> > }
> >
> > /* Go through the descriptors, checking their length and counting the
> >
>
> No, taking this out is wrong. Read the comment :)
>
> We need to increment the count by one, as we want the interfaces to
> stick around until the whole device is cleaned up. That happens from a
> call to usb_release_dev when the final kobject put is called on the
> struct usb_device. Then that calls usb_destroy_configuration() which
> cleans up the raw descriptors of the interfaces that were owned by the
> struct usb_device, and then calls the last put on the struct
> usb_interface, which then will finally free that memory.
>
> If this patch goes in, the interface memory will be freed when the
> interface is unregistered from the driver core (with the call to
> device_unregister(&interface->dev); in usb_disconnect) which is way too
> early in the cleanup sequence.
>
> Did you test that patch? What happened when you unplugged a device?
I did test it, after applying David's usb_set_configuration rework patch.
It worked exactly right. Without my patch, the interface and device were
never released.
Note that my patch was meant to apply on top of David's. He changed the
call to device_unregister(&interface->dev) you mentioned to device_del().
That does make a difference.
Here's the situation with David's patch applied but not mine. The count
starts out at 1. Even if it's not incremented, the interface will stick
around until the whole device is cleaned up. At that time the sequence of
events you described takes place. The device_del() call doesn't change
the count. The last put of the struct usb_interface() decrements the
count to 0 and so releases the memory. But if the get_device() is left
in, the last put will decrement the count to 1 and the memory will never
be freed.
To put it another, the original code has (when the interface is created):
device_initialize()
get_device()
when the config is set:
device_add()
when the device is disconnected:
device_del()
and when the configuration is destroyed:
put_device()
At the end the reference count is 1, not 0.
> > The other is in usb.c:usb_new_device(). Neither the get_device() nor the
> > put_device() in the function should be there. The patch below, which
> > applies on top of David's usb_set_configuration() rework patch, takes care
> > of them.
>
> No, that was added for another reason too, can't remember what it was
> right this second though, gotta go pick up my daughter from school...
Similar comments apply here, although perhaps the changes introduced by
David's patch have confused the matter. usb_alloc_dev() calls
device_initialize(), setting the count to 1. Then usb_new_device() does
usb_get_dev() and usb_disconnect() calls usb_put_dev(). The net result is
that the device's reference count is 1, so it isn't deallocated.
Try adding a debugging line to the end of usb_disconnect() that prints
the value of atomic_read(&dev->dev.kobj.refcount) just before the final
call to usb_put_dev() and you'll see what I mean.
Alan Stern
-------------------------------------------------------
This sf.net email is sponsored by:ThinkGeek
Welcome to geek heaven.
http://thinkgeek.com/sf
_______________________________________________
[EMAIL PROTECTED]
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel
