[v2,-resend#1,1/1] 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. 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(-)
  

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