Message ID | 1298885822-10083-1-git-send-email-jslaby@suse.cz (mailing list archive) |
---|---|
State | Superseded, archived |
Headers |
Return-path: <mchehab@pedra> Envelope-to: mchehab@pedra Delivery-date: Mon, 28 Feb 2011 08:06:25 -0300 Received: from mchehab by pedra with local (Exim 4.72) (envelope-from <mchehab@pedra>) id 1Pu0vo-0001CI-Ps for mchehab@pedra; Mon, 28 Feb 2011 08:06:25 -0300 Received: from casper.infradead.org [85.118.1.10] by pedra with IMAP (fetchmail-6.3.17) for <mchehab@localhost> (single-drop); Mon, 28 Feb 2011 08:06:24 -0300 (BRT) Received: from vger.kernel.org ([209.132.180.67]) by casper.infradead.org with esmtp (Exim 4.72 #1 (Red Hat Linux)) id 1PtzXZ-0001dy-Kh; Mon, 28 Feb 2011 09:37:17 +0000 Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752872Ab1B1JhP (ORCPT <rfc822; kmpark@infradead.org> + 1 other); Mon, 28 Feb 2011 04:37:15 -0500 Received: from mail.pripojeni.net ([217.66.174.14]:52197 "EHLO smtp.pripojeni.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752384Ab1B1JhO (ORCPT <rfc822;linux-media@vger.kernel.org>); Mon, 28 Feb 2011 04:37:14 -0500 Received: from localhost.localdomain ([217.66.174.142]) by smtp.pripojeni.net (Kerio Connect 7.1.3); Mon, 28 Feb 2011 10:37:08 +0100 From: Jiri Slaby <jslaby@suse.cz> To: mchehab@infradead.org Cc: linux-media@vger.kernel.org, linux-kernel@vger.kernel.org, jirislaby@gmail.com, Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> Subject: [PATCH v2 -resend#1 1/1] V4L: videobuf, don't use dma addr as physical Date: Mon, 28 Feb 2011 10:37:02 +0100 Message-Id: <1298885822-10083-1-git-send-email-jslaby@suse.cz> X-Mailer: git-send-email 1.7.4.1 Precedence: bulk List-ID: <linux-media.vger.kernel.org> X-Mailing-List: linux-media@vger.kernel.org Sender: <mchehab@pedra> |
Commit Message
Jiri Slaby
Feb. 28, 2011, 9:37 a.m. UTC
mem->dma_handle is a dma address obtained by dma_alloc_coherent which
needn't be a physical address in presence of IOMMU. So ensure we are
remapping (remap_pfn_range) the right page in __videobuf_mmap_mapper
by using virt_to_phys(mem->vaddr) and not mem->dma_handle.
While at it, use PFN_DOWN instead of explicit shift.
Signed-off-by: Jiri Slaby <jslaby@suse.cz>
Cc: Mauro Carvalho Chehab <mchehab@infradead.org>
Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
drivers/media/video/videobuf-dma-contig.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
Comments
Hi Jiri, On Monday 28 February 2011 10:37:02 Jiri Slaby wrote: > mem->dma_handle is a dma address obtained by dma_alloc_coherent which > needn't be a physical address in presence of IOMMU. So ensure we are > remapping (remap_pfn_range) the right page in __videobuf_mmap_mapper > by using virt_to_phys(mem->vaddr) and not mem->dma_handle. Quoting arch/arm/include/asm/memory.h, /* * These are *only* valid on the kernel direct mapped RAM memory. * Note: Drivers should NOT use these. They are the wrong * translation for translating DMA addresses. Use the driver * DMA support - see dma-mapping.h. */ static inline unsigned long virt_to_phys(const volatile void *x) { return __virt_to_phys((unsigned long)(x)); } Why would you use physically contiguous memory if you have an IOMMU anyway ? > While at it, use PFN_DOWN instead of explicit shift. > > Signed-off-by: Jiri Slaby <jslaby@suse.cz> > Cc: Mauro Carvalho Chehab <mchehab@infradead.org> > Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> > --- > drivers/media/video/videobuf-dma-contig.c | 2 +- > 1 files changed, 1 insertions(+), 1 deletions(-) > > diff --git a/drivers/media/video/videobuf-dma-contig.c > b/drivers/media/video/videobuf-dma-contig.c index c969111..19d3e4a 100644 > --- a/drivers/media/video/videobuf-dma-contig.c > +++ b/drivers/media/video/videobuf-dma-contig.c > @@ -300,7 +300,7 @@ static int __videobuf_mmap_mapper(struct videobuf_queue > *q, > > vma->vm_page_prot = pgprot_noncached(vma->vm_page_prot); > retval = remap_pfn_range(vma, vma->vm_start, > - mem->dma_handle >> PAGE_SHIFT, > + PFN_DOWN(virt_to_phys(mem->vaddr)) > size, vma->vm_page_prot); > if (retval) { > dev_err(q->dev, "mmap: remap failed with error %d. ", retval);
On Mon, Feb 28, 2011 at 10:37:02AM +0100, Jiri Slaby wrote: > mem->dma_handle is a dma address obtained by dma_alloc_coherent which > needn't be a physical address in presence of IOMMU. So ensure we are Can you add a comment why you are fixing it? Is there a bug report for this? Under what conditions did you expose this fault? You also might want to mention that "needn't be a physical address as a hardware IOMMU can (and most likely) will return a bus address where physical != bus address." Otherwise you can stick 'Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>' on it. > remapping (remap_pfn_range) the right page in __videobuf_mmap_mapper > by using virt_to_phys(mem->vaddr) and not mem->dma_handle. > > While at it, use PFN_DOWN instead of explicit shift. > > Signed-off-by: Jiri Slaby <jslaby@suse.cz> > Cc: Mauro Carvalho Chehab <mchehab@infradead.org> > Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> > --- > drivers/media/video/videobuf-dma-contig.c | 2 +- > 1 files changed, 1 insertions(+), 1 deletions(-) > > diff --git a/drivers/media/video/videobuf-dma-contig.c b/drivers/media/video/videobuf-dma-contig.c > index c969111..19d3e4a 100644 > --- a/drivers/media/video/videobuf-dma-contig.c > +++ b/drivers/media/video/videobuf-dma-contig.c > @@ -300,7 +300,7 @@ static int __videobuf_mmap_mapper(struct videobuf_queue *q, > > vma->vm_page_prot = pgprot_noncached(vma->vm_page_prot); > retval = remap_pfn_range(vma, vma->vm_start, > - mem->dma_handle >> PAGE_SHIFT, > + PFN_DOWN(virt_to_phys(mem->vaddr)) > size, vma->vm_page_prot); > if (retval) { > dev_err(q->dev, "mmap: remap failed with error %d. ", retval); > -- > 1.7.4.1 > -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 02/28/2011 11:53 AM, Laurent Pinchart wrote: > On Monday 28 February 2011 10:37:02 Jiri Slaby wrote: >> mem->dma_handle is a dma address obtained by dma_alloc_coherent which >> needn't be a physical address in presence of IOMMU. So ensure we are >> remapping (remap_pfn_range) the right page in __videobuf_mmap_mapper >> by using virt_to_phys(mem->vaddr) and not mem->dma_handle. > > Quoting arch/arm/include/asm/memory.h, > > /* > * These are *only* valid on the kernel direct mapped RAM memory. Which the DMA allocation shall be. > * Note: Drivers should NOT use these. This is weird. > They are the wrong > * translation for translating DMA addresses. Use the driver > * DMA support - see dma-mapping.h. Yes, ACK, and vice versa. DMA addresses cannot be used as physical ones. > */ > static inline unsigned long virt_to_phys(const volatile void *x) > { > return __virt_to_phys((unsigned long)(x)); > } > > Why would you use physically contiguous memory if you have an IOMMU anyway ? Sorry, what? When IOMMU is used, dma_alloc_* functions may return "tags" as a DMA address, not a physical address. So using these DMA "addresses" directly (e.g. in remap_pfn_range) is a bug. Maybe there is a better way to convert a kernel address of the DMA mapping into a physical frame, but I'm not aware of any... >> While at it, use PFN_DOWN instead of explicit shift. >> >> Signed-off-by: Jiri Slaby <jslaby@suse.cz> >> Cc: Mauro Carvalho Chehab <mchehab@infradead.org> >> Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> >> --- >> drivers/media/video/videobuf-dma-contig.c | 2 +- >> 1 files changed, 1 insertions(+), 1 deletions(-) >> >> diff --git a/drivers/media/video/videobuf-dma-contig.c >> b/drivers/media/video/videobuf-dma-contig.c index c969111..19d3e4a 100644 >> --- a/drivers/media/video/videobuf-dma-contig.c >> +++ b/drivers/media/video/videobuf-dma-contig.c >> @@ -300,7 +300,7 @@ static int __videobuf_mmap_mapper(struct videobuf_queue >> *q, >> >> vma->vm_page_prot = pgprot_noncached(vma->vm_page_prot); >> retval = remap_pfn_range(vma, vma->vm_start, >> - mem->dma_handle >> PAGE_SHIFT, >> + PFN_DOWN(virt_to_phys(mem->vaddr)) >> size, vma->vm_page_prot); >> if (retval) { >> dev_err(q->dev, "mmap: remap failed with error %d. ", retval); > regards,
Hi Jiri, On Monday 28 February 2011 16:07:43 Jiri Slaby wrote: > On 02/28/2011 11:53 AM, Laurent Pinchart wrote: > > On Monday 28 February 2011 10:37:02 Jiri Slaby wrote: > >> mem->dma_handle is a dma address obtained by dma_alloc_coherent which > >> needn't be a physical address in presence of IOMMU. So ensure we are > >> remapping (remap_pfn_range) the right page in __videobuf_mmap_mapper > >> by using virt_to_phys(mem->vaddr) and not mem->dma_handle. > > > > Quoting arch/arm/include/asm/memory.h, > > > > /* > > > > * These are *only* valid on the kernel direct mapped RAM memory. > > Which the DMA allocation shall be. > > > * Note: Drivers should NOT use these. > > This is weird. > > > They are the wrong > > > > * translation for translating DMA addresses. Use the driver > > * DMA support - see dma-mapping.h. > > Yes, ACK, and vice versa. DMA addresses cannot be used as physical ones. > > > */ > > > > static inline unsigned long virt_to_phys(const volatile void *x) > > { > > > > return __virt_to_phys((unsigned long)(x)); > > > > } > > > > Why would you use physically contiguous memory if you have an IOMMU > > anyway ? > > Sorry, what? When IOMMU is used, dma_alloc_* functions may return "tags" > as a DMA address, not a physical address. So using these DMA "addresses" > directly (e.g. in remap_pfn_range) is a bug. What I mean is that videobuf-dma-contig is meant to be used by drivers that require physically contiguous memory. If the system has an IOMMU, why would drivers need that ? > Maybe there is a better way to convert a kernel address of the DMA > mapping into a physical frame, but I'm not aware of any... > > >> While at it, use PFN_DOWN instead of explicit shift. > >> > >> Signed-off-by: Jiri Slaby <jslaby@suse.cz> > >> Cc: Mauro Carvalho Chehab <mchehab@infradead.org> > >> Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> > >> --- > >> > >> drivers/media/video/videobuf-dma-contig.c | 2 +- > >> 1 files changed, 1 insertions(+), 1 deletions(-) > >> > >> diff --git a/drivers/media/video/videobuf-dma-contig.c > >> b/drivers/media/video/videobuf-dma-contig.c index c969111..19d3e4a > >> 100644 --- a/drivers/media/video/videobuf-dma-contig.c > >> +++ b/drivers/media/video/videobuf-dma-contig.c > >> @@ -300,7 +300,7 @@ static int __videobuf_mmap_mapper(struct > >> videobuf_queue *q, > >> > >> vma->vm_page_prot = pgprot_noncached(vma->vm_page_prot); > >> retval = remap_pfn_range(vma, vma->vm_start, > >> > >> - mem->dma_handle >> PAGE_SHIFT, > >> + PFN_DOWN(virt_to_phys(mem->vaddr)) > >> > >> size, vma->vm_page_prot); > >> > >> if (retval) { > >> > >> dev_err(q->dev, "mmap: remap failed with error %d. ", retval);
On 02/28/2011 04:14 PM, Laurent Pinchart wrote: > Hi Jiri, > > On Monday 28 February 2011 16:07:43 Jiri Slaby wrote: >> On 02/28/2011 11:53 AM, Laurent Pinchart wrote: >>> On Monday 28 February 2011 10:37:02 Jiri Slaby wrote: >>>> mem->dma_handle is a dma address obtained by dma_alloc_coherent which >>>> needn't be a physical address in presence of IOMMU. So ensure we are >>>> remapping (remap_pfn_range) the right page in __videobuf_mmap_mapper >>>> by using virt_to_phys(mem->vaddr) and not mem->dma_handle. >>> >>> Quoting arch/arm/include/asm/memory.h, >>> >>> /* >>> >>> * These are *only* valid on the kernel direct mapped RAM memory. >> >> Which the DMA allocation shall be. >> >>> * Note: Drivers should NOT use these. >> >> This is weird. >> >>> They are the wrong >>> >>> * translation for translating DMA addresses. Use the driver >>> * DMA support - see dma-mapping.h. >> >> Yes, ACK, and vice versa. DMA addresses cannot be used as physical ones. >> >>> */ >>> >>> static inline unsigned long virt_to_phys(const volatile void *x) >>> { >>> >>> return __virt_to_phys((unsigned long)(x)); >>> >>> } >>> >>> Why would you use physically contiguous memory if you have an IOMMU >>> anyway ? >> >> Sorry, what? When IOMMU is used, dma_alloc_* functions may return "tags" >> as a DMA address, not a physical address. So using these DMA "addresses" >> directly (e.g. in remap_pfn_range) is a bug. > > What I mean is that videobuf-dma-contig is meant to be used by drivers that > require physically contiguous memory. If the system has an IOMMU, why would > drivers need that ? Aha. They actually need not but they would need do the mapping themselves which they currently do not. IOW the vbuf-dma-contig allocator is used unconditionally in the few drivers I checked. BUT Even if they need only one page and use vbuf-dma-contig, which I don't see a reason not to, it will cause problems too. regards,
On 02/28/2011 03:53 PM, Konrad Rzeszutek Wilk wrote: > On Mon, Feb 28, 2011 at 10:37:02AM +0100, Jiri Slaby wrote: >> mem->dma_handle is a dma address obtained by dma_alloc_coherent which >> needn't be a physical address in presence of IOMMU. So ensure we are > > Can you add a comment why you are fixing it? Is there a bug report for this? > Under what conditions did you expose this fault? No, by a just peer review when I was looking for something completely different. > You also might want to mention that "needn't be a physical address as > a hardware IOMMU can (and most likely) will return a bus address where > physical != bus address." Mauro, do you want me to resend this with such an udpate in the changelog? > Otherwise you can stick 'Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>' > on it. thanks,
Em 28-02-2011 12:47, Jiri Slaby escreveu: > On 02/28/2011 03:53 PM, Konrad Rzeszutek Wilk wrote: >> On Mon, Feb 28, 2011 at 10:37:02AM +0100, Jiri Slaby wrote: >>> mem->dma_handle is a dma address obtained by dma_alloc_coherent which >>> needn't be a physical address in presence of IOMMU. So ensure we are >> >> Can you add a comment why you are fixing it? Is there a bug report for this? >> Under what conditions did you expose this fault? > > No, by a just peer review when I was looking for something completely > different. > >> You also might want to mention that "needn't be a physical address as >> a hardware IOMMU can (and most likely) will return a bus address where >> physical != bus address." > > Mauro, do you want me to resend this with such an udpate in the changelog? Having it properly documented is always a good idea, especially since a similar fix might be needed on other drivers that also need contiguous memory. While it currently is used only on devices embedded on hardware with no iommu, there are some x86 hardware that doesn't allow DMA scatter/gather. Btw, it may be worth to take a look at vb2 dma contig module, as it might have similar issues. >> Otherwise you can stick 'Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>' >> on it. Cheers, Mauro -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/media/video/videobuf-dma-contig.c b/drivers/media/video/videobuf-dma-contig.c index c969111..19d3e4a 100644 --- a/drivers/media/video/videobuf-dma-contig.c +++ b/drivers/media/video/videobuf-dma-contig.c @@ -300,7 +300,7 @@ static int __videobuf_mmap_mapper(struct videobuf_queue *q, vma->vm_page_prot = pgprot_noncached(vma->vm_page_prot); retval = remap_pfn_range(vma, vma->vm_start, - mem->dma_handle >> PAGE_SHIFT, + PFN_DOWN(virt_to_phys(mem->vaddr)) size, vma->vm_page_prot); if (retval) { dev_err(q->dev, "mmap: remap failed with error %d. ", retval);