Patchwork Stop pci_set_dma_mask() from failing when RAM doesn't exceed the mask anyway

login
register
mail settings
Submitter David Woodhouse
Date July 31, 2009, 7:41 p.m.
Message ID <1249069310.20192.220.camel@macbook.infradead.org>
Download mbox | patch
Permalink /patch/30444/
State Superseded
Headers show

Comments

David Woodhouse - July 31, 2009, 7:41 p.m.
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>

 #endif
Benjamin Herrenschmidt - July 31, 2009, 10:25 p.m.
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
>
David Woodhouse - Aug. 1, 2009, 7:54 a.m.
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
Benjamin Herrenschmidt - Aug. 1, 2009, 8 a.m.
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.
David Woodhouse - Aug. 1, 2009, 9 a.m.
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).
Benjamin Herrenschmidt - Aug. 2, 2009, 7:50 a.m.
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,
David Woodhouse - Aug. 3, 2009, 1:14 p.m.
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?
Benjamin Herrenschmidt - Aug. 3, 2009, 9:05 p.m.
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.

Patch

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;