Re: proposal for bus_dma(9) change

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

On Wednesday 04 March 2009 14:56:33 Izumi Tsutsui wrote:
> Christoph_Egger%gmx.de@localhost wrote:
> > 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
>
> Ah, are you saying about _bus_dma_alloc_bouncebuf() failure
> in x86/bus_dma.c:_bus_dmamap_create() for example?

Yes, exactly.

>
> > 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.
>
> It's unaccectable to change only bus_dmamap_create(9)
> because all APIs should have consistent meaning of dmamap.

Ok, that needs to be reflected in bus_dma(9) manpage then.
Currently this is only documented for bus_dmamap_create().

> In -current API definition, returned dmamap is undefined
> in error case, so driver can't assume dmamap is valid
> if it isn't NULL and driver should have responsibility
> to keep invalid dmamap NULL in failure path and destroying path.

Looking into the drivers, they don't seem to care about their
responsibility.

> If the definition is changed as NULL dmamap means invalid dmamap,
> bus_dmamap_destroy(9) should also be changed to make passed dmamap NULL,
> and bus_dmamap_create(9) should reject non-NULL dmamap,
> otherwise inconsistent semantics could confuse driver writers
> and they will create more other bugs.

Ok, I'm fine with this. I just talked about bus_dmamap_create() only,
because I stumbled over this issue with bus_dmamap_create().

> > Benefit (you asked for): Once this is done,
>
> Does it require less effort to fix all bus_dmamap_create()
> than fixing each driver?

After the implementation of bus_dmamap_create() has been found,
find its error path and there set the dmamap pointer to NULL.
I already did that in sys/arch/x86/x86/bus_dma.c rev. 1.47
but had to back it out again (rev. 1.48).

I also fixed two drivers: nfe(4) and age(4). See
sys/dev/pci/if_nfe.c rev. 1.42 and sys/dev/pci/if_age.c rev. 1.25.

Look at this those revisions. This should allow you to
make an estimation of the required effort for a) changing bus_dma(9)
or b) fixing all drivers

> > 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.
>
> But you also have to worry about the problem when
> new MD bus_dmamap_create() implementations are added.
> Once all drivers are fixed, we don't have to worry about
> MD bus_dmamap_create(9) implementation.

> MI API with MD implementation should be designed carefully
> because maintainance cost is more expensive than independent drivers.

I'm following NetBSD development much much longer than I have my account.
I have seen way more drivers coming into the tree than new
MD bus_dmamap_create(9) implementations.

You're right, both solutions have maintenance costs. IMO, bus_dma(9)
is a lot cheaper in the long run.

Christoph