Re: CVS commit: src/sys/arch/x86/x86

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

Quentin Garnier wrote:
> On Thu, Feb 19, 2009 at 06:12:59PM -0600, David Young wrote:
>> On Fri, Feb 20, 2009 at 12:54:59AM +0100, Christoph Egger wrote:
>>> David Young wrote:
>>>> On Thu, Feb 19, 2009 at 11:34:14PM +0000, Christoph Egger wrote:
>>>>> Module Name:      src
>>>>> Committed By:     cegger
>>>>> Date:             Thu Feb 19 23:34:14 UTC 2009
>>>>>
>>>>> Modified Files:
>>>>>   src/sys/arch/x86/x86: bus_dma.c
>>>>>
>>>>> Log Message:
>>>>> bus_dmamap_create(): on failure, reset dmamp or drivers
>>>>> like nfe(4) try to call bus_dmamap_destroy() on an invalid dmamap in 
>>>>> their error path.
>>>> Christoph,
>>>>
>>>> Please, back this change out and fix the drivers, instead.
>>> I tried that for nfe.
>>> In nfe_alloc_tx_ring(), setting ring->map = NULL right before
>>> 'goto fail;' has no effect.
>>> In nfe_free_tx_ring() it calls bus_dmamap_destroy(), nonetheless then.
>> It looks to me like a lot of things can go wrong if nfe_attach() calls
>> nfe_free_tx_ring() before everything is set up.  You cannot fix all of
>> nfe's bugs by changing the x86 implementation of an MI API.
> 
> Amen.
> 
>>> Further, bus_dma(9) manpage is not clear about the parameters if they
>>> are intended to be NULL or undefined in error case.
> 
> What does "undefined" means to you, Christoph?  Beside, you can use
> basic logic here;  bus_dmamem_create() already has a mean of indicating
> an error condition, why would it do more?

"undefined" means a random value across MD implementations rather a
certain one in failure case.

>>> manpage just says, bus_dmamap_create returns non-zero in failure case.
>> I have tried to make the manual page clear, see bus_dma.9 rev 1.46.
> 
> Thanks, Dave.
> 
> Please do back that out, Christoph, and try harder fixing nfe.

Done.