[PATCH] base: dma-mapping: Postpone cpu addr translation on mmap()

[PATCH] base: dma-mapping: Postpone cpu addr translation on mmap()

[Jacopo Mondi]

Postpone calling virt_to_page() translation on memory locations not
guaranteed to be backed by a struct page.

This patch fixes a specific issue of SH architecture configured with
SPARSEMEM memory model, when mapping buffers allocated with the memblock
APIs at system initialization time, and thus not backed by the page
infrastructure.

It does apply to the general case though, as an early translation is anyhow
incorrect and shall be postponed after trying to map memory from the device
coherent memory pool first.

Suggested-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>

---
Compared to the RFC version I have tried to generalize the commit message,
please suggest any improvement to that.

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?

Thanks
   j

---
 drivers/base/dma-mapping.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/base/dma-mapping.c b/drivers/base/dma-mapping.c
index 3b11835..8b4ec34 100644
--- a/drivers/base/dma-mapping.c
+++ b/drivers/base/dma-mapping.c
@@ -226,8 +226,8 @@ int dma_common_mmap(struct device *dev, struct vm_area_struct *vma,
 #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,
--
2.7.4

[PATCH] base: dma-mapping: Postpone cpu addr translation on mmap()

[Jacopo Mondi]

Postpone calling virt_to_page() translation on memory locations not
guaranteed to be backed by a struct page.

This patch fixes a specific issue of SH architecture configured with
SPARSEMEM memory model, when mapping buffers allocated with the memblock
APIs at system initialization time, and thus not backed by the page
infrastructure.

It does apply to the general case though, as an early translation is anyhow
incorrect and shall be postponed after trying to map memory from the device
coherent memory pool first.

Suggested-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>

---
Compared to the RFC version I have tried to generalize the commit message,
please suggest any improvement to that.

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?

Thanks
   j

---
 drivers/base/dma-mapping.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/base/dma-mapping.c b/drivers/base/dma-mapping.c
index 3b11835..8b4ec34 100644
--- a/drivers/base/dma-mapping.c
+++ b/drivers/base/dma-mapping.c
@@ -226,8 +226,8 @@ int dma_common_mmap(struct device *dev, struct vm_area_struct *vma,
 #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,
--
2.7.4

Re: [PATCH] base: dma-mapping: Postpone cpu addr translation on mmap()

[Christoph Hellwig]

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?

Re: [PATCH] base: dma-mapping: Postpone cpu addr translation on mmap()

[Christoph Hellwig]

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?

Re: [PATCH] base: dma-mapping: Postpone cpu addr translation on mmap()

[Christoph Hellwig]

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?

Re: [PATCH] base: dma-mapping: Postpone cpu addr translation on mmap()

[jacopo mondi]

[-- Attachment #1: Type: text/plain, Size: 1835 bytes --]

Hi Christoph,

On Mon, Apr 09, 2018 at 10:52:51AM -0700, Christoph Hellwig wrote:
> 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.

I see.. this patch thus just postpones the problem...

>
> >  #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?

Sure, it's better... Should I send a v2 or considering your above
comment this patch is just a mitigation and should be ditched in
favour of a proper solution (which requires a much more considerable amount
of work though)?

Thanks
   j

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH] base: dma-mapping: Postpone cpu addr translation on mmap()

[jacopo mondi]

[-- Attachment #1: Type: text/plain, Size: 1835 bytes --]

Hi Christoph,

On Mon, Apr 09, 2018 at 10:52:51AM -0700, Christoph Hellwig wrote:
> 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.

I see.. this patch thus just postpones the problem...

>
> >  #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?

Sure, it's better... Should I send a v2 or considering your above
comment this patch is just a mitigation and should be ditched in
favour of a proper solution (which requires a much more considerable amount
of work though)?

Thanks
   j

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH] base: dma-mapping: Postpone cpu addr translation on mmap()

[jacopo mondi]

[-- Attachment #1: Type: text/plain, Size: 2091 bytes --]

Hello again,

On Tue, Apr 10, 2018 at 09:57:52AM +0200, jacopo mondi wrote:
> Hi Christoph,
>
> On Mon, Apr 09, 2018 at 10:52:51AM -0700, Christoph Hellwig wrote:
> > 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.
>
> I see.. this patch thus just postpones the problem...
>
> >
> > >  #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?
>
> Sure, it's better... Should I send a v2 or considering your above
> comment this patch is just a mitigation and should be ditched in
> favour of a proper solution (which requires a much more considerable amount
> of work though)?

Don't want to be insistent, but I didn't get from your reply if a v2
is welcome or not :)

Thanks
  j

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH] base: dma-mapping: Postpone cpu addr translation on mmap()

[jacopo mondi]

[-- Attachment #1: Type: text/plain, Size: 2091 bytes --]

Hello again,

On Tue, Apr 10, 2018 at 09:57:52AM +0200, jacopo mondi wrote:
> Hi Christoph,
>
> On Mon, Apr 09, 2018 at 10:52:51AM -0700, Christoph Hellwig wrote:
> > 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.
>
> I see.. this patch thus just postpones the problem...
>
> >
> > >  #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?
>
> Sure, it's better... Should I send a v2 or considering your above
> comment this patch is just a mitigation and should be ditched in
> favour of a proper solution (which requires a much more considerable amount
> of work though)?

Don't want to be insistent, but I didn't get from your reply if a v2
is welcome or not :)

Thanks
  j

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH] base: dma-mapping: Postpone cpu addr translation on mmap()

[Christoph Hellwig]

Please send a v2.

Re: [PATCH] base: dma-mapping: Postpone cpu addr translation on mmap()

[Christoph Hellwig]

Please send a v2.

Re: [PATCH] base: dma-mapping: Postpone cpu addr translation on mmap()

[Christoph Hellwig]

Please send a v2.