[PATCH] ASoC: omap-pcm: Lower the dma coherent mask to 32bits

[PATCH] ASoC: omap-pcm: Lower the dma coherent mask to 32bits

[Peter Ujfalusi]

The underlying API dma_coerce_mask_and_coherent() checks the requested bits
mask against the size of dma_addr_t which is 32bits on OMAP.
Because of the previously used 64bits mask audio was not probing after
commit c9bd5e6 (and 4dcfa6007).
32bits for the DMA_BIT_MASK looks to be the correct one to use.

Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
---
Hi Mark,

This is the fix for OMAP audio for 3.13-rc2+. The dma coherent changes went in
between -rc1 and -rc2 which broke things because omap-pcm was using 64bits for
dma coherent mask.

Regards,
Peter

 sound/soc/omap/omap-pcm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/sound/soc/omap/omap-pcm.c b/sound/soc/omap/omap-pcm.c
index b8fa986..01d59d0 100644
--- a/sound/soc/omap/omap-pcm.c
+++ b/sound/soc/omap/omap-pcm.c
@@ -202,7 +202,7 @@ static int omap_pcm_new(struct snd_soc_pcm_runtime *rtd)
 	struct snd_pcm *pcm = rtd->pcm;
 	int ret;
 
-	ret = dma_coerce_mask_and_coherent(card->dev, DMA_BIT_MASK(64));
+	ret = dma_coerce_mask_and_coherent(card->dev, DMA_BIT_MASK(32));
 	if (ret)
 		return ret;
 
-- 
1.8.4.5

Re: [PATCH] ASoC: omap-pcm: Lower the dma coherent mask to 32bits

[Russell King - ARM Linux]

On Thu, Dec 05, 2013 at 10:06:42AM +0200, Peter Ujfalusi wrote:
> The underlying API dma_coerce_mask_and_coherent() checks the requested bits
> mask against the size of dma_addr_t which is 32bits on OMAP.
> Because of the previously used 64bits mask audio was not probing after
> commit c9bd5e6 (and 4dcfa6007).
> 32bits for the DMA_BIT_MASK looks to be the correct one to use.
> 
> Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
> ---
> Hi Mark,
> 
> This is the fix for OMAP audio for 3.13-rc2+. The dma coherent changes went in
> between -rc1 and -rc2 which broke things because omap-pcm was using 64bits for
> dma coherent mask.

Can you please try to understand _why_ it broke and post an explanation.
This breakage wasn't expected and shouldn't have happened.

Re: [PATCH] ASoC: omap-pcm: Lower the dma coherent mask to 32bits

[Peter Ujfalusi]

On 12/05/2013 11:56 AM, Russell King - ARM Linux wrote:
> On Thu, Dec 05, 2013 at 10:06:42AM +0200, Peter Ujfalusi wrote:
>> The underlying API dma_coerce_mask_and_coherent() checks the requested bits
>> mask against the size of dma_addr_t which is 32bits on OMAP.
>> Because of the previously used 64bits mask audio was not probing after
>> commit c9bd5e6 (and 4dcfa6007).
>> 32bits for the DMA_BIT_MASK looks to be the correct one to use.
>>
>> Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
>> ---
>> Hi Mark,
>>
>> This is the fix for OMAP audio for 3.13-rc2+. The dma coherent changes went in
>> between -rc1 and -rc2 which broke things because omap-pcm was using 64bits for
>> dma coherent mask.
> 
> Can you please try to understand _why_ it broke and post an explanation.
> This breakage wasn't expected and shouldn't have happened.

I'm not that familiar with this part of the code (mm, dma-mapping) but so far
this is what I found:

ret = dma_coerce_mask_and_coherent(card->dev, DMA_BIT_MASK(64));
is successful, no failure to set the mask to 64.

later on when requesting the dma channel however:
arm_dma_alloc() -> get_coherent_dma_mask()  fails.

As far I can see we have different checks in case of
dma_coerce_mask_and_coherent() and arm_dma_alloc():

[1] dma_coerce_mask_and_coherent() -> dma_supported()

 	if (sizeof(mask) != sizeof(dma_addr_t) &&
 	    mask > (dma_addr_t)~0 &&
 	    dma_to_pfn(dev, ~0) > arm_dma_pfn_limit)  /* !!! */
 		return 0;


[2] arm_dma_alloc() -> get_coherent_dma_mask()

 	if (sizeof(mask) != sizeof(dma_addr_t) &&
 	    mask > (dma_addr_t)~0 &&
 	    dma_to_pfn(dev, ~0) > min(max_pfn, arm_dma_pfn_limit)) /* !!! */
 		return 0;

On omap4:
max_pfn: 0xc0000, arm_dma_pfn_limit: 0xfffff

Not sure which check is the correct one but I think in both cases we should
have the same check in this way we can catch the issue at
dma_coerce_mask_and_coherent() time and try to figure out what to do.
In case of omap-pcm we request for 64 bit because of future SoCs, but I think
it would be fine to try first 64 and in case it is not supported fall back to 32.

-- 
Péter

Re: [PATCH] ASoC: omap-pcm: Lower the dma coherent mask to 32bits

[Russell King - ARM Linux]

On Thu, Dec 05, 2013 at 05:33:46PM +0200, Peter Ujfalusi wrote:
> I'm not that familiar with this part of the code (mm, dma-mapping) but so far
> this is what I found:
> 
> ret = dma_coerce_mask_and_coherent(card->dev, DMA_BIT_MASK(64));
> is successful, no failure to set the mask to 64.
> 
> later on when requesting the dma channel however:
> arm_dma_alloc() -> get_coherent_dma_mask()  fails.
> 
> As far I can see we have different checks in case of
> dma_coerce_mask_and_coherent() and arm_dma_alloc():
> 
> [1] dma_coerce_mask_and_coherent() -> dma_supported()
> 
>  	if (sizeof(mask) != sizeof(dma_addr_t) &&
>  	    mask > (dma_addr_t)~0 &&
>  	    dma_to_pfn(dev, ~0) > arm_dma_pfn_limit)  /* !!! */
>  		return 0;
> 
> 
> [2] arm_dma_alloc() -> get_coherent_dma_mask()
> 
>  	if (sizeof(mask) != sizeof(dma_addr_t) &&
>  	    mask > (dma_addr_t)~0 &&
>  	    dma_to_pfn(dev, ~0) > min(max_pfn, arm_dma_pfn_limit)) /* !!! */
>  		return 0;
> 
> On omap4:
> max_pfn: 0xc0000, arm_dma_pfn_limit: 0xfffff

Obviously, we should not be saying that the mask is okay, and then failing
with that same mask.

I've thinking about what the right fix here is - it's been quite a while
since I devised those tests and the knowledge I had back then has been
swapped out...

The point of these tests are to deal with masks which are larger than
dma_addr_t allows, and to only deny if we have sufficient system memory
that we may overflow dma_addr_t.

So...

	sizeof(u64) != sizeof(dma_addr_t)

detects when we have non-64 bit dma_addr_t.

	mask > (dma_addr_t)~0

detects if the mask is larger than 4GiB.

	dma_to_pfn(dev, ~0)

gives us the very last PFN which the bus associated with dev is able to
address.  Hmm - so I got the test wrong on both twice - they should both
be:

 	if (sizeof(mask) != sizeof(dma_addr_t) &&
 	    mask > (dma_addr_t)~0 &&
 	    dma_to_pfn(dev, ~0) < min(max_pfn, arm_dma_pfn_limit))
		return 0;

since we want to fail if we have _more_ memory than the bus will allow.
Can you try the above in both locations please?

Re: [PATCH] ASoC: omap-pcm: Lower the dma coherent mask to 32bits

[Takashi Iwai]

At Thu, 5 Dec 2013 19:28:53 +0000,
Russell King - ARM Linux wrote:
> 
> On Thu, Dec 05, 2013 at 05:33:46PM +0200, Peter Ujfalusi wrote:
> > I'm not that familiar with this part of the code (mm, dma-mapping) but so far
> > this is what I found:
> > 
> > ret = dma_coerce_mask_and_coherent(card->dev, DMA_BIT_MASK(64));
> > is successful, no failure to set the mask to 64.
> > 
> > later on when requesting the dma channel however:
> > arm_dma_alloc() -> get_coherent_dma_mask()  fails.
> > 
> > As far I can see we have different checks in case of
> > dma_coerce_mask_and_coherent() and arm_dma_alloc():
> > 
> > [1] dma_coerce_mask_and_coherent() -> dma_supported()
> > 
> >  	if (sizeof(mask) != sizeof(dma_addr_t) &&
> >  	    mask > (dma_addr_t)~0 &&
> >  	    dma_to_pfn(dev, ~0) > arm_dma_pfn_limit)  /* !!! */
> >  		return 0;
> > 
> > 
> > [2] arm_dma_alloc() -> get_coherent_dma_mask()
> > 
> >  	if (sizeof(mask) != sizeof(dma_addr_t) &&
> >  	    mask > (dma_addr_t)~0 &&
> >  	    dma_to_pfn(dev, ~0) > min(max_pfn, arm_dma_pfn_limit)) /* !!! */
> >  		return 0;
> > 
> > On omap4:
> > max_pfn: 0xc0000, arm_dma_pfn_limit: 0xfffff
> 
> Obviously, we should not be saying that the mask is okay, and then failing
> with that same mask.
> 
> I've thinking about what the right fix here is - it's been quite a while
> since I devised those tests and the knowledge I had back then has been
> swapped out...
> 
> The point of these tests are to deal with masks which are larger than
> dma_addr_t allows, and to only deny if we have sufficient system memory
> that we may overflow dma_addr_t.
> 
> So...
> 
> 	sizeof(u64) != sizeof(dma_addr_t)
> 
> detects when we have non-64 bit dma_addr_t.
> 
> 	mask > (dma_addr_t)~0
> 
> detects if the mask is larger than 4GiB.
> 
> 	dma_to_pfn(dev, ~0)
> 
> gives us the very last PFN which the bus associated with dev is able to
> address.  Hmm - so I got the test wrong on both twice - they should both
> be:
> 
>  	if (sizeof(mask) != sizeof(dma_addr_t) &&
>  	    mask > (dma_addr_t)~0 &&
>  	    dma_to_pfn(dev, ~0) < min(max_pfn, arm_dma_pfn_limit))
> 		return 0;
> 
> since we want to fail if we have _more_ memory than the bus will allow.
> Can you try the above in both locations please?

Does the check of dma_to_pfn(dev, ~0) is really necessary?
In get_coherent_dma_mask(), it checks further

		if (dma_to_pfn(dev, mask) < max_dma_pfn) {

and since mask > ~0, this check should suffice, I think.

And this made me wonder why can we simply use dma_supported() for the
check in get_coherent_dma_mask().  If the check in get_coherent_mask()
must be different, it'd be helpful if you comment on it there, too.


thanks,

Takashi

Re: [PATCH] ASoC: omap-pcm: Lower the dma coherent mask to 32bits

[Russell King - ARM Linux]

On Thu, Dec 05, 2013 at 09:11:50PM +0100, Takashi Iwai wrote:
> Does the check of dma_to_pfn(dev, ~0) is really necessary?

Of course.

> In get_coherent_dma_mask(), it checks further
> 
> 		if (dma_to_pfn(dev, mask) < max_dma_pfn) {
> 
> and since mask > ~0, this check should suffice, I think.

dma_to_pfn() takes a dma_addr_t as the second argument.  At this point,
we've ascertained that 'mask' is larger than dma_addr_t, so the above
will truncate the mask.

The whole point of this is to protect the following code which does
exactly that from this truncation.

Re: [PATCH] ASoC: omap-pcm: Lower the dma coherent mask to 32bits

[Takashi Iwai]

At Thu, 5 Dec 2013 21:07:07 +0000,
Russell King - ARM Linux wrote:
> 
> On Thu, Dec 05, 2013 at 09:11:50PM +0100, Takashi Iwai wrote:
> > Does the check of dma_to_pfn(dev, ~0) is really necessary?
> 
> Of course.
> 
> > In get_coherent_dma_mask(), it checks further
> > 
> > 		if (dma_to_pfn(dev, mask) < max_dma_pfn) {
> > 
> > and since mask > ~0, this check should suffice, I think.
> 
> dma_to_pfn() takes a dma_addr_t as the second argument.  At this point,
> we've ascertained that 'mask' is larger than dma_addr_t, so the above
> will truncate the mask.

Ah, I missed that point...

> The whole point of this is to protect the following code which does
> exactly that from this truncation.

OK, I understand that part.  Thanks for clarification!


But, it's still unclear why only get_coherent_dma_mask() takes max_pfn
into account for the check of dma_to_pfn(dev, mask).

In dma_supported(), 

	unsigned long limit;
	limit = dma_to_pfn(dev, mask);
	if (limit < arm_dma_pfn_limit)
		return 0;

while in get_coherent_dma_mask(),

	unsigned long max_dma_pfn;
	max_dma_pfn = min(max_pfn, arm_dma_pfn_limit);
	if (dma_to_pfn(dev, mask) < max_dma_pfn)
		return 0;

So, the current code looks to me that the results from
dma_set_coherent_mask() and the actual allocation may conflict.

If adding warnings is the purpose of the open code, we can create a
function like __dma_supported(struct device *dev, u64 mask, bool warning)
and call it from the both places.


Takashi

Re: [PATCH] ASoC: omap-pcm: Lower the dma coherent mask to 32bits

[Russell King - ARM Linux]

On Fri, Dec 06, 2013 at 09:12:22AM +0100, Takashi Iwai wrote:
> But, it's still unclear why only get_coherent_dma_mask() takes max_pfn
> into account for the check of dma_to_pfn(dev, mask).
> 
> In dma_supported(), 
> 
> 	unsigned long limit;
> 	limit = dma_to_pfn(dev, mask);
> 	if (limit < arm_dma_pfn_limit)
> 		return 0;
> 
> while in get_coherent_dma_mask(),
> 
> 	unsigned long max_dma_pfn;
> 	max_dma_pfn = min(max_pfn, arm_dma_pfn_limit);
> 	if (dma_to_pfn(dev, mask) < max_dma_pfn)
> 		return 0;
> 
> So, the current code looks to me that the results from
> dma_set_coherent_mask() and the actual allocation may conflict.

I did ask Peter to replace *both* with the same thing.

Re: [PATCH] ASoC: omap-pcm: Lower the dma coherent mask to 32bits

[Takashi Iwai]

At Fri, 6 Dec 2013 12:25:43 +0000,
Russell King - ARM Linux wrote:
> 
> On Fri, Dec 06, 2013 at 09:12:22AM +0100, Takashi Iwai wrote:
> > But, it's still unclear why only get_coherent_dma_mask() takes max_pfn
> > into account for the check of dma_to_pfn(dev, mask).
> > 
> > In dma_supported(), 
> > 
> > 	unsigned long limit;
> > 	limit = dma_to_pfn(dev, mask);
> > 	if (limit < arm_dma_pfn_limit)
> > 		return 0;
> > 
> > while in get_coherent_dma_mask(),
> > 
> > 	unsigned long max_dma_pfn;
> > 	max_dma_pfn = min(max_pfn, arm_dma_pfn_limit);
> > 	if (dma_to_pfn(dev, mask) < max_dma_pfn)
> > 		return 0;
> > 
> > So, the current code looks to me that the results from
> > dma_set_coherent_mask() and the actual allocation may conflict.
> 
> I did ask Peter to replace *both* with the same thing.

Well, I'm looking at different points.  You requested Peter to test
both functions to take:

 	if (sizeof(mask) != sizeof(dma_addr_t) &&
 	    mask > (dma_addr_t)~0 &&
 	    dma_to_pfn(dev, ~0) < min(max_pfn, arm_dma_pfn_limit))
		return 0;

But, after that line, dma_supported() has another check:

 	if (dma_to_pfn(dev, mask) < arm_dma_pfn_limit)
 		return 0;

and get_coherent_dma_mask() has a different check:

 	if (dma_to_pfn(dev, mask) < min(max_pfn, arm_dma_pfn_limit))
 		return 0;

(the expressions here are expanded for easier comparison)

This is what I'm wondering.


Takashi

Re: [PATCH] ASoC: omap-pcm: Lower the dma coherent mask to 32bits

[Peter Ujfalusi]

On 12/06/2013 03:04 PM, Takashi Iwai wrote:
> At Fri, 6 Dec 2013 12:25:43 +0000,
> Russell King - ARM Linux wrote:
>>
>> On Fri, Dec 06, 2013 at 09:12:22AM +0100, Takashi Iwai wrote:
>>> But, it's still unclear why only get_coherent_dma_mask() takes max_pfn
>>> into account for the check of dma_to_pfn(dev, mask).
>>>
>>> In dma_supported(), 
>>>
>>> 	unsigned long limit;
>>> 	limit = dma_to_pfn(dev, mask);
>>> 	if (limit < arm_dma_pfn_limit)
>>> 		return 0;
>>>
>>> while in get_coherent_dma_mask(),
>>>
>>> 	unsigned long max_dma_pfn;
>>> 	max_dma_pfn = min(max_pfn, arm_dma_pfn_limit);
>>> 	if (dma_to_pfn(dev, mask) < max_dma_pfn)
>>> 		return 0;
>>>
>>> So, the current code looks to me that the results from
>>> dma_set_coherent_mask() and the actual allocation may conflict.
>>
>> I did ask Peter to replace *both* with the same thing.
> 
> Well, I'm looking at different points.  You requested Peter to test
> both functions to take:
> 
>  	if (sizeof(mask) != sizeof(dma_addr_t) &&
>  	    mask > (dma_addr_t)~0 &&
>  	    dma_to_pfn(dev, ~0) < min(max_pfn, arm_dma_pfn_limit))
> 		return 0;
> 
> But, after that line, dma_supported() has another check:
> 
>  	if (dma_to_pfn(dev, mask) < arm_dma_pfn_limit)
>  		return 0;
> 
> and get_coherent_dma_mask() has a different check:
> 
>  	if (dma_to_pfn(dev, mask) < min(max_pfn, arm_dma_pfn_limit))
>  		return 0;
> 
> (the expressions here are expanded for easier comparison)
> 
> This is what I'm wondering.


The tests in get_coherent_dma_mask() and dma_supported() should be identical.
Fixing the dma_supported() checks and calling dma_supported() from
get_coherent_dma_mask() instead of doing the same test coded locally there
should be the preferred solution.

I think dma_supported() should be like this:

int dma_supported(struct device *dev, u64 mask)
{
	unsigned long max_dma_pfn;

	/*
	 * 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.
	 */
	max_dma_pfn = min(max_pfn, arm_dma_pfn_limit);
	if (sizeof(mask) != sizeof(dma_addr_t) &&
	    mask > (dma_addr_t)~0 &&
	    dma_to_pfn(dev, ~0) < max_dma_pfn)
		return 0;

	/*
	 * Translate the device's DMA mask to a PFN limit.  This
	 * PFN number includes the page which we can DMA to.
	 */
	if (dma_to_pfn(dev, mask) < max_dma_pfn)
		return 0;

	return 1;
}


-- 
Péter

Re: [PATCH] ASoC: omap-pcm: Lower the dma coherent mask to 32bits

[Russell King - ARM Linux]

On Fri, Dec 06, 2013 at 09:12:22AM +0100, Takashi Iwai wrote:
> But, it's still unclear why only get_coherent_dma_mask() takes max_pfn
> into account for the check of dma_to_pfn(dev, mask).
> 
> In dma_supported(), 
> 
> 	unsigned long limit;
> 	limit = dma_to_pfn(dev, mask);
> 	if (limit < arm_dma_pfn_limit)
> 		return 0;
> 
> while in get_coherent_dma_mask(),
> 
> 	unsigned long max_dma_pfn;
> 	max_dma_pfn = min(max_pfn, arm_dma_pfn_limit);
> 	if (dma_to_pfn(dev, mask) < max_dma_pfn)
> 		return 0;
> 
> So, the current code looks to me that the results from
> dma_set_coherent_mask() and the actual allocation may conflict.

Yes, that's also wrong.  Both should be the same, and both should take
max_pfn into account.

The reason why max_pfn needs to be taken into account is that is the top
of memory - arm_dma_pfn_limit is hard-coded to 4GiB if there's no DMA
zone.  With some buses (eg, ONAP) this results in a failure as
dma_to_pfn(dev, 32-bit) is less than 4GiB.

> If adding warnings is the purpose of the open code, we can create a
> function like __dma_supported(struct device *dev, u64 mask, bool warning)
> and call it from the both places.

Yes, that's a good idea.  Revised version of the patch.  Peter, can you
retest please, thanks.

diff --git a/arch/arm/mm/dma-mapping.c b/arch/arm/mm/dma-mapping.c
index f6b6bfa88ecf..86c564e52ea7 100644
--- a/arch/arm/mm/dma-mapping.c
+++ b/arch/arm/mm/dma-mapping.c
@@ -158,6 +158,44 @@ struct dma_map_ops arm_coherent_dma_ops = {
 };
 EXPORT_SYMBOL(arm_coherent_dma_ops);
 
+static int __dma_supported(struct device *dev, u64 mask, bool warn)
+{
+	unsigned long max_dma_pfn;
+
+	/*
+	 * 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) < max_pfn) {
+		if (warn) {
+			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;
+	}
+
+	max_dma_pfn = min(max_pfn, arm_dma_pfn_limit);
+
+	/*
+	 * Translate the device's DMA mask to a PFN limit.  This
+	 * PFN number includes the page which we can DMA to.
+	 */
+	if (dma_to_pfn(dev, mask) < max_dma_pfn) {
+		if (warn)
+			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,
+				 max_dma_pfn + 1);
+		return 0;
+	}
+
+	return 1;
+}
+
 static u64 get_coherent_dma_mask(struct device *dev)
 {
 	u64 mask = (u64)DMA_BIT_MASK(32);
@@ -176,34 +214,8 @@ static u64 get_coherent_dma_mask(struct device *dev)
 			return 0;
 		}
 
-		max_dma_pfn = min(max_pfn, arm_dma_pfn_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) > max_dma_pfn) {
-			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) < max_dma_pfn) {
-			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);
+		if (!__dma_supported(dev, mask, true))
 			return 0;
-		}
 	}
 
 	return mask;
@@ -1032,28 +1044,7 @@ void arm_dma_sync_sg_for_device(struct device *dev, struct scatterlist *sg,
  */
 int dma_supported(struct device *dev, u64 mask)
 {
-	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;
+	return __dma_supported(dev, mask, false);
 }
 EXPORT_SYMBOL(dma_supported);
 

Re: [PATCH] ASoC: omap-pcm: Lower the dma coherent mask to 32bits

[Peter Ujfalusi]

Hi Russell,

On 12/09/2013 07:03 PM, Russell King - ARM Linux wrote:
> Yes, that's a good idea.  Revised version of the patch.  Peter, can you
> retest please, thanks.

With this patch PandaES and BeagleXM works fine on top of today's mainline.

Thank you Russell and Takashi!

Tested-by: Peter Ujfalusi <peter.ujfalusi@ti.com>

> diff --git a/arch/arm/mm/dma-mapping.c b/arch/arm/mm/dma-mapping.c
> index f6b6bfa88ecf..86c564e52ea7 100644
> --- a/arch/arm/mm/dma-mapping.c
> +++ b/arch/arm/mm/dma-mapping.c
> @@ -158,6 +158,44 @@ struct dma_map_ops arm_coherent_dma_ops = {
>  };
>  EXPORT_SYMBOL(arm_coherent_dma_ops);
>  
> +static int __dma_supported(struct device *dev, u64 mask, bool warn)
> +{
> +	unsigned long max_dma_pfn;
> +
> +	/*
> +	 * 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) < max_pfn) {
> +		if (warn) {
> +			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;
> +	}
> +
> +	max_dma_pfn = min(max_pfn, arm_dma_pfn_limit);
> +
> +	/*
> +	 * Translate the device's DMA mask to a PFN limit.  This
> +	 * PFN number includes the page which we can DMA to.
> +	 */
> +	if (dma_to_pfn(dev, mask) < max_dma_pfn) {
> +		if (warn)
> +			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,
> +				 max_dma_pfn + 1);
> +		return 0;
> +	}
> +
> +	return 1;
> +}
> +
>  static u64 get_coherent_dma_mask(struct device *dev)
>  {
>  	u64 mask = (u64)DMA_BIT_MASK(32);
> @@ -176,34 +214,8 @@ static u64 get_coherent_dma_mask(struct device *dev)
>  			return 0;
>  		}
>  
> -		max_dma_pfn = min(max_pfn, arm_dma_pfn_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) > max_dma_pfn) {
> -			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) < max_dma_pfn) {
> -			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);
> +		if (!__dma_supported(dev, mask, true))
>  			return 0;
> -		}
>  	}
>  
>  	return mask;
> @@ -1032,28 +1044,7 @@ void arm_dma_sync_sg_for_device(struct device *dev, struct scatterlist *sg,
>   */
>  int dma_supported(struct device *dev, u64 mask)
>  {
> -	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;
> +	return __dma_supported(dev, mask, false);
>  }
>  EXPORT_SYMBOL(dma_supported);
>  
> 


-- 
Péter

Re: [PATCH] ASoC: omap-pcm: Lower the dma coherent mask to 32bits

[Peter Ujfalusi]

One note for the patch:

On 12/10/2013 11:37 AM, Peter Ujfalusi wrote:
> Hi Russell,
> 
> On 12/09/2013 07:03 PM, Russell King - ARM Linux wrote:
>> Yes, that's a good idea.  Revised version of the patch.  Peter, can you
>> retest please, thanks.
> 
> With this patch PandaES and BeagleXM works fine on top of today's mainline.
> 
> Thank you Russell and Takashi!
> 
> Tested-by: Peter Ujfalusi <peter.ujfalusi@ti.com>


> 
>> diff --git a/arch/arm/mm/dma-mapping.c b/arch/arm/mm/dma-mapping.c
>> index f6b6bfa88ecf..86c564e52ea7 100644
>> --- a/arch/arm/mm/dma-mapping.c
>> +++ b/arch/arm/mm/dma-mapping.c
>> @@ -158,6 +158,44 @@ struct dma_map_ops arm_coherent_dma_ops = {
>>  };
>>  EXPORT_SYMBOL(arm_coherent_dma_ops);
>>  
>> +static int __dma_supported(struct device *dev, u64 mask, bool warn)
>> +{
>> +	unsigned long max_dma_pfn;
>> +
>> +	/*
>> +	 * 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) < max_pfn) {
>> +		if (warn) {
>> +			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;
>> +	}
>> +
>> +	max_dma_pfn = min(max_pfn, arm_dma_pfn_limit);
>> +
>> +	/*
>> +	 * Translate the device's DMA mask to a PFN limit.  This
>> +	 * PFN number includes the page which we can DMA to.
>> +	 */
>> +	if (dma_to_pfn(dev, mask) < max_dma_pfn) {
>> +		if (warn)
>> +			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,
>> +				 max_dma_pfn + 1);
>> +		return 0;
>> +	}
>> +
>> +	return 1;
>> +}
>> +
>>  static u64 get_coherent_dma_mask(struct device *dev)
>>  {
>>  	u64 mask = (u64)DMA_BIT_MASK(32);

  CC      arch/arm/mach-omap2/control.o
arch/arm/mm/dma-mapping.c: In function ‘get_coherent_dma_mask’:
arch/arm/mm/dma-mapping.c:204:17: warning: unused variable ‘max_dma_pfn’ [-Wunused-variable]
   unsigned long max_dma_pfn;
                 ^


>> @@ -176,34 +214,8 @@ static u64 get_coherent_dma_mask(struct device *dev)
>>  			return 0;
>>  		}
>>  
>> -		max_dma_pfn = min(max_pfn, arm_dma_pfn_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) > max_dma_pfn) {
>> -			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) < max_dma_pfn) {
>> -			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);
>> +		if (!__dma_supported(dev, mask, true))
>>  			return 0;
>> -		}
>>  	}
>>  
>>  	return mask;
>> @@ -1032,28 +1044,7 @@ void arm_dma_sync_sg_for_device(struct device *dev, struct scatterlist *sg,
>>   */
>>  int dma_supported(struct device *dev, u64 mask)
>>  {
>> -	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;
>> +	return __dma_supported(dev, mask, false);
>>  }
>>  EXPORT_SYMBOL(dma_supported);
>>  
>>
> 
> 


-- 
Péter

Re: [PATCH] ASoC: omap-pcm: Lower the dma coherent mask to 32bits

[Peter Ujfalusi]

On 12/10/2013 11:37 AM, Peter Ujfalusi wrote:
> Hi Russell,
> 
> On 12/09/2013 07:03 PM, Russell King - ARM Linux wrote:
>> Yes, that's a good idea.  Revised version of the patch.  Peter, can you
>> retest please, thanks.
> 
> With this patch PandaES and BeagleXM works fine on top of today's mainline.
> 
> Thank you Russell and Takashi!
> 
> Tested-by: Peter Ujfalusi <peter.ujfalusi@ti.com>

Russell,

are you planning to send this for 3.13? Without the patch all OMAP audio fails
to probe in 3.13 kernel.

-- 
Péter

> 
>> diff --git a/arch/arm/mm/dma-mapping.c b/arch/arm/mm/dma-mapping.c
>> index f6b6bfa88ecf..86c564e52ea7 100644
>> --- a/arch/arm/mm/dma-mapping.c
>> +++ b/arch/arm/mm/dma-mapping.c
>> @@ -158,6 +158,44 @@ struct dma_map_ops arm_coherent_dma_ops = {
>>  };
>>  EXPORT_SYMBOL(arm_coherent_dma_ops);
>>  
>> +static int __dma_supported(struct device *dev, u64 mask, bool warn)
>> +{
>> +	unsigned long max_dma_pfn;
>> +
>> +	/*
>> +	 * 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) < max_pfn) {
>> +		if (warn) {
>> +			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;
>> +	}
>> +
>> +	max_dma_pfn = min(max_pfn, arm_dma_pfn_limit);
>> +
>> +	/*
>> +	 * Translate the device's DMA mask to a PFN limit.  This
>> +	 * PFN number includes the page which we can DMA to.
>> +	 */
>> +	if (dma_to_pfn(dev, mask) < max_dma_pfn) {
>> +		if (warn)
>> +			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,
>> +				 max_dma_pfn + 1);
>> +		return 0;
>> +	}
>> +
>> +	return 1;
>> +}
>> +
>>  static u64 get_coherent_dma_mask(struct device *dev)
>>  {
>>  	u64 mask = (u64)DMA_BIT_MASK(32);
>> @@ -176,34 +214,8 @@ static u64 get_coherent_dma_mask(struct device *dev)
>>  			return 0;
>>  		}
>>  
>> -		max_dma_pfn = min(max_pfn, arm_dma_pfn_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) > max_dma_pfn) {
>> -			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) < max_dma_pfn) {
>> -			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);
>> +		if (!__dma_supported(dev, mask, true))
>>  			return 0;
>> -		}
>>  	}
>>  
>>  	return mask;
>> @@ -1032,28 +1044,7 @@ void arm_dma_sync_sg_for_device(struct device *dev, struct scatterlist *sg,
>>   */
>>  int dma_supported(struct device *dev, u64 mask)
>>  {
>> -	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;
>> +	return __dma_supported(dev, mask, false);
>>  }
>>  EXPORT_SYMBOL(dma_supported);
>>  
>>
> 
> 

Re: [PATCH] ASoC: omap-pcm: Lower the dma coherent mask to 32bits

[Russell King - ARM Linux]

On Fri, Dec 13, 2013 at 01:46:46PM +0200, Peter Ujfalusi wrote:
> On 12/10/2013 11:37 AM, Peter Ujfalusi wrote:
> > Hi Russell,
> > 
> > On 12/09/2013 07:03 PM, Russell King - ARM Linux wrote:
> >> Yes, that's a good idea.  Revised version of the patch.  Peter, can you
> >> retest please, thanks.
> > 
> > With this patch PandaES and BeagleXM works fine on top of today's mainline.
> > 
> > Thank you Russell and Takashi!
> > 
> > Tested-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
> 
> Russell,
> 
> are you planning to send this for 3.13? Without the patch all OMAP audio fails
> to probe in 3.13 kernel.

It's queued up in -fixes, but there's a few other fixes in progress which
I want to get together before I finally push it to Linus.

Re: [PATCH] ASoC: omap-pcm: Lower the dma coherent mask to 32bits

[Peter Ujfalusi]

On 12/13/2013 01:49 PM, Russell King - ARM Linux wrote:
> On Fri, Dec 13, 2013 at 01:46:46PM +0200, Peter Ujfalusi wrote:
>> On 12/10/2013 11:37 AM, Peter Ujfalusi wrote:
>>> Hi Russell,
>>>
>>> On 12/09/2013 07:03 PM, Russell King - ARM Linux wrote:
>>>> Yes, that's a good idea.  Revised version of the patch.  Peter, can you
>>>> retest please, thanks.
>>>
>>> With this patch PandaES and BeagleXM works fine on top of today's mainline.
>>>
>>> Thank you Russell and Takashi!
>>>
>>> Tested-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
>>
>> Russell,
>>
>> are you planning to send this for 3.13? Without the patch all OMAP audio fails
>> to probe in 3.13 kernel.
> 
> It's queued up in -fixes, but there's a few other fixes in progress which
> I want to get together before I finally push it to Linus.

Sure, the important thing is that it is going.

Thank you,
Péter

Re: [PATCH] ASoC: omap-pcm: Lower the dma coherent mask to 32bits

[Peter Ujfalusi]

On 12/05/2013 09:28 PM, Russell King - ARM Linux wrote:
> gives us the very last PFN which the bus associated with dev is able to
> address.  Hmm - so I got the test wrong on both twice - they should both
> be:
> 
>  	if (sizeof(mask) != sizeof(dma_addr_t) &&
>  	    mask > (dma_addr_t)~0 &&
>  	    dma_to_pfn(dev, ~0) < min(max_pfn, arm_dma_pfn_limit))
> 		return 0;
> 
> since we want to fail if we have _more_ memory than the bus will allow.
> Can you try the above in both locations please?

Thanks, works fine.
I will be out for a long weekend so you can add my:

Tested-by: Peter Ujfalusi <peter.ujfalusi@ti.com>

to the final patch.

-- 
Péter

Re: [PATCH] ASoC: omap-pcm: Lower the dma coherent mask to 32bits

[Russell King - ARM Linux]

On Fri, Dec 06, 2013 at 11:31:52AM +0200, Peter Ujfalusi wrote:
> On 12/05/2013 09:28 PM, Russell King - ARM Linux wrote:
> > gives us the very last PFN which the bus associated with dev is able to
> > address.  Hmm - so I got the test wrong on both twice - they should both
> > be:
> > 
> >  	if (sizeof(mask) != sizeof(dma_addr_t) &&
> >  	    mask > (dma_addr_t)~0 &&
> >  	    dma_to_pfn(dev, ~0) < min(max_pfn, arm_dma_pfn_limit))
> > 		return 0;
> > 
> > since we want to fail if we have _more_ memory than the bus will allow.
> > Can you try the above in both locations please?
> 
> Thanks, works fine.
> I will be out for a long weekend so you can add my:
> 
> Tested-by: Peter Ujfalusi <peter.ujfalusi@ti.com>

Thanks Peter.