On Thu, Mar 17, 2016 at 07:06:27PM +0800, Leizhen (ThunderTown) wrote: > On 2016/3/16 9:56, Leizhen (ThunderTown) wrote: > > On 2016/3/15 23:37, Catalin Marinas wrote: > >> On Tue, Mar 15, 2016 at 10:12:11AM +0800, Zhen Lei wrote: > >>> diff --git a/arch/arm64/mm/dma-mapping.c b/arch/arm64/mm/dma-mapping.c > >>> index a6e757c..b2f2834 100644 > >>> --- a/arch/arm64/mm/dma-mapping.c > >>> +++ b/arch/arm64/mm/dma-mapping.c > >>> @@ -187,8 +187,6 @@ static void __dma_free(struct device *dev, size_t > >>> size, > >>> void *vaddr, dma_addr_t dma_handle, > >>> struct dma_attrs *attrs) > >>> { > >>> - void *swiotlb_addr = phys_to_virt(dma_to_phys(dev, dma_handle)); > >>> - > >>> size = PAGE_ALIGN(size); > >>> > >>> if (!is_device_dma_coherent(dev)) { > >>> @@ -196,7 +194,7 @@ static void __dma_free(struct device *dev, size_t > >>> size, > >>> return; > >>> vunmap(vaddr); > >>> } > >>> - __dma_free_coherent(dev, size, swiotlb_addr, dma_handle, attrs); > >>> + __dma_free_coherent(dev, size, vaddr, dma_handle, attrs); > >>> } > >> > >> What happens when !is_device_dma_coherent(dev)? (hint: read two lines > >> above __dma_free_coherent). > > Do you afraid "vaddr" maybe modified by these statement? > First, it could not be __free_from_pool. Otherwise, the function > vunmap(which after it) can not work well. Then, it count not be vunmap > too, the parameter is defined as "const void *". > > In the call chain: > __dma_free_coherent-->__dma_free_coherent-->swiotlb_free_coherent, > only swiotlb_free_coherent finally use "vaddr".
Exactly. So you give swiotlb_free_coherent a vaddr which has been unmapped. It doesn't even matter whether it's still mapped since this address is passed further to free_pages() which performs a virt_to_page(). The latter is *only* valid on linear map addresses (and you would actually hit the VM_BUG_ON in free_pages; you can try running this with CONFIG_DEBUG_VM enabled and non-coherent DMA). For non-coherent DMA, the vaddr is not part of the linear mapping as it has been remapped by __dma_alloc() via dma_common_contiguous_remap(), hence for swiotlb freeing we need the actual linear map address (the original "ptr" in __dma_alloc()). We can generate it by a phys_to_virt(dma_to_phys(dma_handle)). -- Catalin