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