[RFC/PATCH] remove unneeded check in bcm43xx

[RFC/PATCH] remove unneeded check in bcm43xx

[Benoit Boissinot]

Since the driver already sets the correct dma_mask, there is no reason
to bail there. In fact if you have an iommu, I think you can have a
address above 1G which will be ok for the device (if it isn't true then
the powerpc dma_alloc_coherent with iommu needs to be fixed because it
doesn't respect the the dma_mask).

Please comment or apply.

Signed-off-by: Benoit Boissinot <benoit.boissinot@ens-lyon.fr>

Index: linux/drivers/net/wireless/bcm43xx/bcm43xx_dma.c
===================================================================
--- linux.orig/drivers/net/wireless/bcm43xx/bcm43xx_dma.c
+++ linux/drivers/net/wireless/bcm43xx/bcm43xx_dma.c
@@ -194,14 +194,6 @@ static int alloc_ringmemory(struct bcm43
 		printk(KERN_ERR PFX "DMA ringmemory allocation failed\n");
 		return -ENOMEM;
 	}
-	if (ring->dmabase + BCM43xx_DMA_RINGMEMSIZE > BCM43xx_DMA_BUSADDRMAX) {
-		printk(KERN_ERR PFX ">>>FATAL ERROR<<<  DMA RINGMEMORY >1G "
-				    "(0x%08x, len: %lu)\n",
-		       ring->dmabase, BCM43xx_DMA_RINGMEMSIZE);
-		dma_free_coherent(dev, BCM43xx_DMA_RINGMEMSIZE,
-				  ring->vbase, ring->dmabase);
-		return -ENOMEM;
-	}
 	assert(!(ring->dmabase & 0x000003FF));
 	memset(ring->vbase, 0, BCM43xx_DMA_RINGMEMSIZE);
 
@@ -303,14 +295,6 @@ static int setup_rx_descbuffer(struct bc
 	if (unlikely(!skb))
 		return -ENOMEM;
 	dmaaddr = map_descbuffer(ring, skb->data, ring->rx_buffersize, 0);
-	if (unlikely(dmaaddr + ring->rx_buffersize > BCM43xx_DMA_BUSADDRMAX)) {
-		unmap_descbuffer(ring, dmaaddr, ring->rx_buffersize, 0);
-		dev_kfree_skb_any(skb);
-		printk(KERN_ERR PFX ">>>FATAL ERROR<<<  DMA RX SKB >1G "
-				    "(0x%08x, len: %u)\n",
-		       dmaaddr, ring->rx_buffersize);
-		return -ENOMEM;
-	}
 	meta->skb = skb;
 	meta->dmaaddr = dmaaddr;
 	skb->dev = ring->bcm->net_dev;
@@ -726,13 +710,6 @@ static int dma_tx_fragment(struct bcm43x
 
 	meta->skb = skb;
 	meta->dmaaddr = map_descbuffer(ring, skb->data, skb->len, 1);
-	if (unlikely(meta->dmaaddr + skb->len > BCM43xx_DMA_BUSADDRMAX)) {
-		return_slot(ring, slot);
-		printk(KERN_ERR PFX ">>>FATAL ERROR<<<  DMA TX SKB >1G "
-				    "(0x%08x, len: %u)\n",
-		       meta->dmaaddr, skb->len);
-		return -ENOMEM;
-	}
 
 	desc_addr = (u32)(meta->dmaaddr + ring->memoffset);
 	desc_ctl = BCM43xx_DMADTOR_FRAMESTART | BCM43xx_DMADTOR_FRAMEEND;

Re: [RFC/PATCH] remove unneeded check in bcm43xx

[Michael Buesch]

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

On Monday 10 April 2006 06:01, you wrote:
> Since the driver already sets the correct dma_mask, there is no reason
> to bail there. In fact if you have an iommu, I think you can have a
> address above 1G which will be ok for the device (if it isn't true then
> the powerpc dma_alloc_coherent with iommu needs to be fixed because it
> doesn't respect the the dma_mask).
> 
> Please comment or apply.

NACK. Don't apply that patch.
I know it is odd, but people are actually hitting these messages.
Maybe benh can explain the issues. I don't know...

-- 
Greetings Michael.

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

Re: [RFC/PATCH] remove unneeded check in bcm43xx

[Benoit Boissinot]

On Mon, Apr 10, 2006 at 06:07:32AM +0200, Michael Buesch wrote:
> On Monday 10 April 2006 06:01, you wrote:
> > Since the driver already sets the correct dma_mask, there is no reason
> > to bail there. In fact if you have an iommu, I think you can have a
> > address above 1G which will be ok for the device (if it isn't true then
> > the powerpc dma_alloc_coherent with iommu needs to be fixed because it
> > doesn't respect the the dma_mask).
> > 
> > Please comment or apply.
> 
> NACK. Don't apply that patch.
> I know it is odd, but people are actually hitting these messages.
> Maybe benh can explain the issues. I don't know...
> 
Yes, I know they hit the message, that's from a message in some forum
that i got interested in the issue. It probably comes from an allocation
from:
http://www.linux-m32r.org/lxr/http/source/arch/powerpc/kernel/pci_direct_iommu.c#L32

Either the ppc code is wrong (it doesn't enforce dma_mask) either the
driver still works without the check.

Maybe ppc should do the same thing as i386:

47         if (dev == NULL || (dev->coherent_dma_mask < 0xffffffff))
48                 gfp |= GFP_DMA;

thanks,

Benoit

-- 
powered by bash/screen/(urxvt/fvwm|linux-console)/gentoo/gnu/linux OS

Re: [RFC/PATCH] remove unneeded check in bcm43xx

[Michael Buesch]

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

On Monday 10 April 2006 06:22, you wrote:
> Either the ppc code is wrong (it doesn't enforce dma_mask) either the
> driver still works without the check.
> 
> Maybe ppc should do the same thing as i386:
> 
> 47         if (dev == NULL || (dev->coherent_dma_mask < 0xffffffff))
> 48                 gfp |= GFP_DMA;

No, GFP_DMA is a NOP on PPC.
Actually the problems seems much more complex and a correct fix
seems to be hard to do.
I think benh is actually fixing this.

To summerize: I actually added these messages, because people were
hitting "this does not work with >1G" issues and did not get an error message.
So I decided to insert warnings until the issue is fixed inside the arch code.
I will remove them once the issue is fixed.

-- 
Greetings Michael.

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

Re: [RFC/PATCH] remove unneeded check in bcm43xx

[Benoit Boissinot]

On Mon, Apr 10, 2006 at 06:28:00AM +0200, Michael Buesch wrote:
> On Monday 10 April 2006 06:22, you wrote:
> > Either the ppc code is wrong (it doesn't enforce dma_mask) either the
> > driver still works without the check.
> > 
> > Maybe ppc should do the same thing as i386:
> > 
> > 47         if (dev == NULL || (dev->coherent_dma_mask < 0xffffffff))
> > 48                 gfp |= GFP_DMA;
> 
> No, GFP_DMA is a NOP on PPC.
> Actually the problems seems much more complex and a correct fix
> seems to be hard to do.
> I think benh is actually fixing this.
> 
> To summerize: I actually added these messages, because people were
> hitting "this does not work with >1G" issues and did not get an error message.
> So I decided to insert warnings until the issue is fixed inside the arch code.
> I will remove them once the issue is fixed.
>

Thanks for the explainations.

Benoit


-- 
powered by bash/screen/(urxvt/fvwm|linux-console)/gentoo/gnu/linux OS

Re: [RFC/PATCH] remove unneeded check in bcm43xx

[John W. Linville]

On Mon, Apr 10, 2006 at 06:28:00AM +0200, Michael Buesch wrote:

> To summerize: I actually added these messages, because people were
> hitting "this does not work with >1G" issues and did not get an error message.
> So I decided to insert warnings until the issue is fixed inside the arch code.
> I will remove them once the issue is fixed.

This sounds like the same sort of problems w/ the b44 driver.
I surmise that both use the same (broken) DMA engine from Broadcom.

Unfortunately, I don't know of any good solution to this.  There are
a few hacks in b44 that deal with the issue.  I don't like them,
although I am the perpetrator of at least one of them.  It might be
worth looking at what was done there?

YMMV...

John
-- 
John W. Linville
linville@tuxdriver.com

Re: [RFC/PATCH] remove unneeded check in bcm43xx

[Arjan van de Ven]

On Mon, 2006-04-10 at 09:46 -0400, John W. Linville wrote:
> On Mon, Apr 10, 2006 at 06:28:00AM +0200, Michael Buesch wrote:
> 
> > To summerize: I actually added these messages, because people were
> > hitting "this does not work with >1G" issues and did not get an error message.
> > So I decided to insert warnings until the issue is fixed inside the arch code.
> > I will remove them once the issue is fixed.
> 
> This sounds like the same sort of problems w/ the b44 driver.
> I surmise that both use the same (broken) DMA engine from Broadcom.
> 
> Unfortunately, I don't know of any good solution to this.  There are
> a few hacks in b44 that deal with the issue.  I don't like them,
> although I am the perpetrator of at least one of them.  It might be
> worth looking at what was done there?

or as better solution we should give i386 the swiommu code from
x86-64 ;)

Re: [RFC/PATCH] remove unneeded check in bcm43xx

[Benoit Boissinot]

On Mon, Apr 10, 2006 at 09:46:30AM -0400, John W. Linville wrote:
> On Mon, Apr 10, 2006 at 06:28:00AM +0200, Michael Buesch wrote:
> 
> > To summerize: I actually added these messages, because people were
> > hitting "this does not work with >1G" issues and did not get an error message.
> > So I decided to insert warnings until the issue is fixed inside the arch code.
> > I will remove them once the issue is fixed.
> 
> This sounds like the same sort of problems w/ the b44 driver.
> I surmise that both use the same (broken) DMA engine from Broadcom.
> 
> Unfortunately, I don't know of any good solution to this.  There are
> a few hacks in b44 that deal with the issue.  I don't like them,
> although I am the perpetrator of at least one of them.  It might be
> worth looking at what was done there?

The hacks i see there is reallocating a buffer with GFP_DMA, so that
means that if the ppc dma_alloc_coherent did the same thing as the i386
counterpart (adding GFP_DMA if dma_mask is less than 32bits) it should
work, no ?

regards,

Benoit

ps: i do not have the hardware so i sadly cannot test.

-- 
powered by bash/screen/(urxvt/fvwm|linux-console)/gentoo/gnu/linux OS

Re: [RFC/PATCH] remove unneeded check in bcm43xx

[David S. Miller]

From: Benoit Boissinot <benoit.boissinot@ens-lyon.org>
Date: Tue, 11 Apr 2006 00:13:59 +0200

> On Mon, Apr 10, 2006 at 09:46:30AM -0400, John W. Linville wrote:
> > On Mon, Apr 10, 2006 at 06:28:00AM +0200, Michael Buesch wrote:
> > 
> > > To summerize: I actually added these messages, because people were
> > > hitting "this does not work with >1G" issues and did not get an error message.
> > > So I decided to insert warnings until the issue is fixed inside the arch code.
> > > I will remove them once the issue is fixed.
> > 
> > This sounds like the same sort of problems w/ the b44 driver.
> > I surmise that both use the same (broken) DMA engine from Broadcom.
> > 
> > Unfortunately, I don't know of any good solution to this.  There are
> > a few hacks in b44 that deal with the issue.  I don't like them,
> > although I am the perpetrator of at least one of them.  It might be
> > worth looking at what was done there?
> 
> The hacks i see there is reallocating a buffer with GFP_DMA, so that
> means that if the ppc dma_alloc_coherent did the same thing as the i386
> counterpart (adding GFP_DMA if dma_mask is less than 32bits) it should
> work, no ?

PowerPC doesn't have a seperate GFP_DMA page pool, so this won't work.

Re: [RFC/PATCH] remove unneeded check in bcm43xx

[Benjamin Herrenschmidt]

> The hacks i see there is reallocating a buffer with GFP_DMA, so that
> means that if the ppc dma_alloc_coherent did the same thing as the i386
> counterpart (adding GFP_DMA if dma_mask is less than 32bits) it should
> work, no ?

Except that GFP_DMA covers the whole address space on ppc64...

Ben.

Re: [RFC/PATCH] remove unneeded check in bcm43xx

[Benjamin Herrenschmidt]

> Yes, I know they hit the message, that's from a message in some forum
> that i got interested in the issue. It probably comes from an allocation
> from:
> http://www.linux-m32r.org/lxr/http/source/arch/powerpc/kernel/pci_direct_iommu.c#L32
> 
> Either the ppc code is wrong (it doesn't enforce dma_mask) either the
> driver still works without the check.
> 
> Maybe ppc should do the same thing as i386:
> 
> 47         if (dev == NULL || (dev->coherent_dma_mask < 0xffffffff))
> 48                 gfp |= GFP_DMA;

PPC doesn't have a ZONE_DMA... PPC assumes that the whole memory is
DMA'able... What happens here is that it works by "chance" on x86 ;)
ZONE_DMA is a thing of the past that represents ISA DMA (below 16M iirc
or something like that). Thus x86 historically maintained a separate
allocation zone down there for ISA devices. What happens here is that
the x86 code sort-of detects that the DMA mask isn't the full 32 bits
and kicks everything down into ZONE_DMA. Makes things work, though the
pressure on ZONE_DMA can often be high enough that you'll have a lot of
failed allocations ...

PPC doesn't have ZONE_DMA, thus can't provide memory guaranteed to be
below that limit.

Now, for ppc32, it should still sort-of work because all of lowmem is
below 1Gb and people generally don't hack their lowmem size (well, I do
but heh, that doesn't count :) and I don't think you'll get skb's in
highmem. But ppc64 hits the problem and at this point, there is nothing
I can do other than either implementing a split zone allocation mecanism
in the ppc64 architecture for the sole sake of bcm43xx (ick !) or doing
some trick with the iommu...

Ben.

Re: [RFC/PATCH] remove unneeded check in bcm43xx

[Michael Buesch]

On Tuesday 11 April 2006 03:46, you wrote:
> 
> > Yes, I know they hit the message, that's from a message in some forum
> > that i got interested in the issue. It probably comes from an allocation
> > from:
> > http://www.linux-m32r.org/lxr/http/source/arch/powerpc/kernel/pci_direct_iommu.c#L32
> > 
> > Either the ppc code is wrong (it doesn't enforce dma_mask) either the
> > driver still works without the check.
> > 
> > Maybe ppc should do the same thing as i386:
> > 
> > 47         if (dev == NULL || (dev->coherent_dma_mask < 0xffffffff))
> > 48                 gfp |= GFP_DMA;
> 
> PPC doesn't have a ZONE_DMA... PPC assumes that the whole memory is
> DMA'able... What happens here is that it works by "chance" on x86 ;)
> ZONE_DMA is a thing of the past that represents ISA DMA (below 16M iirc
> or something like that). Thus x86 historically maintained a separate
> allocation zone down there for ISA devices. What happens here is that
> the x86 code sort-of detects that the DMA mask isn't the full 32 bits
> and kicks everything down into ZONE_DMA. Makes things work, though the
> pressure on ZONE_DMA can often be high enough that you'll have a lot of
> failed allocations ...
> 
> PPC doesn't have ZONE_DMA, thus can't provide memory guaranteed to be
> below that limit.
> 
> Now, for ppc32, it should still sort-of work because all of lowmem is
> below 1Gb and people generally don't hack their lowmem size (well, I do
> but heh, that doesn't count :) and I don't think you'll get skb's in
> highmem. But ppc64 hits the problem and at this point, there is nothing
> I can do other than either implementing a split zone allocation mecanism
> in the ppc64 architecture

> for the sole sake of bcm43xx (ick !)

Nope. For every broadcom device, which has this stupid DMA engine.
That is b44 and bcm43xx, as far as I can tell. But likely there are more.

> or doing some trick with the iommu...

-- 
Greetings Michael.

Re: [RFC/PATCH] remove unneeded check in bcm43xx

[Benoit Boissinot]

On Tue, Apr 11, 2006 at 03:53:51AM +0200, Michael Buesch wrote:
> On Tuesday 11 April 2006 03:46, you wrote:
> > 
> > 
> > Now, for ppc32, it should still sort-of work because all of lowmem is
> > below 1Gb and people generally don't hack their lowmem size (well, I do
> > but heh, that doesn't count :) and I don't think you'll get skb's in
> > highmem. But ppc64 hits the problem and at this point, there is nothing
> > I can do other than either implementing a split zone allocation mecanism
> > in the ppc64 architecture
> 
> > for the sole sake of bcm43xx (ick !)
> 
> Nope. For every broadcom device, which has this stupid DMA engine.
> That is b44 and bcm43xx, as far as I can tell. But likely there are more.

On the other hand, bcm43xx looks very common with apple hardware, so there
are probably a lot of users who cannot use their wifi card in G5's.

regards,

Benoit
-- 
powered by bash/screen/(urxvt/fvwm|linux-console)/gentoo/gnu/linux OS

Re: [RFC/PATCH] remove unneeded check in bcm43xx

[David S. Miller]

From: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Date: Tue, 11 Apr 2006 11:46:12 +1000

> But ppc64 hits the problem and at this point, there is nothing
> I can do other than either implementing a split zone allocation mecanism
> in the ppc64 architecture for the sole sake of bcm43xx (ick !) or doing
> some trick with the iommu...

I think allowing DMA mask range limiting in the IOMMU layer is going
to set a very bad precedence, just don't do it.

It's 2006, we should be way past the era of not putting the full 32
PCI DMA address bits in devices.  In this day and age it is simply
inexscusable.

Maybe we could understand chips coming out 8 years ago when a lot of
designs were transitioning from ISA to PCI, but that no longer applies
in any way today.

Re: [RFC/PATCH] remove unneeded check in bcm43xx

[Michael Buesch]

On Tuesday 11 April 2006 07:49, you wrote:
> From: Benjamin Herrenschmidt <benh-XVmvHMARGAS8U2dJNN8I7kB+6BGkLq7r@public.gmane.org>
> Date: Tue, 11 Apr 2006 11:46:12 +1000
> 
> > But ppc64 hits the problem and at this point, there is nothing
> > I can do other than either implementing a split zone allocation mecanism
> > in the ppc64 architecture for the sole sake of bcm43xx (ick !) or doing
> > some trick with the iommu...
> 
> I think allowing DMA mask range limiting in the IOMMU layer is going
> to set a very bad precedence, just don't do it.
> 
> It's 2006, we should be way past the era of not putting the full 32
> PCI DMA address bits in devices.  In this day and age it is simply
> inexscusable.

Sure, but we are in 2006 and actually _have_ at least two of those
devices, which are in usage by many people.
We can't say "vendors must fix this", because lots of devices are
out there.
It must be worked around in the kernel.

-- 
Greetings Michael.

Re: [RFC/PATCH] remove unneeded check in bcm43xx

[Benjamin Herrenschmidt]

> I think allowing DMA mask range limiting in the IOMMU layer is going
> to set a very bad precedence, just don't do it.
> 
> It's 2006, we should be way past the era of not putting the full 32
> PCI DMA address bits in devices.  In this day and age it is simply
> inexscusable.
> 
> Maybe we could understand chips coming out 8 years ago when a lot of
> designs were transitioning from ISA to PCI, but that no longer applies
> in any way today.

I would tend to agree... except that the broadcom is _the_ wireless card
shipped by Apple with all of their machines for the last few years, and
thus, the problem will be hit by pretty much any G5 user trying to use
theirs...

I don't have another idea on how to fix that at hand... a dma mask limit
in the iommu layer is fairly easy to implement with our iommu
implementation (though it wouldn't work on pseries where ranges are
allocated per slot, but it would work fine on a g5). Still sounds better
than introducing a ZONE_DMA separate from ZONE_NORMAL ...

Ben.

Re: [RFC/PATCH] remove unneeded check in bcm43xx

[David S. Miller]

From: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Date: Wed, 12 Apr 2006 06:49:00 +1000

> I would tend to agree... except that the broadcom is _the_ wireless
> card shipped by Apple with all of their machines for the last few
> years, and thus, the problem will be hit by pretty much any G5 user
> trying to use theirs...
>
> I don't have another idea on how to fix that at hand... a dma mask
> limit in the iommu layer is fairly easy to implement with our iommu
> implementation (though it wouldn't work on pseries where ranges are
> allocated per slot, but it would work fine on a g5). Still sounds
> better than introducing a ZONE_DMA separate from ZONE_NORMAL ...

I still think we shouldn't reward shit hardware by complicating
up our DMA mappings internals. :-)

If you're going to do it anyways, we use a nearly identical
allocation bitmap algorithm under sparc64 so maybe you can
take a stab at trying to put your code there too.  I'm more
than happy to test and integrate.

Thanks.

Re: [RFC/PATCH] remove unneeded check in bcm43xx

[Benjamin Herrenschmidt]

On Tue, 2006-04-11 at 14:34 -0700, David S. Miller wrote:

> I still think we shouldn't reward shit hardware by complicating
> up our DMA mappings internals. :-)

Heh, it's a good point but in that specific case, it's a bit difficult
to tell that to users who don't have a choice of what card to put in
apple proprietary slot ... =P

> If you're going to do it anyways, we use a nearly identical
> allocation bitmap algorithm under sparc64 so maybe you can
> take a stab at trying to put your code there too.  I'm more
> than happy to test and integrate.

I may give it a go later this week. I don't have a wireless card in my
g5 so I'll need to ping-pong the patch with testers first tho.

Ben.

Re: [RFC/PATCH] remove unneeded check in bcm43xx

[Benjamin Herrenschmidt]

> I still think we shouldn't reward shit hardware by complicating
> up our DMA mappings internals. :-)

BTW. In the meantime, can't that driver work in PIO only mode ?

Ben.

Re: [RFC/PATCH] remove unneeded check in bcm43xx

[Benoit Boissinot]

On Wed, Apr 12, 2006 at 08:21:17AM +1000, Benjamin Herrenschmidt wrote:
> 
> > I still think we shouldn't reward shit hardware by complicating
> > up our DMA mappings internals. :-)
> 
> BTW. In the meantime, can't that driver work in PIO only mode ?

yes, I think you just have to have the pci_set_dma_mask fail with
DMA30BIT_MASK.

regards,

Benoit

-- 
powered by bash/screen/(urxvt/fvwm|linux-console)/gentoo/gnu/linux OS

Re: [RFC/PATCH] remove unneeded check in bcm43xx

[Benjamin Herrenschmidt]

On Wed, 2006-04-12 at 00:30 +0200, Benoit Boissinot wrote:
> On Wed, Apr 12, 2006 at 08:21:17AM +1000, Benjamin Herrenschmidt wrote:
> > 
> > > I still think we shouldn't reward shit hardware by complicating
> > > up our DMA mappings internals. :-)
> > 
> > BTW. In the meantime, can't that driver work in PIO only mode ?
> 
> yes, I think you just have to have the pci_set_dma_mask fail with
> DMA30BIT_MASK.

Ok, _that_ makes sense in fact to have ppc do that when the mask is too
big... now the problem is should I compare the mask to the available
RAM ? That is, there is little point in failing for a 30 bits mask if
hte machine only has 512M of RAM.

By extension of the above, what to do on 32 bits kernels, should I test
the mask against total memory or specifically against lowmem ? There is
no clear answer here as some drivers will get highmem pages for DMA,
just not network drivers afaik (block drivers will), though I can't be
sure what will happen with thing like nbd... I'm not familiar enough
with the network stack. It would be sad to have 32 bits laptop switch to
PIO when they have too much RAM if, in practice, skbs are always only in
lowmem..

I think for now, what I may do is just add such a test for ppc64 and not
ppc32 and will talk to paulus see if he happens to have a better idea.

It's all very sad that bcm gets away with such crap though and that so
many vendors just bought it without complaining...

Ben.

Re: [RFC/PATCH] remove unneeded check in bcm43xx

[Michael Buesch]

On Wednesday 12 April 2006 00:21, you wrote:
> 
> > I still think we shouldn't reward shit hardware by complicating
> > up our DMA mappings internals. :-)
> 
> BTW. In the meantime, can't that driver work in PIO only mode ?

No, because I did not finish debugging that, yet. ;)
And a second half-no, because some newer chips don't support
PIO anymore (4318 for example).

-- 
Greetings Michael.