ARM: OMPA4+: is it expected dma_coerce_mask_and_coherent(dev, DMA_BIT_MASK(64)); to fail?

ARM: OMPA4+: is it expected dma_coerce_mask_and_coherent(dev, DMA_BIT_MASK(64)); to fail?

[Grygorii.Strashko@linaro.org]

Hi All,

Now I can see very interesting behavior related to dma_coerce_mask_and_coherent()
and friends which I'd like to explain and clarify.

Below is set of questions I have (why - I explained below):
- Is expected dma_coerce_mask_and_coherent(DMA_BIT_MASK(64)) and friends to fail on 32 bits HW?

- What is expected value for max_pfn: max_phys_pfn or max_phys_pfn + 1?

- What is expected value for struct memblock_region->size: mem_range_size or mem_range_size - 1?

- What is expected value to be returned by memblock_end_of_DRAM():
  @base + @size(max_phys_addr + 1) or @base + @size - 1(max_phys_addr)?


I'm working with BeaglBoard-X15 (AM572x/DRA7xx) board and have following code in OMAP ASOC driver
which is failed SOMETIMES during the boot with error -EIO.
=== to omap-pcm.c:
omap_pcm_new() {
...
	ret = dma_coerce_mask_and_coherent(card->dev, DMA_BIT_MASK(64));
^^ failed sometimes
	if (ret)
		return ret;
}

What I can see is that dma_coerce_mask_and_coherent() and etc may fail or succeed 
depending on - max_pfn value. 
-> max_pfn value depends on memblock configuration
max_pfn = max_high = PFN_DOWN(memblock_end_of_DRAM());
           |- PFN_DOWN(memblock.memory.regions[last_idx].base + memblock.memory.regions[last_idx].size)

-> memblock configuration depends on
a) CONFIG_ARM_LPAE=y|n (my system really works with 32 bit address space)
b) RAM configuration

Example 1 CONFIG_ARM_LPAE=n:
	memory {
		device_type = "memory";
		reg = <0x80000000 0x60000000>; /* 1536 MB */
	};

  memblock will be configured as:
	memory.cnt  = 0x1
	memory[0x0]     [0x00000080000000-0x000000dfffffff], 0x60000000 bytes flags: 0x0
							     ^^^^^^^^^^
  max_pfn = 0x000E0000

Example 2 CONFIG_ARM_LPAE=n:
	memory {
		device_type = "memory";
		reg = <0x80000000 0x80000000>;
	};

  memblock will be configured as:
	memory.cnt  = 0x1
	memory[0x0]     [0x00000080000000-0x000000fffffffe], 0x7fffffff bytes flags: 0x0
							     ^^^^^^^^^^
  max_pfn = 0x000FFFFF

Example 3 CONFIG_ARM_LPAE=y (but system really works with 32 bit address space):
	memory {
		device_type = "memory";
		reg = <0x80000000 0x80000000>;
	};

  memblock will be configured as:
	memory.cnt  = 0x1
	memory[0x0]     [0x00000080000000-0x000000ffffffff], 0x80000000 bytes flags: 0x0
							     ^^^^^^^^^^
  max_pfn = 0x00100000

The dma_coerce_mask_and_coherent() will fail in case 'Example 3' and succeed in cases 1,2.
dma-mapping.c --> __dma_supported()
	if (sizeof(mask) != sizeof(dma_addr_t) && <== true for all OMAP4+
	    mask > (dma_addr_t)~0 &&		<== true for DMA_BIT_MASK(64)
	    dma_to_pfn(dev, ~0) < max_pfn) {  <== true only for Example 3

I've tracked down patch which changes memblock behavior to:
commit eb18f1b5bfb99b1d7d2f5d792e6ee5c9b7d89330
Author: Tejun Heo <tj@kernel.org>
Date:   Thu Dec 8 10:22:07 2011 -0800

    memblock: Make memblock functions handle overflowing range @size

This commit is pretty old :( and it doesn't takes into account LPAE mode
where phys_addr_t is 64 bit, but physically accessible addresses <= 40 bit
(memblock_cap_size()).

The issue with omap-pcm was simply fixed by using DMA_BIT_MASK(32), but It seems problem is
wider and above behavior of dma_set_maskX() and memblock confused me a bit.

I'd be very appreciated for any comments/clarification on questions I've listed at the
beginning of my e-mail - there are no patches from my side as I'd like to understand 
expected behavior of the kernel first (especially taking into account that any
memblock changes might affect on at least half of arches). 

Thanks.


Additional info:
memblock: Make memblock functions handle overflowing range @size
https://lkml.org/lkml/2011/7/26/235
[alsa-devel] [PATCH] ASoC: omap-pcm: Lower the dma coherent mask to 32bits
http://mailman.alsa-project.org/pipermail/alsa-devel/2013-December/069817.html

-- 
regards,
-grygorii

Re: ARM: OMPA4+: is it expected dma_coerce_mask_and_coherent(dev, DMA_BIT_MASK(64)); to fail?

[Russell King - ARM Linux]

On Thu, Mar 05, 2015 at 08:55:07PM +0200, Grygorii.Strashko@linaro.org wrote:
> Now I can see very interesting behavior related to dma_coerce_mask_and_coherent()
> and friends which I'd like to explain and clarify.
> 
> Below is set of questions I have (why - I explained below):
> - Is expected dma_coerce_mask_and_coherent(DMA_BIT_MASK(64)) and friends to fail on 32 bits HW?

Not really.

> - What is expected value for max_pfn: max_phys_pfn or max_phys_pfn + 1?

mm/page_owner.c:
        /* Find an allocated page */
        for (; pfn < max_pfn; pfn++) {

drivers/base/platform.c:    u32 low_totalram = ((max_pfn - 1) << PAGE_SHIFT);
drivers/base/platform.c:    u32 high_totalram = ((max_pfn - 1) >> (32 - PAGE_SHIFT));

So, there's ample evidence that max_pfn is one more than the greatest pfn
which may be used in the system.

> - What is expected value for struct memblock_region->size: mem_range_size or mem_range_size - 1?

A size is a size - it's a number of bytes contained within the region.
If it is value 1, then there is exactly one byte in the region.  If
there are 0x7fffffff, then there are 2G-1 bytes in the region, not 2G.

> - What is expected value to be returned by memblock_end_of_DRAM():
>   @base + @size(max_phys_addr + 1) or @base + @size - 1(max_phys_addr)?

The last address plus one in the system.  However, there's a problem here.
On a 32-bit system, phys_addr_t may be 32-bit.  If it is 32-bit, then
"last address plus one" could be zero, which makes no sense.  Hence, it
is artificially reduced to 0xfffff000, thereby omitting the final page.

> Example 3 CONFIG_ARM_LPAE=y (but system really works with 32 bit address space):
> 	memory {
> 		device_type = "memory";
> 		reg = <0x80000000 0x80000000>;
> 	};
> 
>   memblock will be configured as:
> 	memory.cnt  = 0x1
> 	memory[0x0]     [0x00000080000000-0x000000ffffffff], 0x80000000 bytes flags: 0x0
> 							     ^^^^^^^^^^
>   max_pfn = 0x00100000
> 
> The dma_coerce_mask_and_coherent() will fail in case 'Example 3' and succeed in cases 1,2.
> dma-mapping.c --> __dma_supported()
> 	if (sizeof(mask) != sizeof(dma_addr_t) && <== true for all OMAP4+
> 	    mask > (dma_addr_t)~0 &&		<== true for DMA_BIT_MASK(64)
> 	    dma_to_pfn(dev, ~0) < max_pfn) {  <== true only for Example 3

Hmm, I think this may make more sense to be "< max_pfn - 1" here, as
that would be better suited to our intention.

The result of dma_to_pfn(dev, ~0) is the maximum PFN which we could
address via DMA, but we're comparing it with the maximum PFN in the
system plus 1 - so we need to subtract one from it.

Please think about this and test this out; I'm not back to normal yet
(post-op) so I could very well not be thinking straight yet.

Thanks.

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.

Re: ARM: OMPA4+: is it expected dma_coerce_mask_and_coherent(dev, DMA_BIT_MASK(64)); to fail?

[Grygorii.Strashko@linaro.org]

Hi Russell,

On 03/05/2015 10:17 PM, Russell King - ARM Linux wrote:
> On Thu, Mar 05, 2015 at 08:55:07PM +0200, Grygorii.Strashko@linaro.org wrote:
>> Now I can see very interesting behavior related to dma_coerce_mask_and_coherent()
>> and friends which I'd like to explain and clarify.
>>
>> Below is set of questions I have (why - I explained below):
>> - Is expected dma_coerce_mask_and_coherent(DMA_BIT_MASK(64)) and friends to fail on 32 bits HW?
> 
> Not really.
> 
>> - What is expected value for max_pfn: max_phys_pfn or max_phys_pfn + 1?
> 
> mm/page_owner.c:
>          /* Find an allocated page */
>          for (; pfn < max_pfn; pfn++) {
> 
> drivers/base/platform.c:    u32 low_totalram = ((max_pfn - 1) << PAGE_SHIFT);
> drivers/base/platform.c:    u32 high_totalram = ((max_pfn - 1) >> (32 - PAGE_SHIFT));
> 
> So, there's ample evidence that max_pfn is one more than the greatest pfn
> which may be used in the system.
> 
>> - What is expected value for struct memblock_region->size: mem_range_size or mem_range_size - 1?
> 
> A size is a size - it's a number of bytes contained within the region.
> If it is value 1, then there is exactly one byte in the region.  If
> there are 0x7fffffff, then there are 2G-1 bytes in the region, not 2G.

Thanks - it seems clear now.

>> - What is expected value to be returned by memblock_end_of_DRAM():
>>    @base + @size(max_phys_addr + 1) or @base + @size - 1(max_phys_addr)?
> 
> The last address plus one in the system.  However, there's a problem here.
> On a 32-bit system, phys_addr_t may be 32-bit.  If it is 32-bit, then
> "last address plus one" could be zero, which makes no sense.  Hence, it
> is artificially reduced to 0xfffff000, thereby omitting the final page.

^ this part seems not fully true now, because for ARM32 + DT the 
fdt.c->early_init_dt_add_memory_arch() is called instead of arm_add_memory()
 and it works in a different way a bit.

For example, I don't see below message when reg = <0x80000000 0x80000000>:
"Truncating memory at 0x80000000 to fit in 32-bit physical address space"

instead memblock silently configured as
memory.cnt  = 0x1
memory[0x0].base = 0x80000000
memory[0x0].size = 0x7fffffff


> 
>> Example 3 CONFIG_ARM_LPAE=y (but system really works with 32 bit address space):
>> 	memory {
>> 		device_type = "memory";
>> 		reg = <0x80000000 0x80000000>;
>> 	};
>>
>>    memblock will be configured as:
>> 	memory.cnt  = 0x1
>> 	memory[0x0]     [0x00000080000000-0x000000ffffffff], 0x80000000 bytes flags: 0x0
>> 							     ^^^^^^^^^^
>>    max_pfn = 0x00100000
>>
>> The dma_coerce_mask_and_coherent() will fail in case 'Example 3' and succeed in cases 1,2.
>> dma-mapping.c --> __dma_supported()
>> 	if (sizeof(mask) != sizeof(dma_addr_t) && <== true for all OMAP4+
>> 	    mask > (dma_addr_t)~0 &&		<== true for DMA_BIT_MASK(64)
>> 	    dma_to_pfn(dev, ~0) < max_pfn) {  <== true only for Example 3
> 
> Hmm, I think this may make more sense to be "< max_pfn - 1" here, as
> that would be better suited to our intention.
> 
> The result of dma_to_pfn(dev, ~0) is the maximum PFN which we could
> address via DMA, but we're comparing it with the maximum PFN in the
> system plus 1 - so we need to subtract one from it.

Ok. I'll try it.

> 
> Please think about this and test this out; I'm not back to normal yet
> (post-op) so I could very well not be thinking straight yet.

Thanks for your comments. I hope you feel better.

-- 
regards,
-grygorii

Re: ARM: OMPA4+: is it expected dma_coerce_mask_and_coherent(dev, DMA_BIT_MASK(64)); to fail?

[Arnd Bergmann]

On Thursday 05 March 2015 20:55:07 Grygorii.Strashko@linaro.org wrote:
> Hi All,
> 
> Now I can see very interesting behavior related to dma_coerce_mask_and_coherent()
> and friends which I'd like to explain and clarify.
> 
> Below is set of questions I have (why - I explained below):
> - Is expected dma_coerce_mask_and_coherent(DMA_BIT_MASK(64)) and friends to fail on 32 bits HW?

No. dma_coerce_mask_and_coherent() is meant to ignore the actual mask. It's
usually considered a bug to use this function for that reason.

> - What is expected value for max_pfn: max_phys_pfn or max_phys_pfn + 1?
> 
> - What is expected value for struct memblock_region->size: mem_range_size or mem_range_size - 1?
> 
> - What is expected value to be returned by memblock_end_of_DRAM():
>   @base + @size(max_phys_addr + 1) or @base + @size - 1(max_phys_addr)?
> 
> 
> I'm working with BeaglBoard-X15 (AM572x/DRA7xx) board and have following code in OMAP ASOC driver
> which is failed SOMETIMES during the boot with error -EIO.
> === to omap-pcm.c:
> omap_pcm_new() {
> ...
> 	ret = dma_coerce_mask_and_coherent(card->dev, DMA_BIT_MASK(64));
> ^^ failed sometimes
> 	if (ret)
> 		return ret;
> }

The code should be fixed to use dma_set_mask_and_coherent(), which is expected to
fail if the bus is incapable of addressing all RAM within the mask.

> I'd be very appreciated for any comments/clarification on questions I've listed at the
> beginning of my e-mail - there are no patches from my side as I'd like to understand 
> expected behavior of the kernel first (especially taking into account that any
> memblock changes might affect on at least half of arches). 

Is the device you have actually 64-bit capable?

Is the bus it is connected to 64-bit wide?

Does the dma-ranges property of the parent bus reflect the correct address width?

	Arnd

Re: ARM: OMPA4+: is it expected dma_coerce_mask_and_coherent(dev, DMA_BIT_MASK(64)); to fail?

[Russell King - ARM Linux]

On Fri, Mar 06, 2015 at 11:47:48PM +0200, Grygorii.Strashko@linaro.org wrote:
> Hi Russell,
> 
> On 03/05/2015 10:17 PM, Russell King - ARM Linux wrote:
> > On Thu, Mar 05, 2015 at 08:55:07PM +0200, Grygorii.Strashko@linaro.org wrote:
> >> The dma_coerce_mask_and_coherent() will fail in case 'Example 3' and succeed in cases 1,2.
> >> dma-mapping.c --> __dma_supported()
> >> 	if (sizeof(mask) != sizeof(dma_addr_t) && <== true for all OMAP4+
> >> 	    mask > (dma_addr_t)~0 &&		<== true for DMA_BIT_MASK(64)
> >> 	    dma_to_pfn(dev, ~0) < max_pfn) {  <== true only for Example 3
> > 
> > Hmm, I think this may make more sense to be "< max_pfn - 1" here, as
> > that would be better suited to our intention.
> > 
> > The result of dma_to_pfn(dev, ~0) is the maximum PFN which we could
> > address via DMA, but we're comparing it with the maximum PFN in the
> > system plus 1 - so we need to subtract one from it.
> 
> Ok. I'll try it.

Any news on this - I think it is a real off-by-one bug which we should
fix in any case.

Thanks.

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.

Re: ARM: OMPA4+: is it expected dma_coerce_mask_and_coherent(dev, DMA_BIT_MASK(64)); to fail?

[Grygorii.Strashko@linaro.org]

Hi Russell,

On 03/10/2015 01:05 PM, Russell King - ARM Linux wrote:
> On Fri, Mar 06, 2015 at 11:47:48PM +0200, Grygorii.Strashko@linaro.org wrote:
>> On 03/05/2015 10:17 PM, Russell King - ARM Linux wrote:
>>> On Thu, Mar 05, 2015 at 08:55:07PM +0200, Grygorii.Strashko@linaro.org wrote:
>>>> The dma_coerce_mask_and_coherent() will fail in case 'Example 3' and succeed in cases 1,2.
>>>> dma-mapping.c --> __dma_supported()
>>>> 	if (sizeof(mask) != sizeof(dma_addr_t) && <== true for all OMAP4+
>>>> 	    mask > (dma_addr_t)~0 &&		<== true for DMA_BIT_MASK(64)
>>>> 	    dma_to_pfn(dev, ~0) < max_pfn) {  <== true only for Example 3
>>>
>>> Hmm, I think this may make more sense to be "< max_pfn - 1" here, as
>>> that would be better suited to our intention.
>>>
>>> The result of dma_to_pfn(dev, ~0) is the maximum PFN which we could
>>> address via DMA, but we're comparing it with the maximum PFN in the
>>> system plus 1 - so we need to subtract one from it.
>>
>> Ok. I'll try it.
> 
> Any news on this - I think it is a real off-by-one bug which we should
> fix in any case.

Sorry for delay, there was a day-off on my side.

As per my test results - with above change 
 dma_coerce_mask_and_coherent(DMA_BIT_MASK(64)) and friends will succeed always.


=========== Test results:

==== Test case 1:
Input data:
- RAM: start = 0x80000000 size = 0x80000000
- CONFIG_ARM_LPAE=n and sizeof(phys_addr_t) = 4

a) NO changes:
 memory registered within memblock as:
   memory.cnt  = 0x1
   memory[0x0]     [0x00000080000000-0x000000fffffffe], 0x7fffffff bytes flags: 0x0

 max_pfn   = 0xFFFFF
 max_mapnr = 0x7FFFF

 dma_set_mask_and_coherent(dev, DMA_BIT_MASK(64)); -- succeeded

b) with change in __dma_supported():
        if (sizeof(mask) != sizeof(dma_addr_t) &&
            mask > (dma_addr_t)~0 &&
-           dma_to_pfn(dev, ~0) < max_pfn) {
+           dma_to_pfn(dev, ~0) < (max_pfn - 1)) {
                if (warn) {

 memory registered within memblock as:
   memory.cnt  = 0x1
   memory[0x0]     [0x00000080000000-0x000000fffffffe], 0x7fffffff bytes flags: 0x0

 max_pfn   = 0xFFFFF
 max_mapnr = 0x7FFFF

 dma_set_mask_and_coherent(dev, DMA_BIT_MASK(64)); -- succeeded


==== Test case 2:
Input data:
- RAM: start = 0x80000000 size = 0x80000000
- CONFIG_ARM_LPAE=y and sizeof(phys_addr_t) = 8

a) NO changes:
 memory registered within memblock as:
   memory.cnt  = 0x1
   memory[0x0]     [0x00000080000000-0x000000ffffffff], 0x80000000 bytes flags: 0x0

 max_pfn   = 0x100000
 max_mapnr = 0x80000

 dma_set_mask_and_coherent(dev, DMA_BIT_MASK(64)); -- failed
[    5.468470] asoc-simple-card sound@0: Coherent DMA mask 0xffffffffffffffff is larger than dma_addr_t allows
[    5.478706] asoc-simple-card sound@0: Driver did not use or check the return value from dma_set_coherent_mask()?
[    5.496620] davinci-mcasp 48468000.mcasp: ASoC: pcm constructor failed: -5
[    5.503844] asoc-simple-card sound@0: ASoC: can't create pcm davinci-mcasp.0-tlv320aic3x-hifi :-5


b) with change in __dma_supported():
        if (sizeof(mask) != sizeof(dma_addr_t) &&
            mask > (dma_addr_t)~0 &&
-           dma_to_pfn(dev, ~0) < max_pfn) {
+           dma_to_pfn(dev, ~0) < (max_pfn - 1)) {
                if (warn) {

 memory registered within memblock as:
   memory.cnt  = 0x1
   memory[0x0]     [0x00000080000000-0x000000ffffffff], 0x80000000 bytes flags: 0x0

 max_pfn   = 0x100000
 max_mapnr = 0x80000

 dma_set_mask_and_coherent(dev, DMA_BIT_MASK(64)); -- succeeded

regards,
-grygorii

-- 
regards,
-grygorii

Re: ARM: OMPA4+: is it expected dma_coerce_mask_and_coherent(dev, DMA_BIT_MASK(64)); to fail?

[Grygorii.Strashko@linaro.org]

Hi Arnd,

On 03/09/2015 11:33 PM, Arnd Bergmann wrote:
> On Thursday 05 March 2015 20:55:07 Grygorii.Strashko@linaro.org wrote:
>> Hi All,
>>
>> Now I can see very interesting behavior related to dma_coerce_mask_and_coherent()
>> and friends which I'd like to explain and clarify.
>>
>> Below is set of questions I have (why - I explained below):
>> - Is expected dma_coerce_mask_and_coherent(DMA_BIT_MASK(64)) and friends to fail on 32 bits HW?
> 
> No. dma_coerce_mask_and_coherent() is meant to ignore the actual mask. It's
> usually considered a bug to use this function for that reason.
> 
>> - What is expected value for max_pfn: max_phys_pfn or max_phys_pfn + 1?
>>
>> - What is expected value for struct memblock_region->size: mem_range_size or mem_range_size - 1?
>>
>> - What is expected value to be returned by memblock_end_of_DRAM():
>>    @base + @size(max_phys_addr + 1) or @base + @size - 1(max_phys_addr)?
>>
>>
>> I'm working with BeaglBoard-X15 (AM572x/DRA7xx) board and have following code in OMAP ASOC driver
>> which is failed SOMETIMES during the boot with error -EIO.
>> === to omap-pcm.c:
>> omap_pcm_new() {
>> ...
>> 	ret = dma_coerce_mask_and_coherent(card->dev, DMA_BIT_MASK(64));
>> ^^ failed sometimes
>> 	if (ret)
>> 		return ret;
>> }
> 
> The code should be fixed to use dma_set_mask_and_coherent(), which is expected to
> fail if the bus is incapable of addressing all RAM within the mask.
> 
>> I'd be very appreciated for any comments/clarification on questions I've listed at the
>> beginning of my e-mail - there are no patches from my side as I'd like to understand
>> expected behavior of the kernel first (especially taking into account that any
>> memblock changes might affect on at least half of arches).
> 
> Is the device you have actually 64-bit capable?
> 
> Is the bus it is connected to 64-bit wide?

As I mentioned before - The device was fixed by switching to use 32 bit mask
"The issue with omap-pcm was simply fixed by using DMA_BIT_MASK(32), ".

> 
> Does the dma-ranges property of the parent bus reflect the correct address width?

dma-ranges is not used and all devices are created with default mask DMA_BIT_MASK(32);


My goal was to clarify above questions (first of all), because on my HW I can see
different values of  max_pfn, max_mapnr and memblock configuration depending on 
CONFIG_ARM_LPAE=n|y and when RAM is defined as: start = 0x80000000 size = 0x80000000.
(and also between kernels 3.14 and LKML).

Looks like such RAM configuration is a corner case, which is not always handled as expected
(and how is it expected to be handled?).
For example:
before commit ARM: 8025/1: Get rid of meminfo
- registered RAM  start = 0x80000000 size = 0x80000000 will be adjusted by arm_add_memory()
and final RAM configuration will be start = 0x80000000 size = 0x7FFFF000
after this commit:
- code will try to register start = 0x80000000 size = 0x80000000, but memblock will
adjust it to start = 0x80000000 size = 0x7fffffff.



-- 
regards,
-grygorii