[PATCH v2 -resend#1 1/1] V4L: videobuf, don't use dma addr as physical

[PATCH v2 -resend#1 1/1] V4L: videobuf, don't use dma addr as physical

[Jiri Slaby]

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

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

Re: [PATCH v2 -resend#1 1/1] V4L: videobuf, don't use dma addr as physical

[Laurent Pinchart]

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

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 -resend#1 1/1] V4L: videobuf, don't use dma addr as physical

[Jiri Slaby]

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,
-- 
js
suse labs

Re: [PATCH v2 -resend#1 1/1] V4L: videobuf, don't use dma addr as physical

[Laurent Pinchart]

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

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 -resend#1 1/1] V4L: videobuf, don't use dma addr as physical

[Jiri Slaby]

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,
-- 
js
suse labs

Re: [PATCH v2 -resend#1 1/1] V4L: videobuf, don't use dma addr as physical

[Konrad Rzeszutek Wilk]

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
> 

Re: [PATCH v2 -resend#1 1/1] V4L: videobuf, don't use dma addr as physical

[Jiri Slaby]

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,
-- 
js
suse labs

Re: [PATCH v2 -resend#1 1/1] V4L: videobuf, don't use dma addr as physical

[Mauro Carvalho Chehab]

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

Re: [PATCH v2 -resend#1 1/1] V4L: videobuf, don't use dma addr as physical

[Mauro Carvalho Chehab]

Em 28-02-2011 15:20, Mauro Carvalho Chehab escreveu:
> 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

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 <jslaby@suse.cz>
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 <jslaby@suse.cz>
    Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
    Signed-off-by: Mauro Carvalho Chehab <mchehab@redhat.com>

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

Re: [PATCH v2 -resend#1 1/1] V4L: videobuf, don't use dma addr as physical

[Jiri Slaby]

On 03/21/2011 11:43 PM, Mauro Carvalho Chehab wrote:
> As I got no return, and the patch looked sane, I've reviewed the comment myself,

Aha, I forgot to send it. Sorry.

It looks OK.

> Author: Jiri Slaby <jslaby@suse.cz>
> 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 <jslaby@suse.cz>
>     Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
>     Signed-off-by: Mauro Carvalho Chehab <mchehab@redhat.com>
> 
> 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);

thanks,
-- 
js
suse labs