On Wed, Mar 04, 2009 at 02:27:02PM +0100, Christoph Egger wrote: [...] > The driver's attaching function knows something failed and calls the > function which free's the dma memory (actually, some drivers And who says this is a correct thing to do? It seems to me that here you're only trying to give an excuse to sloppy programming. An allocation function that leaves half of what it is supposed to allocate hanging around while returning an error is not exactly good practice. > don't do even that!). This free function just gets a softc argument, > so the error code is unknown. The free routine doesn't even > know if something failed. All what it can do is that it checks This is where your argument is fallacious. The error code returned by the xxx_dma_alloc function doesn't express the point at which it stopped doing allocations anyway. > the dmamap pointer against NULL. If it isn't NULL, it calls > bus_dmamap_destroy() => BOOM > > > > If the bug is caused by reusing dmamaps, we should explicitly > > make dmamap NULL in driver dependent free functions, rather than > > MI bus_dmamap_create(9) API. > > How should the *free* function do that ? But that's the point. Calling the free function for partial clean-up is what is sloppy. I know of no other API that uses two different means simultaneously to indicate an error. Changing bus_dma(9 to do that would be setting a very bad precedent. > There are two ways to fix above described problem: > > 1) In the driver: In the dma allocation routine, explicitely set > the dmamap pointer to NULL in the error path of bus_dmamap_create() > before it returns. This makes the dmamap pointer check against NULL > as described above correct and bus_dmamap_destroy() isn't called. > > Required effort: We have to go through all drivers and we always > have to fix modified/new drivers again and again. That's something to be done on all drivers *anyway*. > Disadvantage: This is a forever task since we will never stop changing or > writing new drivers. That's another fallacious argument. The reason why failures during attachment are so badly handled is because historically the driver was toasted at that point anyway. It's only very recently that the user is able to detach it, although most of them don't even have a detach() method. I'm sure you have noticed how anything that uses bus_dma(9) or bus_space(9) after attachment takes good care of using it properly. All drivers will have to be revisted at some point. Attach functions will be allowed to return an error, and no, it's no my intent to call the detach function in that case to get the driver to clean up. It'd just not make any sense to me, no matter how much sense it seems to make to you with DMA memory allocation routines. -- Quentin Garnier - cube%cubidou.net@localhost - cube%NetBSD.org@localhost "See the look on my face from staying too long in one place [...] every time the morning breaks I know I'm closer to falling" KT Tunstall, Saving My Face, Drastic Fantastic, 2007.
Attachment:
pgp7agwMnhmQ4.pgp
Description: PGP signature