> > (Does qh->dev really need to be valid for each qh on the
> > qh_remove_list, though? Could that be invalidated when
> > each qh was put on that list -- usb_dec_dev_use and null the
> > pointer?)
>
> It doesn't have to be, we can decrement the reference earlier, but it won't
> change how correct the code is, it'll just result in the memory being freed
> 1ms earlier.
Well, next frame plus random interrupt latency later ... it could be a
lot longer than 1ms in some cases.
If it's correct to do that, then I think that'd prevent the issue I'm
concerned about. Which _is_ a correctness issue, even if it's
not yet been observed in the wild (2.5.8+) ... see below.
> > > usb_free_dev 5
> >
> > You must mean usb_dec_dev_use() at this point.
> > Who's calling it though? Remember khubd only
> > calls this when every disconnect() routine finished.
>
> At the end of usb_disconnect. usb_dec_dev_use is an alias for
> usb_free_dev, so it's the same code. Note the indentation I used
> for the calling order.
As of 2.5.8, usb_dec_dev_use() is no longer a #define for
usb_free_dev() ... some of the previously loose device
refcounting is now tighter, to ensure that the various parts
of the USB code handle their responsiblities consistently.
(HCDs, interface drivers, hub driver, usbfs, and so on.)
It's certainly a lot better in my book to report such bugs
early with a BUG() than to let the bugs linger for a few
years (as with that usbfs disconnect bug!) until someone
finally manages to assemble enough of the puzzle pieces
to figure out which part was at fault.
> uhci_free_dev will never be called before qh_remove_list removes the QH
> because the reference count won't be 0 until qh_remove_list is
> processed.
See what I wrote earlier. I recognize that there's a strange ordering
in usb_free_dev(), which I'm inclined to call a latent bug. It'd affect
any HCD that touches dev refcounts in its device deallocate code,
and the symptom would be that such an HCD would BUG() out in
the updated usb_free_dev() code. We don't have any of those now.
The real scenario I thought I saw, and you've now confirmed could
happen (I don't think it's been seen yet) is:
- khubd disconnect()s all drivers
- which leaves N qh->dev refs on the remove_list
- RACE does interrupt happen first?
* YES! (or N=0)
... the qh->dev refs vanish
... khubd successfully frees (count == 1)
* NO!
... khubd tries to free the device
...BUG() because of qh->dev refs (count = N+1)
The simple fix is to invalidate those qh->dev pointers earlier.
An alternate one would be to have uhci_free_dev() block till
the remove list is empty, AND (IMO generally the right thing
to do) invoke the HCD deallocate() before checking that
device refcount.
- Dave
_______________________________________________
[EMAIL PROTECTED]
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel
