On Tue, Feb 18, 2003, Greg KH <[EMAIL PROTECTED]> wrote:
> On Tue, Feb 18, 2003 at 08:59:15AM -0800, David Brownell wrote:
> > Greg KH wrote:
> > >
> > >Yes, this is a problem, the module can be unloaded and then the
> > >completion fuction can be called.
> >
> > How? I don't see it. Unless the driver completely violates
> > basic rules like implementing disconnect() correctly, which
> > will break more than just module unloading.
>
> Hm, let me think about this some more before responding...
>
> > > I'm trying to fix up the usb device reference counting
> > >logic right now and running into issues where host controllers try to
> > >access urbs after their devices have been removed from the system :(
> >
> > Not supposed to happen ... when they call giveback(), that's supposed to
> > mean they've forgotten about that URB. Is it buggy disconnect() logic,
> > or some HCD calling giveback too early?
>
> I'm seeing uhci_irq() get called for an urb that belongs to a usb device
> that has already been disconnected. Things go wrong when
> uhci_transfer_result() is called, which calls usb_release_bandwidth()
> which expects dev->bus to be a valid pointer. I thought that after
> deallocate() is called on the bus for the device, everything would be
> cleaned up.
This is a reference counting problem.
In 2.4, uhci.c holds a reference to the device until after it's done
calling the completion handler.
Now in 2.5, the hcd layer doesn't hold that reference anymore it seems.
That should be fixed.
> Here's the patch to the usb core that shows this quite nicely:
>
>
> ===== usb.c 1.190 vs edited =====
> --- 1.190/drivers/usb/core/usb.c Sun Feb 16 05:48:02 2003
> +++ edited/usb.c Sun Feb 16 09:03:33 2003
> @@ -750,10 +750,7 @@
> if (!udev)
> return;
>
> - if (udev->bus && udev->bus->op && udev->bus->op->deallocate)
> - udev->bus->op->deallocate(udev);
> usb_destroy_configuration (udev);
> - usb_bus_put (udev->bus);
> kfree (udev);
> }
>
> @@ -820,6 +817,7 @@
> void usb_disconnect(struct usb_device **pdev)
> {
> struct usb_device * dev = *pdev;
> + struct usb_bus *bus;
> int i;
>
> if (!dev)
> @@ -853,6 +851,17 @@
> usbfs_remove_device(dev);
> }
> device_unregister(&dev->dev);
> +
> + /* Detach the bus from the device right now, as it's no longer a
> + * valid link. This helps for drivers that keep a reference to
> + * the device around for a while if userspace holds a device
> + * open. It will cause all further urb submissions to fail,
> + * which is what we want to have happen. */
> + bus = dev->bus;
> + if (bus && bus->op && bus->op->deallocate)
> + bus->op->deallocate(dev);
> + dev->bus = NULL;
> + usb_bus_put (bus);
>
> /* Decrement the reference count, it'll auto free everything when */
> /* it hits 0 which could very well be now */
>
>
>
> I have a patch to the usb serial code that tests this logic out by
> holding a reference to the usb device structure while a port is opened
> by a user. That way, if the device disappears, then all writes and
> reads get a -ENODEV, and when the last consumer leaves the driver, the
> device is finally deleted from memory properly. This seems to solve the
> nasty race conditions that people have been having with the usb-serial
> drivers lately.
>
> Time to test this out on ohci to see how well it works...
I think you're solving the symptom, not the problem.
JE
-------------------------------------------------------
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
