diff mbox

[V3] of: Set the DMA mask to 64 bits when dma_addr_t is 64-bits

Message ID 1367008266-1431-1-git-send-email-lauraa@codeaurora.org (mailing list archive)
State New, archived
Headers show

Commit Message

Laura Abbott April 26, 2013, 8:31 p.m. UTC
Currently, of_platform_device_create_pdata always sets the
coherent DMA mask to 32 bits. On ARM systems without CONFIG_ZONE_DMA,
arm_dma_limit gets set to ~0 or 0xFFFFFFFFFFFFFFFF on LPAE based
systems. Since arm_dma_limit represents the smallest dma_mask
on the system, the default of 32 bits prevents any dma_coherent
allocation from succeeding unless clients manually set the
dma mask first. Rather than make every client on an LPAE system set
the mask manually, account for the size of dma_addr_t when setting
the coherent mask.

Signed-off-by: Laura Abbott <lauraa@codeaurora.org>
---
 drivers/of/platform.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

Comments

Catalin Marinas April 26, 2013, 8:52 p.m. UTC | #1
On Fri, Apr 26, 2013 at 09:31:06PM +0100, Laura Abbott wrote:
> Currently, of_platform_device_create_pdata always sets the
> coherent DMA mask to 32 bits. On ARM systems without CONFIG_ZONE_DMA,
> arm_dma_limit gets set to ~0 or 0xFFFFFFFFFFFFFFFF on LPAE based
> systems. Since arm_dma_limit represents the smallest dma_mask
> on the system, the default of 32 bits prevents any dma_coherent
> allocation from succeeding unless clients manually set the
> dma mask first. Rather than make every client on an LPAE system set
> the mask manually, account for the size of dma_addr_t when setting
> the coherent mask.
> 
> Signed-off-by: Laura Abbott <lauraa@codeaurora.org>

Looks good.

Acked-by: Catalin Marinas <catalin.marinas@arm.com>
Rob Herring April 26, 2013, 9:32 p.m. UTC | #2
On 04/26/2013 03:31 PM, Laura Abbott wrote:
> Currently, of_platform_device_create_pdata always sets the
> coherent DMA mask to 32 bits. On ARM systems without CONFIG_ZONE_DMA,
> arm_dma_limit gets set to ~0 or 0xFFFFFFFFFFFFFFFF on LPAE based
> systems. Since arm_dma_limit represents the smallest dma_mask
> on the system, the default of 32 bits prevents any dma_coherent
> allocation from succeeding unless clients manually set the
> dma mask first. Rather than make every client on an LPAE system set
> the mask manually, account for the size of dma_addr_t when setting
> the coherent mask.
> 
> Signed-off-by: Laura Abbott <lauraa@codeaurora.org>
> ---
>  drivers/of/platform.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/of/platform.c b/drivers/of/platform.c
> index 0970505..5f0ba94 100644
> --- a/drivers/of/platform.c
> +++ b/drivers/of/platform.c
> @@ -214,7 +214,7 @@ struct platform_device *of_platform_device_create_pdata(
>  #if defined(CONFIG_MICROBLAZE)
>  	dev->archdata.dma_mask = 0xffffffffUL;
>  #endif
> -	dev->dev.coherent_dma_mask = DMA_BIT_MASK(32);
> +	dev->dev.coherent_dma_mask = DMA_BIT_MASK(sizeof(dma_addr_t) * 8);

This is going to change the mask from 32 to 64 bits on 64-bit powerpc
and others possibly. Maybe it doesn't matter. I think it doesn't, but
I'm not sure enough to apply for 3.10. So I'll queue it for 3.11.

Rob


>  	dev->dev.bus = &platform_bus_type;
>  	dev->dev.platform_data = platform_data;
>  
>
Ming Lei July 3, 2013, 2:15 p.m. UTC | #3
On Sat, Apr 27, 2013 at 5:32 AM, Rob Herring <robherring2@gmail.com> wrote:
> On 04/26/2013 03:31 PM, Laura Abbott wrote:
>> Currently, of_platform_device_create_pdata always sets the
>> coherent DMA mask to 32 bits. On ARM systems without CONFIG_ZONE_DMA,
>> arm_dma_limit gets set to ~0 or 0xFFFFFFFFFFFFFFFF on LPAE based
>> systems. Since arm_dma_limit represents the smallest dma_mask
>> on the system, the default of 32 bits prevents any dma_coherent
>> allocation from succeeding unless clients manually set the
>> dma mask first. Rather than make every client on an LPAE system set
>> the mask manually, account for the size of dma_addr_t when setting
>> the coherent mask.
>>
>> Signed-off-by: Laura Abbott <lauraa@codeaurora.org>
>> ---
>>  drivers/of/platform.c |    2 +-
>>  1 files changed, 1 insertions(+), 1 deletions(-)
>>
>> diff --git a/drivers/of/platform.c b/drivers/of/platform.c
>> index 0970505..5f0ba94 100644
>> --- a/drivers/of/platform.c
>> +++ b/drivers/of/platform.c
>> @@ -214,7 +214,7 @@ struct platform_device *of_platform_device_create_pdata(
>>  #if defined(CONFIG_MICROBLAZE)
>>       dev->archdata.dma_mask = 0xffffffffUL;
>>  #endif
>> -     dev->dev.coherent_dma_mask = DMA_BIT_MASK(32);
>> +     dev->dev.coherent_dma_mask = DMA_BIT_MASK(sizeof(dma_addr_t) * 8);
>
> This is going to change the mask from 32 to 64 bits on 64-bit powerpc
> and others possibly. Maybe it doesn't matter. I think it doesn't, but
> I'm not sure enough to apply for 3.10. So I'll queue it for 3.11.

Without the patch, LPAE enabled board may not boot at all, but looks
it still isn't in -next tree.

But I am wondering if it is a correct approach, because enabling LPAE
doesn't mean the I/O devices can support DMA to/from 64bit address, and
it is very probably devices can't do it at all.

Another way is to always set arm_dma_limit as 0xFFFFFFFF when
CONFIG_ZONE_DMA is unset, and let driver override device's dma
mask if the device supports 64bit DMA.

Thanks,
Laura Abbott July 5, 2013, 7:33 p.m. UTC | #4
On 7/3/2013 7:15 AM, Ming Lei wrote:
> On Sat, Apr 27, 2013 at 5:32 AM, Rob Herring <robherring2@gmail.com> wrote:
>> On 04/26/2013 03:31 PM, Laura Abbott wrote:
>>> Currently, of_platform_device_create_pdata always sets the
>>> coherent DMA mask to 32 bits. On ARM systems without CONFIG_ZONE_DMA,
>>> arm_dma_limit gets set to ~0 or 0xFFFFFFFFFFFFFFFF on LPAE based
>>> systems. Since arm_dma_limit represents the smallest dma_mask
>>> on the system, the default of 32 bits prevents any dma_coherent
>>> allocation from succeeding unless clients manually set the
>>> dma mask first. Rather than make every client on an LPAE system set
>>> the mask manually, account for the size of dma_addr_t when setting
>>> the coherent mask.
>>>
>>> Signed-off-by: Laura Abbott <lauraa@codeaurora.org>
>>> ---
>>>   drivers/of/platform.c |    2 +-
>>>   1 files changed, 1 insertions(+), 1 deletions(-)
>>>
>>> diff --git a/drivers/of/platform.c b/drivers/of/platform.c
>>> index 0970505..5f0ba94 100644
>>> --- a/drivers/of/platform.c
>>> +++ b/drivers/of/platform.c
>>> @@ -214,7 +214,7 @@ struct platform_device *of_platform_device_create_pdata(
>>>   #if defined(CONFIG_MICROBLAZE)
>>>        dev->archdata.dma_mask = 0xffffffffUL;
>>>   #endif
>>> -     dev->dev.coherent_dma_mask = DMA_BIT_MASK(32);
>>> +     dev->dev.coherent_dma_mask = DMA_BIT_MASK(sizeof(dma_addr_t) * 8);
>>
>> This is going to change the mask from 32 to 64 bits on 64-bit powerpc
>> and others possibly. Maybe it doesn't matter. I think it doesn't, but
>> I'm not sure enough to apply for 3.10. So I'll queue it for 3.11.
>
> Without the patch, LPAE enabled board may not boot at all, but looks
> it still isn't in -next tree.
>
> But I am wondering if it is a correct approach, because enabling LPAE
> doesn't mean the I/O devices can support DMA to/from 64bit address, and
> it is very probably devices can't do it at all.
>

The problem is the way the arm_dma_limit is set up, all dma allocations 
are currently broken regardless of if the actual device supports 64-bit 
addresses or not.

> Another way is to always set arm_dma_limit as 0xFFFFFFFF when
> CONFIG_ZONE_DMA is unset, and let driver override device's dma
> mask if the device supports 64bit DMA.
>

I previously asked about the arm_dma_limit and was told that the current 
behavior is the correct approach (see 
https://lists.ozlabs.org/pipermail/devicetree-discuss/2013-April/032729.html 
and 
https://lists.ozlabs.org/pipermail/devicetree-discuss/2013-April/032690.html) 


The point here is to set the mask to something reasonable such that 
allocations can succeed by default. If devices can't use part of the 
memory space they can adjust the limit accordingly.

Thanks,
Laura
Russell King - ARM Linux July 5, 2013, 10:19 p.m. UTC | #5
On Wed, Jul 03, 2013 at 10:15:50PM +0800, Ming Lei wrote:
> 
> Without the patch, LPAE enabled board may not boot at all, but looks
> it still isn't in -next tree.
> 
> But I am wondering if it is a correct approach, because enabling LPAE
> doesn't mean the I/O devices can support DMA to/from 64bit address, and
> it is very probably devices can't do it at all.
> 
> Another way is to always set arm_dma_limit as 0xFFFFFFFF when
> CONFIG_ZONE_DMA is unset, and let driver override device's dma
> mask if the device supports 64bit DMA.

Okay, here's a teach-in on DMA masks, this is how it's supposed to work:

1. The bus code which creates the devices sets a default mask appropriate
   for the bus type.  For PCI, this is 32-bit, for ISA, it's 24-bit.

2. Before performing DMA, drivers should query the capabilities of the
   platform using dma_set_mask() for streaming transfers, and
   dma_set_coherent_mask() for coherent transfers.  Note: the DMA API
   specifies that a mask accepted by dma_set_mask() will also be
   accepted by dma_set_coherent_mask() - in other words, it is
   permissible _not_ to check the return value of dma_set_coherent_mask()
   iff that same mask is currently in use via dma_set_mask().

   (Side note: I have a patch in my tree which introduces
   dma_set_mask_and_coherent() which sets both.)

3. Drivers should attempt to set a mask appropriate for the device.
   If the device can drive 32-bit addresses, it requests a 32-bit
   mask.  If only 24-bit, a 24-bit mask.  If it wants to do more
   than 32-bit, it should set a larger mask.

   (See the "DMA addressing limitations" section of
    Documentation/DMA-API-HOWTO.txt).

4. PCI drivers use this as part of a negotiation with the rest of the
   system; you can plug a 64-bit capable PCI card into a 32-bit only
   capable host, and it should work - the driver should try to
   request the 64-bit mask first, if that succeeds then it can use
   DMA to 64-bit addresses.  If it fails, then it should then try to
   set a 32-bit mask.

So, that's more or less what's currently "known" of DMA masks.  What a lot
of ARM drivers do is totally ignore (3) and just assume that the platform
code set the mask up appropriately.  This is an incorrect assumption, and
it's something I'm slowly fixing where I have the knowledge.

What we shouldn't be relying upon is the default DMA mask being correct.

Last point is - the device actually doing DMA should be the one which the
DMA mask counts for above; if the device is just a user of DMA, then its
DMA mask should not matter (if it does, it's likely because of x86/PCI
assumptions made in the subsystem code) and actually should _not_ be set.
In other words, the DMA engine probe function should set the DMA mask on
the DMA controller, and you should do DMA mappings against the DMA
controller's struct device, not against the IO peripheral's struct
device.
Ming Lei July 6, 2013, 2:21 a.m. UTC | #6
On Sat, Jul 6, 2013 at 3:33 AM, Laura Abbott <lauraa@codeaurora.org> wrote:
> On 7/3/2013 7:15 AM, Ming Lei wrote:
>>
>> On Sat, Apr 27, 2013 at 5:32 AM, Rob Herring <robherring2@gmail.com>
>> wrote:
>>>
>>> On 04/26/2013 03:31 PM, Laura Abbott wrote:
>>>>
>>>> Currently, of_platform_device_create_pdata always sets the
>>>> coherent DMA mask to 32 bits. On ARM systems without CONFIG_ZONE_DMA,
>>>> arm_dma_limit gets set to ~0 or 0xFFFFFFFFFFFFFFFF on LPAE based
>>>> systems. Since arm_dma_limit represents the smallest dma_mask
>>>> on the system, the default of 32 bits prevents any dma_coherent
>>>> allocation from succeeding unless clients manually set the
>>>> dma mask first. Rather than make every client on an LPAE system set
>>>> the mask manually, account for the size of dma_addr_t when setting
>>>> the coherent mask.
>>>>
>>>> Signed-off-by: Laura Abbott <lauraa@codeaurora.org>
>>>> ---
>>>>   drivers/of/platform.c |    2 +-
>>>>   1 files changed, 1 insertions(+), 1 deletions(-)
>>>>
>>>> diff --git a/drivers/of/platform.c b/drivers/of/platform.c
>>>> index 0970505..5f0ba94 100644
>>>> --- a/drivers/of/platform.c
>>>> +++ b/drivers/of/platform.c
>>>> @@ -214,7 +214,7 @@ struct platform_device
>>>> *of_platform_device_create_pdata(
>>>>   #if defined(CONFIG_MICROBLAZE)
>>>>        dev->archdata.dma_mask = 0xffffffffUL;
>>>>   #endif
>>>> -     dev->dev.coherent_dma_mask = DMA_BIT_MASK(32);
>>>> +     dev->dev.coherent_dma_mask = DMA_BIT_MASK(sizeof(dma_addr_t) * 8);
>>>
>>>
>>> This is going to change the mask from 32 to 64 bits on 64-bit powerpc
>>> and others possibly. Maybe it doesn't matter. I think it doesn't, but
>>> I'm not sure enough to apply for 3.10. So I'll queue it for 3.11.
>>
>>
>> Without the patch, LPAE enabled board may not boot at all, but looks
>> it still isn't in -next tree.
>>
>> But I am wondering if it is a correct approach, because enabling LPAE
>> doesn't mean the I/O devices can support DMA to/from 64bit address, and
>> it is very probably devices can't do it at all.
>>
>
> The problem is the way the arm_dma_limit is set up, all dma allocations are
> currently broken regardless of if the actual device supports 64-bit
> addresses or not.

Yes, I know, and since arm_dma_limit becomes 0xFFFFFFFFFFFFFFFF
now if LPAE is enabled and ZONE_DMA unset, so we have to set
dev->coherent_dma_mask as so big, otherwise __dma_alloc() can't go
ahead.

But please see below words in Documentation/DMA-API.txt:

           Further, the physical address of the memory must be within the
           dma_mask of the device (the dma_mask represents a bit mask of the
           addressable region for the device.  I.e., if the physical address of
           the memory anded with the dma_mask is still equal to the physical
           address, then the device can perform DMA to the memory).

You may argue that the description is about dev->dma_mask, but the
usage should be same.

In fact, most of devices in ARM SoC(even with LPAE enabled) doesn't
support 64bit DMA, so I don't see the point that the default mask is set
as 64bit.

Also we might need to consider why ppc64 didn't do that previously.

>
>
>> Another way is to always set arm_dma_limit as 0xFFFFFFFF when
>> CONFIG_ZONE_DMA is unset, and let driver override device's dma
>> mask if the device supports 64bit DMA.
>>
>
> I previously asked about the arm_dma_limit and was told that the current
> behavior is the correct approach (see
> https://lists.ozlabs.org/pipermail/devicetree-discuss/2013-April/032729.html
> and
> https://lists.ozlabs.org/pipermail/devicetree-discuss/2013-April/032690.html)
>
> The point here is to set the mask to something reasonable such that

Looks the default 64bit mask isn't reasonable, is it?

> allocations can succeed by default. If devices can't use part of the memory
> space they can adjust the limit accordingly.

No, device can't set the mask any more since any other value which is less
than (u64)-1 can't succeed, and looks that is the current problem.

Thanks,
Russell King - ARM Linux July 6, 2013, 9:15 a.m. UTC | #7
On Sat, Jul 06, 2013 at 10:21:05AM +0800, Ming Lei wrote:
> But please see below words in Documentation/DMA-API.txt:
> 
>            Further, the physical address of the memory must be within the
>            dma_mask of the device (the dma_mask represents a bit mask of the
>            addressable region for the device.  I.e., if the physical address of
>            the memory anded with the dma_mask is still equal to the physical
>            address, then the device can perform DMA to the memory).
> 
> You may argue that the description is about dev->dma_mask, but the
> usage should be same.
> 
> In fact, most of devices in ARM SoC(even with LPAE enabled) doesn't
> support 64bit DMA, so I don't see the point that the default mask is set
> as 64bit.

There's another couple of issues there:

(a) Linux assumes that physical memory starts at location zero.  There
    are various places in the kernel that assume that this holds true:

	max_dma_pfn = (dma_mask >> PAGE_SHIFT) + 1

    One notable place is the block layer.  I've suggested to Santosh a
    way to fix this but I need to help him a little more with it.

(b) Device DMA addresses are a *separate* address space to the physical
    address space.  Device DMA addresses are the addresses which need to
    be programmed into the device to perform DMA to the identified
    location.

What (b) means is that if you are ending up with a 64-bit address to be
programmed into a 32-bit only register, there is something very very
wrong.  What this also means is that a 32-bit DMA mask should suffice,
because if the DMA address results in a 32-bit address _for the DMA
device_ then we are within its capabilities.

In any case, the work around is not to set the DMA mask to be 64-bit.
Think about it.  What if you have 8GB of physical memory in a LPAE
system, but your DMA controllers can only be programmed with a 32-bit
address?

Lastly, think about what I said last night about most of the ARM drivers
being rather buggy in that they do not call either dma_set_mask() or
dma_set_coherent_mask().

So, I think we're well past the point where we need to stop assuming that
DMA masks somehow relate to physical addresses, and that they can be used
to indicate a range of physical addresses starting at zero and extending
up to and including the mask value.  To call such a thing a "mask" is
absolutely absurd.
Ming Lei July 8, 2013, 10:03 a.m. UTC | #8
On Sat, Jul 6, 2013 at 5:15 PM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> On Sat, Jul 06, 2013 at 10:21:05AM +0800, Ming Lei wrote:
>> But please see below words in Documentation/DMA-API.txt:
>>
>>            Further, the physical address of the memory must be within the
>>            dma_mask of the device (the dma_mask represents a bit mask of the
>>            addressable region for the device.  I.e., if the physical address of
>>            the memory anded with the dma_mask is still equal to the physical
>>            address, then the device can perform DMA to the memory).
>>
>> You may argue that the description is about dev->dma_mask, but the
>> usage should be same.
>>
>> In fact, most of devices in ARM SoC(even with LPAE enabled) doesn't
>> support 64bit DMA, so I don't see the point that the default mask is set
>> as 64bit.
>
> There's another couple of issues there:
>
> (a) Linux assumes that physical memory starts at location zero.  There
>     are various places in the kernel that assume that this holds true:
>
>         max_dma_pfn = (dma_mask >> PAGE_SHIFT) + 1
>
>     One notable place is the block layer.  I've suggested to Santosh a
>     way to fix this but I need to help him a little more with it.
>
> (b) Device DMA addresses are a *separate* address space to the physical
>     address space.  Device DMA addresses are the addresses which need to
>     be programmed into the device to perform DMA to the identified
>     location.
>
> What (b) means is that if you are ending up with a 64-bit address to be
> programmed into a 32-bit only register, there is something very very
> wrong.  What this also means is that a 32-bit DMA mask should suffice,
> because if the DMA address results in a 32-bit address _for the DMA
> device_ then we are within its capabilities.
>
> In any case, the work around is not to set the DMA mask to be 64-bit.

So how about working around the problem by setting arm_dma_limit as
(phys_addr_t)0xffffffff if CONFIG_ZONE_DMA is unset? Or other better
solutions?

Otherwise enabling LPAE may cause system boot failure because
mmc card may not work when arm_dma_limit becomes (u64)-1.

> Think about it.  What if you have 8GB of physical memory in a LPAE
> system, but your DMA controllers can only be programmed with a 32-bit
> address?
>
> Lastly, think about what I said last night about most of the ARM drivers
> being rather buggy in that they do not call either dma_set_mask() or
> dma_set_coherent_mask().
>
> So, I think we're well past the point where we need to stop assuming that
> DMA masks somehow relate to physical addresses, and that they can be used
> to indicate a range of physical addresses starting at zero and extending
> up to and including the mask value.  To call such a thing a "mask" is
> absolutely absurd.


Thanks,
Russell King - ARM Linux July 8, 2013, 10:42 a.m. UTC | #9
On Fri, Jul 05, 2013 at 12:33:21PM -0700, Laura Abbott wrote:
> On 7/3/2013 7:15 AM, Ming Lei wrote:
>> On Sat, Apr 27, 2013 at 5:32 AM, Rob Herring <robherring2@gmail.com> wrote:
>>> On 04/26/2013 03:31 PM, Laura Abbott wrote:
>>>> Currently, of_platform_device_create_pdata always sets the
>>>> coherent DMA mask to 32 bits. On ARM systems without CONFIG_ZONE_DMA,
>>>> arm_dma_limit gets set to ~0 or 0xFFFFFFFFFFFFFFFF on LPAE based
>>>> systems. Since arm_dma_limit represents the smallest dma_mask
>>>> on the system, the default of 32 bits prevents any dma_coherent
>>>> allocation from succeeding unless clients manually set the
>>>> dma mask first. Rather than make every client on an LPAE system set
>>>> the mask manually, account for the size of dma_addr_t when setting
>>>> the coherent mask.
>>>>
>>>> Signed-off-by: Laura Abbott <lauraa@codeaurora.org>
>>>> ---
>>>>   drivers/of/platform.c |    2 +-
>>>>   1 files changed, 1 insertions(+), 1 deletions(-)
>>>>
>>>> diff --git a/drivers/of/platform.c b/drivers/of/platform.c
>>>> index 0970505..5f0ba94 100644
>>>> --- a/drivers/of/platform.c
>>>> +++ b/drivers/of/platform.c
>>>> @@ -214,7 +214,7 @@ struct platform_device *of_platform_device_create_pdata(
>>>>   #if defined(CONFIG_MICROBLAZE)
>>>>        dev->archdata.dma_mask = 0xffffffffUL;
>>>>   #endif
>>>> -     dev->dev.coherent_dma_mask = DMA_BIT_MASK(32);
>>>> +     dev->dev.coherent_dma_mask = DMA_BIT_MASK(sizeof(dma_addr_t) * 8);
>>>
>>> This is going to change the mask from 32 to 64 bits on 64-bit powerpc
>>> and others possibly. Maybe it doesn't matter. I think it doesn't, but
>>> I'm not sure enough to apply for 3.10. So I'll queue it for 3.11.
>>
>> Without the patch, LPAE enabled board may not boot at all, but looks
>> it still isn't in -next tree.
>>
>> But I am wondering if it is a correct approach, because enabling LPAE
>> doesn't mean the I/O devices can support DMA to/from 64bit address, and
>> it is very probably devices can't do it at all.
>>
>
> The problem is the way the arm_dma_limit is set up, all dma allocations  
> are currently broken regardless of if the actual device supports 64-bit  
> addresses or not.

Please explain this statement.

> I previously asked about the arm_dma_limit and was told that the current  
> behavior is the correct approach (see  
> https://lists.ozlabs.org/pipermail/devicetree-discuss/2013-April/032729.html 
> and  
> https://lists.ozlabs.org/pipermail/devicetree-discuss/2013-April/032690.html) 
>
> The point here is to set the mask to something reasonable such that  
> allocations can succeed by default. If devices can't use part of the  
> memory space they can adjust the limit accordingly.

The point is arm_dma_limit is that it sets the limit of the memory you
get from the page allocators when you ask for a GFP_DMA allocation.

In the case of no GFP_DMA zone, all memory is available for allocations.
Hence, arm_dma_limit is set to the absolute maximum physical memory
address which an allocator could return.

In the case of a 32-bit only system, this is 32-bits.  In the case of a
LPAE system, it is 64-bit.  (This follows because arm_dma_limit is
phys_addr_t, and the size of this follows the bit size of the system.)

The only questionable case is where we have a LPAE system with a GFP_DMA
zone and arm_dma_limit is set to 0xfffffffff - this limit is probably
too low.

Now the reason for the arm_dma_limit is very simple: if the streaming
APIs are passed a buffer outside of the DMA mask, we must reallocate
that memory.  We must have a way to get memory which is within the DMA
mask.  That's what passing GFP_DMA does.  If you have no GFP_DMA zone,
then any memory could be returned.  In the case of a LPAE system with
memory both above and below the 32-bit address range, and you try to
allocate a buffer which happens to be above the 32-bit boundary, what
do you do?  Do you just retry the allocation and hope for the best?
What if that allocation also came out above the 32-bit boundary?  Do
you continue gobbling up system memory until you get a page which you
can DMA do and then free all those buffers you unsuccesfully obtained?

We have a massive problem with DMA masks in the Linux kernel, because
of the x86-ism of assuming that physical memory always starts at
address zero.  This assumption simply is not true on ARM, even more so
with LPAE, which is why people are starting to see a problem.

Consider this case: you have an existing 32-bit DMA controller.  This
controller sets a 32-bit DMA mask, because that's all it is capable of
addressing.  It gets used on 32-bit systems, and it works fine.

It gets put onto a LPAE system where memory starts at the 4GB boundary.
The DMA controller can address the first 4GB of memory because that's
how it's wired up.  The DMA controller still has a 32-bit DMA mask
because as far as the DMA controller is concerned, nothing has changed.
But now things fail because the kernel assumes that a DMA mask is
something to do with a physical address.

The solution to this is to fix the DMA masks and such like.  The problem
is getting the traction throughout the architectures, drivers, subsystem
maintainers etc to accept a change - and working out a way to do the
change without causing a lot of breakage.
Russell King - ARM Linux July 8, 2013, 10:44 a.m. UTC | #10
On Mon, Jul 08, 2013 at 06:03:57PM +0800, Ming Lei wrote:
> On Sat, Jul 6, 2013 at 5:15 PM, Russell King - ARM Linux
> <linux@arm.linux.org.uk> wrote:
> > On Sat, Jul 06, 2013 at 10:21:05AM +0800, Ming Lei wrote:
> >> But please see below words in Documentation/DMA-API.txt:
> >>
> >>            Further, the physical address of the memory must be within the
> >>            dma_mask of the device (the dma_mask represents a bit mask of the
> >>            addressable region for the device.  I.e., if the physical address of
> >>            the memory anded with the dma_mask is still equal to the physical
> >>            address, then the device can perform DMA to the memory).
> >>
> >> You may argue that the description is about dev->dma_mask, but the
> >> usage should be same.
> >>
> >> In fact, most of devices in ARM SoC(even with LPAE enabled) doesn't
> >> support 64bit DMA, so I don't see the point that the default mask is set
> >> as 64bit.
> >
> > There's another couple of issues there:
> >
> > (a) Linux assumes that physical memory starts at location zero.  There
> >     are various places in the kernel that assume that this holds true:
> >
> >         max_dma_pfn = (dma_mask >> PAGE_SHIFT) + 1
> >
> >     One notable place is the block layer.  I've suggested to Santosh a
> >     way to fix this but I need to help him a little more with it.
> >
> > (b) Device DMA addresses are a *separate* address space to the physical
> >     address space.  Device DMA addresses are the addresses which need to
> >     be programmed into the device to perform DMA to the identified
> >     location.
> >
> > What (b) means is that if you are ending up with a 64-bit address to be
> > programmed into a 32-bit only register, there is something very very
> > wrong.  What this also means is that a 32-bit DMA mask should suffice,
> > because if the DMA address results in a 32-bit address _for the DMA
> > device_ then we are within its capabilities.
> >
> > In any case, the work around is not to set the DMA mask to be 64-bit.
> 
> So how about working around the problem by setting arm_dma_limit as
> (phys_addr_t)0xffffffff if CONFIG_ZONE_DMA is unset? Or other better
> solutions?
> 
> Otherwise enabling LPAE may cause system boot failure because
> mmc card may not work when arm_dma_limit becomes (u64)-1.

Well, working around it by bodging it means that the bodges will stay
and the problem will become even worse later, and we won't have the
weight of people saying it doesn't work to use as leverage to persuade
people that DMA masks need to change.  Sorry.
Laura Abbott July 9, 2013, 2:38 a.m. UTC | #11
On 7/8/2013 3:42 AM, Russell King - ARM Linux wrote:
>>
>> The problem is the way the arm_dma_limit is set up, all dma allocations
>> are currently broken regardless of if the actual device supports 64-bit
>> addresses or not.
>
> Please explain this statement.
>

The statement was a bit shortsighted given recent discussions. My point 
was that on LPAE systems right now without ZONE_DMA, the allocation 
fails by default (i.e. no call to set_mask) even if the device could 
support 64-bit physical memory. Based on this thread though, drivers 
should stop assuming the dma_mask will be set up properly and always set 
the mask appropriately.

The conclusion also seems to be that if devices on an LPAE system can't 
handle > 32-bit addresses and want to limit via mask, ZONE_DMA must be 
set up.

Please correct me if I misunderstood anything.

Thanks,
Laura
diff mbox

Patch

diff --git a/drivers/of/platform.c b/drivers/of/platform.c
index 0970505..5f0ba94 100644
--- a/drivers/of/platform.c
+++ b/drivers/of/platform.c
@@ -214,7 +214,7 @@  struct platform_device *of_platform_device_create_pdata(
 #if defined(CONFIG_MICROBLAZE)
 	dev->archdata.dma_mask = 0xffffffffUL;
 #endif
-	dev->dev.coherent_dma_mask = DMA_BIT_MASK(32);
+	dev->dev.coherent_dma_mask = DMA_BIT_MASK(sizeof(dma_addr_t) * 8);
 	dev->dev.bus = &platform_bus_type;
 	dev->dev.platform_data = platform_data;