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@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

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 at 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@10.5Mbps down 400kbps up
according to speedtest.net.

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 at 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

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 at 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 at 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.

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 at 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 at 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 at 0: Coherent DMA mask 0xffffffffffffffff is larger than dma_addr_t allows
[    5.478706] asoc-simple-card sound at 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

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 at 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

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 at 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