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

[Peter Ujfalusi <peter.ujfalusi@ti.com>]

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