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; }