Re: proposal for bus_dma(9) change

[Christoph Egger <Christoph_Egger%gmx.de@localhost>]

On Wednesday 04 March 2009 13:48:05 Izumi Tsutsui wrote:
> Christoph_Egger%gmx.de@localhost wrote:
> > > Changing API is really pain than fixing bugs, IMO.
> >
> > My proposal is not like adding an additional parameter
> > to somewhere.
> > My proposal would change the behaviour of one function
> > in case of error.
>
> My question is if is there any benefit to change all MD
> bus_dmamap_create(9) implementation instead of fixing all drivers.
>
> In most drivers, dmamaps are prepared in softc and they are
> initialized to NULL in attach functions so checking it
> in failure path is valid.

Up to this point, this is correct. But then when the driver allocates
DMA memory, it first calls bus_dmamap_create() before it uses
any other bus_dma(9) functions.
The point here is, what if bus_dmamap_create() fails?
In this case, the dmamap pointer is undefined, meaning it may
have any value.
The drivers check the error code and return an error code back to
the caller  - the driver's attachment. This error code is often even
not the one from the failing bus_dmamap_create() function.
So the exact error code is lost past this point.

The driver's attaching function knows something failed and calls the
function which free's the dma memory (actually, some drivers
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
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 ?

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.
Disadvantage: This is a forever task since we will never stop changing or 
writing new drivers.

2) My proposal to change the behaviour of bus_dmamap_create():
In case of error, the dmamap is NULL (instead of undefined).
Effect: This makes the dmamap pointer check against NULL as
described above correct and bus_dmamap_destroy() isn't called.

Required effort: Find all implementations of bus_dmamap_create()
and adjust the implementation.
Benefit (you asked for): Once this is done, we don't need to worry
about exact *this* problem in the drivers anymore. This also
makes the use of bus_dmamap_create() more intuitively usable.


Christoph