From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752998AbdBUNFn (ORCPT ); Tue, 21 Feb 2017 08:05:43 -0500 Received: from foss.arm.com ([217.140.101.70]:60322 "EHLO foss.arm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752499AbdBUNFR (ORCPT ); Tue, 21 Feb 2017 08:05:17 -0500 Subject: Re: [PATCH 3/7] drivers: dma-coherent: Account dma_pfn_offset when used with device tree To: Robin Murphy , linux-arm-kernel@lists.infradead.org References: <1487152792-34214-1-git-send-email-vladimir.murzin@arm.com> <1487152792-34214-4-git-send-email-vladimir.murzin@arm.com> Cc: Rich Felker , Yoshinori Sato , Greg Kroah-Hartman , linux-kernel@vger.kernel.org, Michal Nazarewicz , linux@armlinux.org.uk, Alan Stern , kbuild-all@01.org, akpm@linux-foundation.org, Roger Quadros , Marek Szyprowski From: Vladimir Murzin Message-ID: Date: Tue, 21 Feb 2017 13:05:12 +0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.7.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 21/02/17 13:02, Vladimir Murzin wrote: > On 21/02/17 12:37, Robin Murphy wrote: >> On 15/02/17 09:59, Vladimir Murzin wrote: >>> dma_declare_coherent_memory() and friends are designed to account >>> difference in CPU and device addresses. However, when it is used with >>> reserved memory regions there is assumption that CPU and device have >>> the same view on address space. This assumption gets invalid when >>> reserved memory for coherent DMA allocations is referenced by device >>> with non-empty "dma-range" property. >>> >>> Simply feeding device address as rmem->base + dev->dma_pfn_offset >>> would not work due to reserved memory region can be shared, so this >>> patch turns device address to be expressed with help of CPU address >>> and device's dma_pfn_offset. >>> >>> For the case where device tree is not used and device sees memory >>> different to CPU we explicitly set device's dma_pfn_offset to >>> accomplish such difference. The latter might look controversial, but >>> it seems only a few drivers set device address different to CPU's: >>> - drivers/usb/host/ohci-sm501.c >>> - arch/sh/drivers/pci/fixups-dreamcast.c >>> so they can be screwed only if dma_pfn_offset there is set and not in >>> sync with device address range - we try to catch such cases with >>> WARN_ON. >>> >>> Cc: Michal Nazarewicz >>> Cc: Marek Szyprowski >>> Cc: Alan Stern >>> Cc: Yoshinori Sato >>> Cc: Rich Felker >>> Cc: Roger Quadros >>> Cc: Greg Kroah-Hartman >>> Tested-by: Benjamin Gaignard >>> Tested-by: Andras Szemzo >>> Tested-by: Alexandre TORGUE >>> Signed-off-by: Vladimir Murzin >>> --- >>> drivers/base/dma-coherent.c | 17 +++++++++++++++-- >>> 1 file changed, 15 insertions(+), 2 deletions(-) >>> >>> diff --git a/drivers/base/dma-coherent.c b/drivers/base/dma-coherent.c >>> index 640a7e6..c59708c 100644 >>> --- a/drivers/base/dma-coherent.c >>> +++ b/drivers/base/dma-coherent.c >>> @@ -18,6 +18,12 @@ struct dma_coherent_mem { >>> spinlock_t spinlock; >>> }; >>> >>> +static inline dma_addr_t dma_get_device_base(struct device *dev, >>> + struct dma_coherent_mem * mem) >>> +{ >>> + return (mem->pfn_base - dev->dma_pfn_offset) << PAGE_SHIFT; >>> +} >>> + >>> static bool dma_init_coherent_memory( >>> phys_addr_t phys_addr, dma_addr_t device_addr, size_t size, int flags, >>> struct dma_coherent_mem **mem) >>> @@ -83,9 +89,16 @@ static void dma_release_coherent_memory(struct dma_coherent_mem *mem) >>> static int dma_assign_coherent_memory(struct device *dev, >>> struct dma_coherent_mem *mem) >>> { >>> + unsigned long dma_pfn_offset = mem->pfn_base - PFN_DOWN(mem->device_base); >>> + >>> if (dev->dma_mem) >>> return -EBUSY; >>> >>> + if (dev->dma_pfn_offset) >>> + WARN_ON(dma_pfn_offset && (dev->dma_pfn_offset != dma_pfn_offset)); >>> + else >>> + dev->dma_pfn_offset = dma_pfn_offset; >> >> This makes me rather uneasy - I can well imagine a device sharing the >> CPU physical address map of external system memory, but having its own >> view of its local coherent memory such that pfn_base != device_base >> still. I know for a fact we've had internal FPGA tiles set up that way, >> although whether it was entirely intentional is another matter... ;) >> >> In that situation, setting dev->dma_pfn_offset like this would break >> streaming DMA for such devices. Could we not keep the pool-specific >> offset and the device-specific offset independent, apply whichever is >> non-zero, and scream if both are set? > > Something like fixup bellow? > > diff --git a/drivers/base/dma-coherent.c b/drivers/base/dma-coherent.c > index 0c577ea..2060010 100644 > --- a/drivers/base/dma-coherent.c > +++ b/drivers/base/dma-coherent.c > @@ -30,7 +30,15 @@ static inline struct dma_coherent_mem *dev_get_coherent_memory(struct device *de > static inline dma_addr_t dma_get_device_base(struct device *dev, > struct dma_coherent_mem * mem) > { > - return (mem->pfn_base - dev->dma_pfn_offset) << PAGE_SHIFT; > + unsigned long dma_pfn_offset = mem->pfn_base - PFN_DOWN(mem->device_base); > + > + if (dma_pfn_offset != dev->dma_pfn_offset ) > + WARN_ON(device_pfn_offset && dev->dma_pfn_offset); ^^^^^^^^^^^^^^^^^ should be dma_pfn_offset > + > + if (dma_pfn_offset) > + return mem->device_base; > + else > + return (mem->pfn_base - dev->dma_pfn_offset) << PAGE_SHIFT; > } > > static bool dma_init_coherent_memory( > @@ -98,19 +106,12 @@ static void dma_release_coherent_memory(struct dma_coherent_mem *mem) > static int dma_assign_coherent_memory(struct device *dev, > struct dma_coherent_mem *mem) > { > - unsigned long dma_pfn_offset = mem->pfn_base - PFN_DOWN(mem->device_base); > - > if (!dev) > return -ENODEV; > > if (dev->dma_mem) > return -EBUSY; > > - if (dev->dma_pfn_offset) > - WARN_ON(dma_pfn_offset && (dev->dma_pfn_offset != dma_pfn_offset)); > - else > - dev->dma_pfn_offset = dma_pfn_offset; > - > dev->dma_mem = mem; > /* FIXME: this routine just ignores DMA_MEMORY_INCLUDES_CHILDREN */ > > > Cheers > Vladimir > >> >> Robin. >> >>> + >>> dev->dma_mem = mem; >>> /* FIXME: this routine just ignores DMA_MEMORY_INCLUDES_CHILDREN */ >>> >>> @@ -133,7 +146,7 @@ void *dma_mark_declared_memory_occupied(struct device *dev, >>> return ERR_PTR(-EINVAL); >>> >>> spin_lock_irqsave(&mem->spinlock, flags); >>> - pos = (device_addr - mem->device_base) >> PAGE_SHIFT; >>> + pos = PFN_DOWN(device_addr - dma_get_device_base(dev, mem)); >>> err = bitmap_allocate_region(mem->bitmap, pos, get_order(size)); >>> spin_unlock_irqrestore(&mem->spinlock, flags); >>> >>> @@ -186,7 +199,7 @@ int dma_alloc_from_coherent(struct device *dev, ssize_t size, >>> /* >>> * Memory was found in the per-device area. >>> */ >>> - *dma_handle = mem->device_base + (pageno << PAGE_SHIFT); >>> + *dma_handle = dma_get_device_base(dev, mem) + (pageno << PAGE_SHIFT); >>> *ret = mem->virt_base + (pageno << PAGE_SHIFT); >>> dma_memory_map = (mem->flags & DMA_MEMORY_MAP); >>> spin_unlock_irqrestore(&mem->spinlock, flags); >>> >> >> > > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel >