From mboxrd@z Thu Jan 1 00:00:00 1970 From: Christoph Hellwig Date: Mon, 09 Apr 2018 17:52:51 +0000 Subject: Re: [PATCH] base: dma-mapping: Postpone cpu addr translation on mmap() Message-Id: <20180409175251.GA5426@infradead.org> List-Id: References: <1523293148-18726-1-git-send-email-jacopo+renesas@jmondi.org> In-Reply-To: <1523293148-18726-1-git-send-email-jacopo+renesas-AW8dsiIh9cEdnm+yROfE0A@public.gmane.org> MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: Jacopo Mondi Cc: dalias-8zAoT0mYgF4@public.gmane.org, ysato-Rn4VEauK+AKRv+LV9MX5uooqe+aC9MnS@public.gmane.org, linux-sh-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, linux-renesas-soc-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org, laurent.pinchart-ryLnwIuWjnjg/C1BVhZhaw@public.gmane.org On Mon, Apr 09, 2018 at 06:59:08PM +0200, Jacopo Mondi wrote: > I'm still a bit puzzled on what happens if dma_mmap_from_dev_coherent() fails. > Does a dma_mmap_from_dev_coherent() failure guarantee anyhow that the > successive virt_to_page() isn't problematic as it is today? > Or is it the > if (off < count && user_count <= (count - off)) > check that makes the translation safe? It doesn't. I think one major issue is that we should not simply fall to dma_common_mmap if no method is required, but need every instance of dma_map_ops to explicitly opt into an mmap method that is known to work. > #ifndef CONFIG_ARCH_NO_COHERENT_DMA_MMAP > unsigned long user_count = vma_pages(vma); > unsigned long count = PAGE_ALIGN(size) >> PAGE_SHIFT; > - unsigned long pfn = page_to_pfn(virt_to_page(cpu_addr)); > unsigned long off = vma->vm_pgoff; > + unsigned long pfn; > > vma->vm_page_prot = pgprot_noncached(vma->vm_page_prot); > > @@ -235,6 +235,7 @@ int dma_common_mmap(struct device *dev, struct vm_area_struct *vma, > return ret; > > if (off < count && user_count <= (count - off)) { > + pfn = page_to_pfn(virt_to_page(cpu_addr)); > ret = remap_pfn_range(vma, vma->vm_start, > pfn + off, > user_count << PAGE_SHIFT, Why not: ret = remap_pfn_range(vma, vma->vm_start, page_to_pfn(virt_to_page(cpu_addr)) + off, and save the temp variable? From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752947AbeDIRxG (ORCPT ); Mon, 9 Apr 2018 13:53:06 -0400 Received: from bombadil.infradead.org ([198.137.202.133]:49326 "EHLO bombadil.infradead.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751942AbeDIRxE (ORCPT ); Mon, 9 Apr 2018 13:53:04 -0400 Date: Mon, 9 Apr 2018 10:52:51 -0700 From: Christoph Hellwig To: Jacopo Mondi Cc: laurent.pinchart@ideasonboard.com, robin.murphy@arm.com, dalias@libc.org, ysato@users.sourceforge.jp, linux-sh@vger.kernel.org, linux-kernel@vger.kernel.org, linux-renesas-soc@vger.kernel.org, iommu@lists.linux-foundation.org Subject: Re: [PATCH] base: dma-mapping: Postpone cpu addr translation on mmap() Message-ID: <20180409175251.GA5426@infradead.org> References: <1523293148-18726-1-git-send-email-jacopo+renesas@jmondi.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1523293148-18726-1-git-send-email-jacopo+renesas@jmondi.org> User-Agent: Mutt/1.9.2 (2017-12-15) X-SRS-Rewrite: SMTP reverse-path rewritten from by bombadil.infradead.org. See http://www.infradead.org/rpr.html Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Mon, Apr 09, 2018 at 06:59:08PM +0200, Jacopo Mondi wrote: > I'm still a bit puzzled on what happens if dma_mmap_from_dev_coherent() fails. > Does a dma_mmap_from_dev_coherent() failure guarantee anyhow that the > successive virt_to_page() isn't problematic as it is today? > Or is it the > if (off < count && user_count <= (count - off)) > check that makes the translation safe? It doesn't. I think one major issue is that we should not simply fall to dma_common_mmap if no method is required, but need every instance of dma_map_ops to explicitly opt into an mmap method that is known to work. > #ifndef CONFIG_ARCH_NO_COHERENT_DMA_MMAP > unsigned long user_count = vma_pages(vma); > unsigned long count = PAGE_ALIGN(size) >> PAGE_SHIFT; > - unsigned long pfn = page_to_pfn(virt_to_page(cpu_addr)); > unsigned long off = vma->vm_pgoff; > + unsigned long pfn; > > vma->vm_page_prot = pgprot_noncached(vma->vm_page_prot); > > @@ -235,6 +235,7 @@ int dma_common_mmap(struct device *dev, struct vm_area_struct *vma, > return ret; > > if (off < count && user_count <= (count - off)) { > + pfn = page_to_pfn(virt_to_page(cpu_addr)); > ret = remap_pfn_range(vma, vma->vm_start, > pfn + off, > user_count << PAGE_SHIFT, Why not: ret = remap_pfn_range(vma, vma->vm_start, page_to_pfn(virt_to_page(cpu_addr)) + off, and save the temp variable? From mboxrd@z Thu Jan 1 00:00:00 1970 From: Christoph Hellwig Subject: Re: [PATCH] base: dma-mapping: Postpone cpu addr translation on mmap() Date: Mon, 9 Apr 2018 10:52:51 -0700 Message-ID: <20180409175251.GA5426@infradead.org> References: <1523293148-18726-1-git-send-email-jacopo+renesas@jmondi.org> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Content-Disposition: inline In-Reply-To: <1523293148-18726-1-git-send-email-jacopo+renesas-AW8dsiIh9cEdnm+yROfE0A@public.gmane.org> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: iommu-bounces-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org Errors-To: iommu-bounces-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org To: Jacopo Mondi Cc: dalias-8zAoT0mYgF4@public.gmane.org, ysato-Rn4VEauK+AKRv+LV9MX5uooqe+aC9MnS@public.gmane.org, linux-sh-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, linux-renesas-soc-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org, laurent.pinchart-ryLnwIuWjnjg/C1BVhZhaw@public.gmane.org List-Id: iommu@lists.linux-foundation.org On Mon, Apr 09, 2018 at 06:59:08PM +0200, Jacopo Mondi wrote: > I'm still a bit puzzled on what happens if dma_mmap_from_dev_coherent() fails. > Does a dma_mmap_from_dev_coherent() failure guarantee anyhow that the > successive virt_to_page() isn't problematic as it is today? > Or is it the > if (off < count && user_count <= (count - off)) > check that makes the translation safe? It doesn't. I think one major issue is that we should not simply fall to dma_common_mmap if no method is required, but need every instance of dma_map_ops to explicitly opt into an mmap method that is known to work. > #ifndef CONFIG_ARCH_NO_COHERENT_DMA_MMAP > unsigned long user_count = vma_pages(vma); > unsigned long count = PAGE_ALIGN(size) >> PAGE_SHIFT; > - unsigned long pfn = page_to_pfn(virt_to_page(cpu_addr)); > unsigned long off = vma->vm_pgoff; > + unsigned long pfn; > > vma->vm_page_prot = pgprot_noncached(vma->vm_page_prot); > > @@ -235,6 +235,7 @@ int dma_common_mmap(struct device *dev, struct vm_area_struct *vma, > return ret; > > if (off < count && user_count <= (count - off)) { > + pfn = page_to_pfn(virt_to_page(cpu_addr)); > ret = remap_pfn_range(vma, vma->vm_start, > pfn + off, > user_count << PAGE_SHIFT, Why not: ret = remap_pfn_range(vma, vma->vm_start, page_to_pfn(virt_to_page(cpu_addr)) + off, and save the temp variable?