aic7xxx strange code

aic7xxx strange code

[Arjan van de Ven]

[-- Attachment #1: Type: text/plain, Size: 1499 bytes --]

Hi,


the snippet below (from aic7xxx driver) is rather strange and probably
broken:

        /*
         * Although we can dma data above 4GB, our
         * "consistent" memory is below 4GB for
         * space efficiency reasons (only need a 4byte
         * address).  For this reason, we have to reset
         * our dma mask when doing allocations.
         */
        if (ahc->dev_softc != NULL)
                if (ahc_pci_set_dma_mask(ahc->dev_softc, 0xFFFFFFFF)) {
                        printk(KERN_WARNING "aic7xxx: No suitable DMA
available.\n");
                        return (ENODEV);
                }
        *vaddr = pci_alloc_consistent(ahc->dev_softc,
                                      dmat->maxsize, &map->bus_addr);
        if (ahc->dev_softc != NULL)
                if (ahc_pci_set_dma_mask(ahc->dev_softc,
                                     ahc->platform_data->hw_dma_mask)) {
                        printk(KERN_WARNING "aic7xxx: No suitable DMA
available.\n");
                        return (ENODEV);
                }


consistent memory ALWAYS comes from lower 4Gb regardless of the
pci_set_dma_mask() setting; only the pci_set_consistent_dma_mask()
function controls where consistent memory comes from, even in 2.4.0
already (which the ifdef surrounding this code tests for)

Justin: are you willing to just remove the unneeded dma mask sets? They
can race etc etc and are 100% unneeded.

Greetings,
   Arjan van de Ven

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: aic7xxx strange code

[Justin T. Gibbs]

> Hi,
> 
> 
> the snippet below (from aic7xxx driver) is rather strange and probably
> broken:
> 
>         /*
>          * Although we can dma data above 4GB, our
>          * "consistent" memory is below 4GB for
>          * space efficiency reasons (only need a 4byte
>          * address).  For this reason, we have to reset
>          * our dma mask when doing allocations.
>          */

...

> consistent memory ALWAYS comes from lower 4Gb regardless of the
> pci_set_dma_mask() setting;

This isn't true on some architectures in the 2.4 kernel.  Look at, for
example, arch/ia64/hp/common/sba_iommu.c in the RedHat 2.4.18-e.31 kernel.
This is why this particular hack was added.

> Justin: are you willing to just remove the unneeded dma mask sets? They
> can race etc etc and are 100% unneeded.

The hack is not racy for any mapping calls within the driver since they
are protected by the softc lock.  For any other clients that are
looking at the mask, the worse case is that they end up mapping some
transaction below the 4GB boundary that could have been sent unmapped.

I'm more than happy to change the #ifdefs on the hack to exclude 2.6
so long as all platforms the driver operates under behave as expected.
If 2.4 is any guide, however, there may still be some bogus code lurking
around that uses the "normal" dma mask incorrectly.

--
Justin

Re: aic7xxx strange code

[Arjan van de Ven]

[-- Attachment #1: Type: text/plain, Size: 903 bytes --]

On Wed, Dec 31, 2003 at 11:36:26AM -0700, Justin T. Gibbs wrote:
> > consistent memory ALWAYS comes from lower 4Gb regardless of the
> > pci_set_dma_mask() setting;
> 
> This isn't true on some architectures in the 2.4 kernel.  Look at, for
> example, arch/ia64/hp/common/sba_iommu.c in the RedHat 2.4.18-e.31 kernel.
> This is why this particular hack was added.

Thank you for reporting this very serious bug in our kernel; we will fix it
as soon as possible.

> I'm more than happy to change the #ifdefs on the hack to exclude 2.6
> so long as all platforms the driver operates under behave as expected.
> If 2.4 is any guide, however, there may still be some bogus code lurking
> around that uses the "normal" dma mask incorrectly.

Those platforms break the API and should be fixed as opposed to hacks added
to every single driver in drivers/*

Greetings,
    Arjan van de Ven

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: aic7xxx strange code

[Justin T. Gibbs]

>> I'm more than happy to change the #ifdefs on the hack to exclude 2.6
>> so long as all platforms the driver operates under behave as expected.
>> If 2.4 is any guide, however, there may still be some bogus code lurking
>> around that uses the "normal" dma mask incorrectly.
> 
> Those platforms break the API and should be fixed as opposed to hacks added
> to every single driver in drivers/*

Unfortunately, fixing 2.4 and 2.6 now doesn't help in supporting older
kernels that are still in production.  You can't change the past no
matter how hard we'd like to try. 8-)

--
Justin

Re: aic7xxx strange code

[Arjan van de Ven]

[-- Attachment #1: Type: text/plain, Size: 718 bytes --]

On Thu, Jan 01, 2004 at 09:16:23AM -0700, Justin T. Gibbs wrote:
> >> I'm more than happy to change the #ifdefs on the hack to exclude 2.6
> >> so long as all platforms the driver operates under behave as expected.
> >> If 2.4 is any guide, however, there may still be some bogus code lurking
> >> around that uses the "normal" dma mask incorrectly.
> > 
> > Those platforms break the API and should be fixed as opposed to hacks added
> > to every single driver in drivers/*
> 
> Unfortunately, fixing 2.4 and 2.6 now doesn't help in supporting older
> kernels that are still in production.  You can't change the past no
> matter how hard we'd like to try. 

But vendors will update those kernels.....


[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: aic7xxx strange code

[Justin T. Gibbs]

>> Unfortunately, fixing 2.4 and 2.6 now doesn't help in supporting older
>> kernels that are still in production.  You can't change the past no
>> matter how hard we'd like to try. 
> 
> But vendors will update those kernels.....

Yes, but I can't remove support for these older kernels for quite
some time.  So, I'd be more than happy to update the comments explaining
why the code is there and change the #ifdefs to further limit its use,
but the code needs to remain for the time being.

Are you planning an audit of the other platforms to ensure API compliance?

--
Justin

Re: aic7xxx strange code

[Arjan van de Ven]

[-- Attachment #1: Type: text/plain, Size: 995 bytes --]

On Thu, Jan 01, 2004 at 09:21:38AM -0700, Justin T. Gibbs wrote:
> >> Unfortunately, fixing 2.4 and 2.6 now doesn't help in supporting older
> >> kernels that are still in production.  You can't change the past no
> >> matter how hard we'd like to try. 
> > 
> > But vendors will update those kernels.....
> 
> Yes, but I can't remove support for these older kernels for quite
> some time. 

2.6 seems to do the right thing at least so for your 2.6 drivers you can fix
it.


> Are you planning an audit of the other platforms to ensure API compliance?

This is the first time I've seen this bug (thanks again for reporting it)
and it's clearly an API violation of the ONE API in linux which is very well
documented so no other platform will have copied this bad bug... Adaptect
aic7xxx is not the only card which has the "ringbuffer needs to be in 32 bit
dma space" requirements and no other drivers have workarounds like this, a
sign that the bug isn't actually "common".

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: aic7xxx strange code

[Andi Kleen]

Arjan van de Ven <arjanv@redhat.com> writes:
>
> This is the first time I've seen this bug (thanks again for reporting it)
> and it's clearly an API violation of the ONE API in linux which is very well
> documented so no other platform will have copied this bad bug... Adaptect

It may be well documented, but ports do not follow the documentation
very well in that area. In particular i386 handles the consistent dma mask
completely different than all the 64bit ports. Small overview:

Architecture                  pci_alloc_consistent uses
i386                          normal dma mask
most 64bit archs/old x86-64   hardcoded 0xffffffff
newer IA64/some x86-64        consistent dma mask (2) 
newest x86-64                 normal dma mask & consistent dma mask (1) (2)
old redhat IA64               hardcoded 0xffffffffffffffff (?)

(1) to be bug-to-bug compatible with i386, some drivers rely on that.
(2) consistent dma mask is normally equivalent to hardcoded 0xffffffff
because that is the default value
                                    
It's quite a mess unfortunately. Due to (2) the 64bit architectures
are consistent, except for that old IA64. But i386 doing something
different is quite a problem.

-Andi

Re: aic7xxx strange code

[Arjan van de Ven]

[-- Attachment #1: Type: text/plain, Size: 1592 bytes --]

On Thu, Jan 01, 2004 at 11:31:17PM +0100, Andi Kleen wrote:
> Arjan van de Ven <arjanv@redhat.com> writes:
> >
> > This is the first time I've seen this bug (thanks again for reporting it)
> > and it's clearly an API violation of the ONE API in linux which is very well
> > documented so no other platform will have copied this bad bug... Adaptect
> 
> It may be well documented, but ports do not follow the documentation
> very well in that area. In particular i386 handles the consistent dma mask
> completely different than all the 64bit ports. Small overview:
> 
> Architecture                  pci_alloc_consistent uses
> i386                          normal dma mask
				.... but clipped to 32 bit

> most 64bit archs/old x86-64   hardcoded 0xffffffff
> newer IA64/some x86-64        consistent dma mask (2) 
> newest x86-64                 normal dma mask & consistent dma mask (1) (2)
> old redhat IA64               hardcoded 0xffffffffffffffff (?)

not just old RH; At least one old SuSE too :0 (in fact I found a previous
report claiming this only affected SuSE but it seems we're in the same boat
now)

> 
> (1) to be bug-to-bug compatible with i386, some drivers rely on that.
> (2) consistent dma mask is normally equivalent to hardcoded 0xffffffff
> because that is the default value
>                                     
> It's quite a mess unfortunately. Due to (2) the 64bit architectures
> are consistent, except for that old IA64. But i386 doing something
> different is quite a problem.

but i386 will *never* return > 32 bit at least.

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: aic7xxx strange code

[Andi Kleen]

Arjan van de Ven <arjanv@redhat.com> writes:

> On Thu, Jan 01, 2004 at 11:31:17PM +0100, Andi Kleen wrote:
>> Architecture                  pci_alloc_consistent uses
>> i386                          normal dma mask
> 				.... but clipped to 32 bit
>
>> most 64bit archs/old x86-64   hardcoded 0xffffffff
>> newer IA64/some x86-64        consistent dma mask (2) 
>> newest x86-64                 normal dma mask & consistent dma mask (1) (2)
>> old redhat IA64               hardcoded 0xffffffffffffffff (?)
>
> not just old RH; At least one old SuSE too :0 (in fact I found a previous
> report claiming this only affected SuSE but it seems we're in the same boat
> now)

Ok. Old IA64 in general @) Or rather IA64/HP, right?

>> 
>> (1) to be bug-to-bug compatible with i386, some drivers rely on that.
>> (2) consistent dma mask is normally equivalent to hardcoded 0xffffffff
>> because that is the default value
>>                                     
>> It's quite a mess unfortunately. Due to (2) the 64bit architectures
>> are consistent, except for that old IA64. But i386 doing something
>> different is quite a problem.
>
> but i386 will *never* return > 32 bit at least.

The problem occurs with smaller masks than 32bit (e.g. used by some
sound drivers). The driver sets the normal mask < 32bit and expects
the memory to be allocated GFP_DMA. But it isn't.

The right thing to do would be to set the consistent mask, but that won't
work on i386 without an ifdef :-/

-Andi