On Monday 25 September 2006 1:17 pm, Gerhard Pircher wrote: > Now, the problem is deciding in which of these steps the actual error > lies, since none of these facts (apart from #6) is set in stone. In > my opinion though, it makes sense to simply say that atomic_t:s (and > therefore kref:s) are not supported in DMA memory. This would place > the error in the EHCI driver, with two possible solutions: > > A) Rewrite qh_get() and qh_put() to use something else than > kref_get()/kref_put(). Simply using non-atomic increment and > decrement made the Oops go away, but as I don't know the design > decision behind using a struct kref, I can't say that atomicity > isn't needed, so such a simple fix might lead to race conditions.
The use of kref was a simplification ... adopting an idiom that was used in many other places in the kernel. You're right that there was no need for its "atomic" aspect, since those refcounts should always have been protected by the driver spinlock. I like the idea of keeping to that idiom. How about just defining a non-atomic version of kref, for use in cases where atomic ops are not appropriate? Seems to me that should be fully inlinable. > B) Break struct ehci_qh into two parts, one allcated with > dma_pool_alloc() and one allocated with kmalloc(), where the fields > accessed by the hardware is put into the former, and the driver > private data (which includes the kref) in the latter. Safe and no > major performance hit (just one level of indirection added in some > places, and using cache-enabled memory for the internal data might > actually improve performance), but the change touches a rather > large amount of lines (patch attached). I don't much like this one, other than the potential improvement in cache behavior. While it's true that _now_ there appear to be no memory leaks, or related races with the controller, we got there by removing complexity ... like the complexity of two memory allocations for each QH. (Or QTD, or ITD, or SITD ... you'd need to change all of those if the rationale for this change were good.) > C) Basically the same as B, but only the kref (and a pointer back to > the rest of the data, so that qh_destroy can find it) is moved to > the kmalloced part. This means only ehci_qh_alloc(), qh_get() and > qh_put() need to be changed, so the changeset is much smaller. I > don't have a patch ready for this, but I can make one on request. Like this even less. > A completely different approach would be > > D) Make the DSI exception handler emulate lwarx on cache-inhibited > pages. I see you have prioritized these proposals so that each successive one makes me less and less comfortable ... ;) - Dave ------------------------------------------------------------------------- Take Surveys. Earn Cash. Influence the Future of IT Join SourceForge.net's Techsay panel and you'll get the chance to share your opinions on IT & business topics through brief surveys -- and earn cash http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV _______________________________________________ linux-usb-devel@lists.sourceforge.net To unsubscribe, use the last form field at: https://lists.sourceforge.net/lists/listinfo/linux-usb-devel
