diff mbox

[RFC,46/51] ARM: DMA-API: better handing of DMA masks for coherent allocations

Message ID CAL_JsqJH-T3bBNZvQZF2aWS18n5wfSzCMEi20-5e1RmYEfF3Ow@mail.gmail.com (mailing list archive)
State New, archived
Headers show

Commit Message

Rob Herring Aug. 5, 2013, 10:43 p.m. UTC
On Thu, Aug 1, 2013 at 5:20 PM, Russell King
<rmk+kernel@arm.linux.org.uk> wrote:
> We need to start treating DMA masks as something which is specific to
> the bus that the device resides on, otherwise we're going to hit all
> sorts of nasty issues with LPAE and 32-bit DMA controllers in >32-bit
> systems, where memory is offset from PFN 0.
>
> In order to start doing this, we convert the DMA mask to a PFN using
> the device specific dma_to_pfn() macro.  This is the reverse of the
> pfn_to_dma() macro which is used to get the DMA address for the device.
>
> This gives us a PFN mask, which we can then check against the PFN
> limit of the DMA zone.
>
> Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
> ---
>  arch/arm/mm/dma-mapping.c |   49 ++++++++++++++++++++++++++++++++++++++++----
>  arch/arm/mm/init.c        |    2 +
>  arch/arm/mm/mm.h          |    2 +
>  3 files changed, 48 insertions(+), 5 deletions(-)

I believe you missed handling __dma_alloc. I have a different fix than
what Andreas posted. I think DMA zone handling is broken in all cases
here. Feel free to combine this in to your patch if you agree.

Author: Rob Herring <rob.herring@calxeda.com>
Date:   Thu Aug 1 15:51:17 2013 -0500

    ARM: fix dma-mapping on LPAE

    With LPAE, the DMA zone size may be 4GB, so the GFP_DMA flag needs to be
    set when the mask is less than or equal to the arm_dma_limit. This also
    fixes a bug with DMA zone mask handling. GFP_DMA should be set for any
    mask less than or equal to arm_dma_limit, not just less than ~0UL.

    Signed-off-by: Rob Herring <rob.herring@calxeda.com>

        /*

>
> diff --git a/arch/arm/mm/dma-mapping.c b/arch/arm/mm/dma-mapping.c
> index 7f9b179..ef0efab 100644
> --- a/arch/arm/mm/dma-mapping.c
> +++ b/arch/arm/mm/dma-mapping.c
> @@ -173,10 +173,30 @@ static u64 get_coherent_dma_mask(struct device *dev)
>                         return 0;
>                 }
>
> -               if ((~mask) & (u64)arm_dma_limit) {
> -                       dev_warn(dev, "coherent DMA mask %#llx is smaller "
> -                                "than system GFP_DMA mask %#llx\n",
> -                                mask, (u64)arm_dma_limit);
> +               /*
> +                * If the mask allows for more memory than we can address,
> +                * and we actually have that much memory, then fail the
> +                * allocation.
> +                */
> +               if (sizeof(mask) != sizeof(dma_addr_t) &&
> +                   mask > (dma_addr_t)~0 &&
> +                   dma_to_pfn(dev, ~0) > arm_dma_pfn_limit) {
> +                       dev_warn(dev, "Coherent DMA mask %#llx is larger than dma_addr_t allows\n",
> +                                mask);
> +                       dev_warn(dev, "Driver did not use or check the return value from dma_set_coherent_mask()?\n");
> +                       return 0;
> +               }
> +
> +               /*
> +                * Now check that the mask, when translated to a PFN,
> +                * fits within the allowable addresses which we can
> +                * allocate.
> +                */
> +               if (dma_to_pfn(dev, mask) < arm_dma_pfn_limit) {
> +                       dev_warn(dev, "Coherent DMA mask %#llx (pfn %#lx-%#lx) covers a smaller range of system memory than the DMA zone pfn 0x0-%#lx\n",
> +                                mask,
> +                                dma_to_pfn(dev, 0), dma_to_pfn(dev, mask) + 1,
> +                                arm_dma_pfn_limit + 1);
>                         return 0;
>                 }
>         }
> @@ -1008,8 +1028,27 @@ void arm_dma_sync_sg_for_device(struct device *dev, struct scatterlist *sg,
>   */
>  int dma_supported(struct device *dev, u64 mask)
>  {
> -       if (mask < (u64)arm_dma_limit)
> +       unsigned long limit;
> +
> +       /*
> +        * If the mask allows for more memory than we can address,
> +        * and we actually have that much memory, then we must
> +        * indicate that DMA to this device is not supported.
> +        */
> +       if (sizeof(mask) != sizeof(dma_addr_t) &&
> +           mask > (dma_addr_t)~0 &&
> +           dma_to_pfn(dev, ~0) > arm_dma_pfn_limit)
> +               return 0;
> +
> +       /*
> +        * Translate the device's DMA mask to a PFN limit.  This
> +        * PFN number includes the page which we can DMA to.
> +        */
> +       limit = dma_to_pfn(dev, mask);
> +
> +       if (limit < arm_dma_pfn_limit)
>                 return 0;
> +
>         return 1;
>  }
>  EXPORT_SYMBOL(dma_supported);
> diff --git a/arch/arm/mm/init.c b/arch/arm/mm/init.c
> index 15225d8..b5b5836 100644
> --- a/arch/arm/mm/init.c
> +++ b/arch/arm/mm/init.c
> @@ -217,6 +217,7 @@ EXPORT_SYMBOL(arm_dma_zone_size);
>   * so a successful GFP_DMA allocation will always satisfy this.
>   */
>  phys_addr_t arm_dma_limit;
> +unsigned long arm_dma_pfn_limit;
>
>  static void __init arm_adjust_dma_zone(unsigned long *size, unsigned long *hole,
>         unsigned long dma_size)
> @@ -239,6 +240,7 @@ void __init setup_dma_zone(struct machine_desc *mdesc)
>                 arm_dma_limit = PHYS_OFFSET + arm_dma_zone_size - 1;
>         } else
>                 arm_dma_limit = 0xffffffff;
> +       arm_dma_pfn_limit = arm_dma_limit >> PAGE_SHIFT;
>  #endif
>  }
>
> diff --git a/arch/arm/mm/mm.h b/arch/arm/mm/mm.h
> index d5a4e9a..d5a982d 100644
> --- a/arch/arm/mm/mm.h
> +++ b/arch/arm/mm/mm.h
> @@ -81,8 +81,10 @@ extern __init void add_static_vm_early(struct static_vm *svm);
>
>  #ifdef CONFIG_ZONE_DMA
>  extern phys_addr_t arm_dma_limit;
> +extern unsigned long arm_dma_pfn_limit;
>  #else
>  #define arm_dma_limit ((phys_addr_t)~0)
> +#define arm_dma_pfn_limit (~0ul >> PAGE_SHIFT)
>  #endif
>
>  extern phys_addr_t arm_lowmem_limit;
> --
> 1.7.4.4
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Comments

Russell King - ARM Linux Aug. 5, 2013, 11:44 p.m. UTC | #1
On Mon, Aug 05, 2013 at 05:43:47PM -0500, Rob Herring wrote:
> On Thu, Aug 1, 2013 at 5:20 PM, Russell King
> <rmk+kernel@arm.linux.org.uk> wrote:
> > We need to start treating DMA masks as something which is specific to
> > the bus that the device resides on, otherwise we're going to hit all
> > sorts of nasty issues with LPAE and 32-bit DMA controllers in >32-bit
> > systems, where memory is offset from PFN 0.
> >
> > In order to start doing this, we convert the DMA mask to a PFN using
> > the device specific dma_to_pfn() macro.  This is the reverse of the
> > pfn_to_dma() macro which is used to get the DMA address for the device.
> >
> > This gives us a PFN mask, which we can then check against the PFN
> > limit of the DMA zone.
> >
> > Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
> > ---
> >  arch/arm/mm/dma-mapping.c |   49 ++++++++++++++++++++++++++++++++++++++++----
> >  arch/arm/mm/init.c        |    2 +
> >  arch/arm/mm/mm.h          |    2 +
> >  3 files changed, 48 insertions(+), 5 deletions(-)
> 
> I believe you missed handling __dma_alloc. I have a different fix than
> what Andreas posted. I think DMA zone handling is broken in all cases
> here. Feel free to combine this in to your patch if you agree.

I was starting to wonder if whether anyone was going to look at those
patches...

> diff --git a/arch/arm/mm/dma-mapping.c b/arch/arm/mm/dma-mapping.c
> index 7f9b179..3d9bdfb 100644
> --- a/arch/arm/mm/dma-mapping.c
> +++ b/arch/arm/mm/dma-mapping.c
> @@ -651,7 +651,7 @@ static void *__dma_alloc(struct device *dev,
> size_t size, dma_addr_t *handle,
>         if (!mask)
>                 return NULL;
> 
> -       if (mask < 0xffffffffULL)
> +       if (mask <= (u64)arm_dma_limit)
>                 gfp |= GFP_DMA;

I'm not convinced on that - I think you've missed the entire point in
this patch series about what address space the 'mask' is in.  'mask' is
in the device's address space, which may not be the same as the physical
address space.

With LPAE, the two can become quite different address spaces with a
4GB offset between them.  That's why we must stop the old-school
thinking that DMA addresses and physical addresses are the same thing.
We also need to stop doing stuff like passing dma_addr_t variables into
phys_to_virt().  (All those short-cuts are going to break!)

arm_dma_limit is the physical address space.  So, comparing the two
makes no sense what so ever.

However, the use of arm_dma_limit at the top of get_coherent_dma_mask()
is a bug, I think that should just become something like a 24-bit
constant mask, so the NULL device gets GFP_DMA allocations like they
do on x86 for that case.

I also think that 'mask' should be converted to a pfn at the location
you point out before comparing with the amount of memory in the DMA
zone.
diff mbox

Patch

diff --git a/arch/arm/mm/dma-mapping.c b/arch/arm/mm/dma-mapping.c
index 7f9b179..3d9bdfb 100644
--- a/arch/arm/mm/dma-mapping.c
+++ b/arch/arm/mm/dma-mapping.c
@@ -651,7 +651,7 @@  static void *__dma_alloc(struct device *dev,
size_t size, dma_addr_t *handle,
        if (!mask)
                return NULL;

-       if (mask < 0xffffffffULL)
+       if (mask <= (u64)arm_dma_limit)
                gfp |= GFP_DMA;