[PATCH] Stop pci_set_dma_mask() from failing when RAM doesn't exceed the mask anyway

[PATCH] Stop pci_set_dma_mask() from failing when RAM doesn't exceed the mask anyway

[David Woodhouse]

On an iMac G5, the b43 driver is failing to initialise because trying to
set the dma mask to 30-bit fails. Even though there's only 512MiB of RAM
in the machine anyway:
	https://bugzilla.redhat.com/show_bug.cgi?id=514787

We should probably let it succeed if the available RAM in the system
doesn't exceed the requested limit.

Signed-off-by: David Woodhouse <David.Woodhouse@intel.com>

diff --git a/arch/powerpc/kernel/dma.c b/arch/powerpc/kernel/dma.c
index 20a60d6..1769a8e 100644
--- a/arch/powerpc/kernel/dma.c
+++ b/arch/powerpc/kernel/dma.c
@@ -90,11 +90,11 @@ static void dma_direct_unmap_sg(struct device *dev,
struct scatterlist *sg,
 static int dma_direct_dma_supported(struct device *dev, u64 mask)
 {
 #ifdef CONFIG_PPC64
-	/* Could be improved to check for memory though it better be
-	 * done via some global so platforms can set the limit in case
+	extern unsigned long highest_memmap_pfn;
+	/* Could be improved so platforms can set the limit in case
 	 * they have limited DMA windows
 	 */
-	return mask >= DMA_BIT_MASK(32);
+	return (mask >> PAGE_SHIFT) >= highest_memmap_pfn;
 #else
 	return 1;
 #endif

-- 
David Woodhouse                            Open Source Technology Centre
David.Woodhouse@intel.com                              Intel Corporation

Re: [PATCH] Stop pci_set_dma_mask() from failing when RAM doesn't exceed the mask anyway

[Benjamin Herrenschmidt]

On Fri, 2009-07-31 at 20:41 +0100, David Woodhouse wrote:
> On an iMac G5, the b43 driver is failing to initialise because trying to
> set the dma mask to 30-bit fails. Even though there's only 512MiB of RAM
> in the machine anyway:
> 	https://bugzilla.redhat.com/show_bug.cgi?id=514787
> 
> We should probably let it succeed if the available RAM in the system
> doesn't exceed the requested limit.
> 
> Signed-off-by: David Woodhouse <David.Woodhouse@intel.com>

Also, isn't our iommu code smart enough to clamp allocations to the DMA
mask nowadays ? In that case, we could probably just force iommu on
always...

Cheers,
Ben.

> diff --git a/arch/powerpc/kernel/dma.c b/arch/powerpc/kernel/dma.c
> index 20a60d6..1769a8e 100644
> --- a/arch/powerpc/kernel/dma.c
> +++ b/arch/powerpc/kernel/dma.c
> @@ -90,11 +90,11 @@ static void dma_direct_unmap_sg(struct device *dev,
> struct scatterlist *sg,
>  static int dma_direct_dma_supported(struct device *dev, u64 mask)
>  {
>  #ifdef CONFIG_PPC64
> -	/* Could be improved to check for memory though it better be
> -	 * done via some global so platforms can set the limit in case
> +	extern unsigned long highest_memmap_pfn;
> +	/* Could be improved so platforms can set the limit in case
>  	 * they have limited DMA windows
>  	 */
> -	return mask >= DMA_BIT_MASK(32);
> +	return (mask >> PAGE_SHIFT) >= highest_memmap_pfn;
>  #else
>  	return 1;
>  #endif
> 

Re: [PATCH] Stop pci_set_dma_mask() from failing when RAM doesn't exceed the mask anyway

[David Woodhouse]

On Sat, 2009-08-01 at 08:25 +1000, Benjamin Herrenschmidt wrote:
> On Fri, 2009-07-31 at 20:41 +0100, David Woodhouse wrote:
> > On an iMac G5, the b43 driver is failing to initialise because trying to
> > set the dma mask to 30-bit fails. Even though there's only 512MiB of RAM
> > in the machine anyway:
> > 	https://bugzilla.redhat.com/show_bug.cgi?id=514787
> > 
> > We should probably let it succeed if the available RAM in the system
> > doesn't exceed the requested limit.
> > 
> > Signed-off-by: David Woodhouse <David.Woodhouse@intel.com>
> 
> Also, isn't our iommu code smart enough to clamp allocations to the DMA
> mask nowadays ? In that case, we could probably just force iommu on
> always...

We're not using the IOMMU on this box:

PowerMac motherboard: iMac G5
DART: table not allocated, using direct DMA

-- 
David Woodhouse                            Open Source Technology Centre
David.Woodhouse@intel.com                              Intel Corporation

Re: [PATCH] Stop pci_set_dma_mask() from failing when RAM doesn't exceed the mask anyway

[Benjamin Herrenschmidt]

On Sat, 2009-08-01 at 08:54 +0100, David Woodhouse wrote:
> On Sat, 2009-08-01 at 08:25 +1000, Benjamin Herrenschmidt wrote:
> > On Fri, 2009-07-31 at 20:41 +0100, David Woodhouse wrote:
> > > On an iMac G5, the b43 driver is failing to initialise because trying to
> > > set the dma mask to 30-bit fails. Even though there's only 512MiB of RAM
> > > in the machine anyway:
> > > 	https://bugzilla.redhat.com/show_bug.cgi?id=514787
> > > 
> > > We should probably let it succeed if the available RAM in the system
> > > doesn't exceed the requested limit.
> > > 
> > > Signed-off-by: David Woodhouse <David.Woodhouse@intel.com>
> > 
> > Also, isn't our iommu code smart enough to clamp allocations to the DMA
> > mask nowadays ? In that case, we could probably just force iommu on
> > always...
> 
> We're not using the IOMMU on this box:
> 
> PowerMac motherboard: iMac G5
> DART: table not allocated, using direct DMA

I know, I was suggesting we do :-)

Cheers,
Ben.

Re: [PATCH] Stop pci_set_dma_mask() from failing when RAM doesn't exceed the mask anyway

[David Woodhouse]

On Sat, 2009-08-01 at 18:00 +1000, Benjamin Herrenschmidt wrote:
> On Sat, 2009-08-01 at 08:54 +0100, David Woodhouse wrote:
> > On Sat, 2009-08-01 at 08:25 +1000, Benjamin Herrenschmidt wrote:
> > > On Fri, 2009-07-31 at 20:41 +0100, David Woodhouse wrote:
> > > > On an iMac G5, the b43 driver is failing to initialise because trying to
> > > > set the dma mask to 30-bit fails. Even though there's only 512MiB of RAM
> > > > in the machine anyway:
> > > > 	https://bugzilla.redhat.com/show_bug.cgi?id=514787
> > > > 
> > > > We should probably let it succeed if the available RAM in the system
> > > > doesn't exceed the requested limit.
> > > > 
> > > > Signed-off-by: David Woodhouse <David.Woodhouse@intel.com>
> > > 
> > > Also, isn't our iommu code smart enough to clamp allocations to the DMA
> > > mask nowadays ? In that case, we could probably just force iommu on
> > > always...
> > 
> > We're not using the IOMMU on this box:
> > 
> > PowerMac motherboard: iMac G5
> > DART: table not allocated, using direct DMA
> 
> I know, I was suggesting we do :-)

I'm not sure. Losing 16MiB on a machine which only has 512MiB anyway
doesn't seem ideal, and we'll want to make the no-iommu code DTRT
_anyway_, surely?

So we might as well let the DART keep its existing logic (which is only
to bother if we have more than 1GiB of RAM; a limit chosen specifically
because of the Broadcom brokenness).

-- 
David Woodhouse                            Open Source Technology Centre
David.Woodhouse@intel.com                              Intel Corporation

Re: [PATCH] Stop pci_set_dma_mask() from failing when RAM doesn't exceed the mask anyway

[Benjamin Herrenschmidt]

On Sat, 2009-08-01 at 10:00 +0100, David Woodhouse wrote:
> I'm not sure. Losing 16MiB on a machine which only has 512MiB anyway
> doesn't seem ideal, and we'll want to make the no-iommu code DTRT
> _anyway_, surely?
>
> So we might as well let the DART keep its existing logic (which is
> only
> to bother if we have more than 1GiB of RAM; 

Ah right, so when do we enable the DART ? Above 1G ? I though it was
above 2G but we may well have moved that down to 1G just for b43 indeed.

I definitely agree on the fix to the mask so it only compares to the
available RAM. I'll check that in when I'm back from the snow fields 
on tuesday :-)

Cheers,
Ben,

Re: [PATCH] Stop pci_set_dma_mask() from failing when RAM doesn't exceed the mask anyway

[David Woodhouse]

On Sun, 2009-08-02 at 17:50 +1000, Benjamin Herrenschmidt wrote:
> On Sat, 2009-08-01 at 10:00 +0100, David Woodhouse wrote:
> > I'm not sure. Losing 16MiB on a machine which only has 512MiB anyway
> > doesn't seem ideal, and we'll want to make the no-iommu code DTRT
> > _anyway_, surely?
> >
> > So we might as well let the DART keep its existing logic (which is
> > only
> > to bother if we have more than 1GiB of RAM; 
> 
> Ah right, so when do we enable the DART ? Above 1G ? I though it was
> above 2G but we may well have moved that down to 1G just for b43 indeed.

void __init alloc_dart_table(void)
{
        /* Only reserve DART space if machine has more than 1GB of RAM
         * or if requested with iommu=on on cmdline.
         *
         * 1GB of RAM is picked as limit because some default devices
         * (i.e. Airport Extreme) have 30 bit address range limits.
         */

        if (iommu_is_off)
                return;

        if (!iommu_force_on && lmb_end_of_DRAM() <= 0x40000000ull)
                return;


> I definitely agree on the fix to the mask so it only compares to the
> available RAM. I'll check that in when I'm back from the snow fields 
> on tuesday :-)

I see one potential failure mode with this. You need:
 - No IOMMU
 - Crappy devices
 - Hotpluggable memory
 - Boot with only "low" memory, and allow a pci_set_dma_mask() to
   succeed because you don't have that much memory anyway.
 - Hotplug some "high" memory that the crappy device can't reach.

Do we care about that scenario? I think we might be able to "fix" it by
setting the memory_limit when we allow pci_set_dma_mask() to succeed?
That will effectively prevent the addition of memory that our crappy
device can't reach, won't it?

-- 
David Woodhouse                            Open Source Technology Centre
David.Woodhouse@intel.com                              Intel Corporation

Re: [PATCH] Stop pci_set_dma_mask() from failing when RAM doesn't exceed the mask anyway

[Benjamin Herrenschmidt]

On Mon, 2009-08-03 at 14:14 +0100, David Woodhouse wrote:

> Do we care about that scenario? I think we might be able to "fix" it by
> setting the memory_limit when we allow pci_set_dma_mask() to succeed?
> That will effectively prevent the addition of memory that our crappy
> device can't reach, won't it?

We don't support hotplug memory on those machines anyway, do we ?

Cheers,
Ben.

Re: [PATCH] Stop pci_set_dma_mask() from failing when RAM doesn't exceed the mask anyway

[Benjamin Herrenschmidt]

On Fri, 2009-07-31 at 20:41 +0100, David Woodhouse wrote:
> On an iMac G5, the b43 driver is failing to initialise because trying to
> set the dma mask to 30-bit fails. Even though there's only 512MiB of RAM
> in the machine anyway:
> 	https://bugzilla.redhat.com/show_bug.cgi?id=514787
> 
> We should probably let it succeed if the available RAM in the system
> doesn't exceed the requested limit.

I've slightly modified the patch to use lmb_end_of_DRAM() instead
of highest_memmap_pfn because the later is an mm internal.h thing,
it looked too fishy to rely on it form arch code with a local
"extern" in the C code :-) It's not going to cause much more overhead
and it's not a hot path anyway afaik.

Below is a copy of what I've commited to my merge branch and will
ask Linus to pull in a few minutes.

>From 10ba96df690618632a8786494b2359f5ee350048 Mon Sep 17 00:00:00 2001
From: David Woodhouse <dwmw2@infradead.org>
Date: Fri, 31 Jul 2009 09:41:50 +0000
Subject: [PATCH] powerpc/dma: pci_set_dma_mask() shouldn't fail if mask fits in RAM

On an iMac G5, the b43 driver is failing to initialise because trying to
set the dma mask to 30-bit fails. Even though there's only 512MiB of RAM
in the machine anyway:
	https://bugzilla.redhat.com/show_bug.cgi?id=514787

We should probably let it succeed if the available RAM in the system
doesn't exceed the requested limit.

Signed-off-by: David Woodhouse <David.Woodhouse@intel.com>
Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
---
 arch/powerpc/kernel/dma.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/arch/powerpc/kernel/dma.c b/arch/powerpc/kernel/dma.c
index 20a60d6..ccf129d 100644
--- a/arch/powerpc/kernel/dma.c
+++ b/arch/powerpc/kernel/dma.c
@@ -7,6 +7,7 @@
 
 #include <linux/device.h>
 #include <linux/dma-mapping.h>
+#include <linux/lmb.h>
 #include <asm/bug.h>
 #include <asm/abs_addr.h>
 
@@ -90,11 +91,10 @@ static void dma_direct_unmap_sg(struct device *dev, struct scatterlist *sg,
 static int dma_direct_dma_supported(struct device *dev, u64 mask)
 {
 #ifdef CONFIG_PPC64
-	/* Could be improved to check for memory though it better be
-	 * done via some global so platforms can set the limit in case
+	/* Could be improved so platforms can set the limit in case
 	 * they have limited DMA windows
 	 */
-	return mask >= DMA_BIT_MASK(32);
+	return mask >= (lmb_end_of_DRAM() - 1);
 #else
 	return 1;
 #endif
-- 
1.6.1.2.14.gf26b5