On Thu, Apr 18, 2002, David Brownell <[EMAIL PROTECTED]> wrote: > > 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.
Ok, now you're nitpicking. > > > > 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. I just noticed that and I'm appalled that this was done. I'm all for correctness, but you've made everything complicated along the way. > > 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) You've totally defeated the purpose of reference counting. Please, please, please go read the rest of the kernel to see how it has been used traditionally and how it should be used. > 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. The real fix is to put the reference counting back to the way it was and is supposed to be. JE _______________________________________________ [EMAIL PROTECTED] To unsubscribe, use the last form field at: https://lists.sourceforge.net/lists/listinfo/linux-usb-devel
