Em 28-02-2011 15:20, Mauro Carvalho Chehab escreveu:

Em 28-02-2011 12:47, Jiri Slaby escreveu:

quoted

On 02/28/2011 03:53 PM, Konrad Rzeszutek Wilk wrote:

quoted

On Mon, Feb 28, 2011 at 10:37:02AM +0100, Jiri Slaby wrote:

quoted

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.

quoted

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.

quoted

quoted

Otherwise you can stick 'Reviewed-by: Konrad Rzeszutek Wilk [off-list ref]'
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

As I got no return, and the patch looked sane, I've reviewed the comment myself,
in order to make it more explicit, as suggested by Konrad, and added his
reviewed-by: tag:

Author: Jiri Slaby [off-list ref]
Date:   Mon Feb 28 06:37:02 2011 -0300

    [media] V4L: videobuf, don't use dma addr as physical
    
    mem->dma_handle is a dma address obtained by dma_alloc_coherent which
    needn't be a physical address in presence of IOMMU, as
    a hardware IOMMU can (and most likely) will return a bus address where
    physical != bus address.
    
    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 [off-list ref]
    Reviewed-by: Konrad Rzeszutek Wilk [off-list ref]
    Signed-off-by: Mauro Carvalho Chehab [off-list ref]

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);