From patchwork Tue Feb 21 13:02:06 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Vladimir Murzin X-Patchwork-Id: 9584509 Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork.web.codeaurora.org (Postfix) with ESMTP id B014B602A7 for ; Tue, 21 Feb 2017 13:03:10 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 8F78226E75 for ; Tue, 21 Feb 2017 13:03:10 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id 8414F285AE; Tue, 21 Feb 2017 13:03:10 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-1.9 required=2.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID autolearn=unavailable version=3.3.1 Received: from bombadil.infradead.org (bombadil.infradead.org [65.50.211.133]) (using TLSv1.2 with cipher AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.wl.linuxfoundation.org (Postfix) with ESMTPS id 6DB8C26E75 for ; Tue, 21 Feb 2017 13:03:08 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20170209; h=Sender: Content-Transfer-Encoding:Content-Type:Cc:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:In-Reply-To:MIME-Version:Date: Message-ID:From:References:To:Subject:Reply-To:Content-ID:Content-Description :Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=b30rxLd1ZRMMkRV6e5VpNcGDUaEgKFk397T9rFOJK/w=; b=uN55EN4Q3mKc7F 7mE47rbiYlGoGjPZSQkrK6Sv2m7urZ/X9Lb8LCWqv1mryn4+8XaM8utYeULOW2yBjQJxU/3Nqpi4l 7RmRjN7M8iDBZXo5om9opmg21pvVfBqSTmGvFtHKjPl6kwRHLPRMNk+RjcqWmRhYrPJM3kEGSOLeF sE0Ct6KYHBSA7jWqvXCce9bA4ZawsWxcMpruuMDv/VVGxmNUz/e01xTmXg9+13DxvoccxdlhU0DoL VnWGsGEh7Kt/Almb5GlTzzmR+J/4XH/w1Xat3TZmh+r68BBggOaFQdBnnvTNe+e/FdEfY6I2BmiQt DNzS9zHwnZnWnH1r2DgQ==; Received: from localhost ([127.0.0.1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.87 #1 (Red Hat Linux)) id 1cgA5s-0006Oi-Ua; Tue, 21 Feb 2017 13:03:00 +0000 Received: from foss.arm.com ([217.140.101.70]) by bombadil.infradead.org with esmtp (Exim 4.87 #1 (Red Hat Linux)) id 1cgA5P-0006L0-5m for linux-arm-kernel@lists.infradead.org; Tue, 21 Feb 2017 13:02:37 +0000 Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.72.51.249]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 4BE6E28; Tue, 21 Feb 2017 05:02:10 -0800 (PST) Received: from [10.1.79.56] (unknown [10.1.79.56]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id EC56B3F23B; Tue, 21 Feb 2017 05:02:07 -0800 (PST) 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> From: Vladimir Murzin Message-ID: Date: Tue, 21 Feb 2017 13:02:06 +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: X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20170221_050231_292135_6E4E5549 X-CRM114-Status: GOOD ( 20.29 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , 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 Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+patchwork-linux-arm=patchwork.kernel.org@lists.infradead.org X-Virus-Scanned: ClamAV using ClamSMTP 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? 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); >> > > 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); + + 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 */