From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754284AbbGQQum (ORCPT ); Fri, 17 Jul 2015 12:50:42 -0400 Received: from mx1.redhat.com ([209.132.183.28]:40080 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752328AbbGQQuk (ORCPT ); Fri, 17 Jul 2015 12:50:40 -0400 Subject: Re: [PATCH] staging: ion: ion_cma_heap: Don't directly use dma_common_get_sgtable To: Robin Murphy , "Jon Medhurst (Tixy)" , Greg Kroah-Hartman , =?UTF-8?Q?Arve_Hj=c3=b8nnev=c3=a5g?= , Riley Andrews References: <1437130889.3221.53.camel@linaro.org> <55A91D5E.9000009@arm.com> Cc: "linux-kernel@vger.kernel.org" , "devel@driverdev.osuosl.org" , Zeng Tao From: Laura Abbott Message-ID: <55A9325E.5030903@redhat.com> Date: Fri, 17 Jul 2015 09:50:38 -0700 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.0.1 MIME-Version: 1.0 In-Reply-To: <55A91D5E.9000009@arm.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 07/17/2015 08:21 AM, Robin Murphy wrote: > Hi Tixy, > > On 17/07/15 12:01, Jon Medhurst (Tixy) wrote: >> Use dma_get_sgtable rather than dma_common_get_sgtable so a device's >> dma_ops aren't bypassed. This is essential in situations where a device >> uses an IOMMU and the physical memory is not contiguous (as the common >> function assumes). >> >> Signed-off-by: Jon Medhurst > > The lack of obvious users of this code makes it hard to tell if "dev" > hereis always the right, real, device pointer and never null or some > dummy device with the wrong dma_ops, but the rest of the calls in this > file are to the proper DMA API interface so at least this patch definitely > makes things less wrong in that respect. > Ion currently lacks any standard way to set up heaps and associate a device with a heap. This means it's basically a free for all for what devices get associated (getting something mainlined might help...). I agree that using the proper DMA APIs is a step in the right direction. > Reviewed-by: Robin Murphy > >> --- >> >> This also begs the question as to what happens if the memory region _is_ >> contiguous but is in highmem or an ioremapped region. Should a device >> always provide dma_ops for that case? Because I believe the current >> implementation of dma_common_get_sgtable won't work for those as it uses >> virt_to_page. >> >> I see that this point has been raised before [1] by Zeng Tao, and I >> myself have been given a different fix to apply to a Linaro kernel tree. >> However, both solutions looked wrong to me as they treat a dma_addr_t as >> a physical address, so should at least be using dma_to_phys. >> So, should we fix dma_common_get_sgtable or mandate that the device >> has dma_ops? The latter seems to be implied by the commit message which >> introduced the function: >> >> This patch provides a generic implementation based on >> virt_to_page() call. Architectures which require more >> sophisticated translation might provide their own get_sgtable() >> methods. > > Given that we're largely here due to having poked this on arm64 systems, > I'm inclined to think that implementing our own get_sgtable as per arch/arm > is the right course of action. Since a lot of architectures using > dma_common_get_sgtable don't even implement dma_to_phys, I don't think it > would be right to try complicating the common code for a case that seems to > be all but common. I can spin an arm64 patch if you like. > This would be hit on any system that has non-coherent DMA or highmem. I'm not sure I agree this isn't a common case. How many of the other architectures are actually using the dma_get_sgtable and would have the potential to find a problem? Thanks, Laura