Re: [RFC v2 2/2] base: dma-mapping: Postpone page_to_pfn() on mmap()

[Laurent Pinchart]

Hi Rich,

On Monday, 9 April 2018 18:11:13 EEST Rich Felker wrote:
> On Mon, Apr 09, 2018 at 04:06:15PM +0300, Laurent Pinchart wrote:
> > On Monday, 9 April 2018 14:11:22 EEST Robin Murphy wrote:
> >> On 09/04/18 08:25, jacopo mondi wrote:
> >>> Hi Robin, Laurent,
> >>> 
> >>>  a long time passed, sorry about this.
> >>> 
> >>> On Wed, Nov 15, 2017 at 01:38:23PM +, Robin Murphy wrote:
>  On 14/11/17 17:08, Jacopo Mondi wrote:
> > On SH4 architecture, with SPARSEMEM memory model, translating page
> > to pfn hangs the CPU. Post-pone translation to pfn after
> > dma_mmap_from_dev_coherent() function call as it succeeds and make
> > page translation not necessary.
> > 
> > This patch was suggested by Laurent Pinchart and he's working to
> > submit a proper fix mainline. Not sending for inclusion at the
> > moment.
>  
>  Y'know, I think this patch does have some merit by itself - until we
>  know that cpu_addr *doesn't* represent some device-private memory
>  which is not guaranteed to be backed by a struct page, calling
>  virt_to_page() on it is arguably semantically incorrect, even if it
>  might happen to be benign in most cases.
> >>> 
> >>> I still need to carry this patch in my trees to have a working dma
> >>> memory mapping on SH4 platforms. My understanding from your comment is
> >>> that there may be a way forward for this patch, do you still think the
> >>> same? Have you got any suggestion on how to improve this eventually
> >>> for inclusion?
> >> 
> >> As before, the change itself does seem reasonable; it might be worth
> >> rewording the commit message in more general terms rather than making it
> >> sound like an SH-specific workaround (which I really don't think it is),
> >> but otherwise I'd say just repost it as a non-RFC patch.
> > 
> > I actually can't remember any better fix I would have in mind, so this
> > looks good to me :-) I agree with Robin, the commit message should be
> > reworded. Robin's explanation of why virt_to_page() should be postponed
> > until we know that cpu_addr represents memory that is guaranteed to be
> > backed by a struct page is a good starting point. You can mention SH4 as
> > an example of an architecture that will crash when calling virt_to_page()
> > in such a case, but the fix isn't specific to SH4.
> 
> Just checking since I joined late -- is the consensus that this does
> not require any action/review specific to SH (in my role as maintainer
> way behind on maintenance)?

I don't think it requires any action specific to SH. If you want to review the 
patch in the context of SH though, please feel free to do so :-)

-- 
Regards,

Laurent Pinchart

Re: [RFC v2 2/2] base: dma-mapping: Postpone page_to_pfn() on mmap()

[Rich Felker]

On Mon, Apr 09, 2018 at 04:06:15PM +0300, Laurent Pinchart wrote:
> Hello,
> 
> On Monday, 9 April 2018 14:11:22 EEST Robin Murphy wrote:
> > On 09/04/18 08:25, jacopo mondi wrote:
> > > Hi Robin, Laurent,
> > > 
> > >  a long time passed, sorry about this.
> > > 
> > > On Wed, Nov 15, 2017 at 01:38:23PM +, Robin Murphy wrote:
> > >> On 14/11/17 17:08, Jacopo Mondi wrote:
> > >>> On SH4 architecture, with SPARSEMEM memory model, translating page to
> > >>> pfn hangs the CPU. Post-pone translation to pfn after
> > >>> dma_mmap_from_dev_coherent() function call as it succeeds and make page
> > >>> translation not necessary.
> > >>> 
> > >>> This patch was suggested by Laurent Pinchart and he's working to submit
> > >>> a proper fix mainline. Not sending for inclusion at the moment.
> > >> 
> > >> Y'know, I think this patch does have some merit by itself - until we know
> > >> that cpu_addr *doesn't* represent some device-private memory which is not
> > >> guaranteed to be backed by a struct page, calling virt_to_page() on it is
> > >> arguably semantically incorrect, even if it might happen to be benign in
> > >> most cases.
> > > 
> > > I still need to carry this patch in my trees to have a working dma memory
> > > mapping on SH4 platforms. My understanding from your comment is that
> > > there may be a way forward for this patch, do you still think the same?
> > > Have you got any suggestion on how to improve this eventually for
> > > inclusion?
> > 
> > As before, the change itself does seem reasonable; it might be worth
> > rewording the commit message in more general terms rather than making it
> > sound like an SH-specific workaround (which I really don't think it is),
> > but otherwise I'd say just repost it as a non-RFC patch.
> 
> I actually can't remember any better fix I would have in mind, so this looks 
> good to me :-) I agree with Robin, the commit message should be reworded. 
> Robin's explanation of why virt_to_page() should be postponed until we know 
> that cpu_addr represents memory that is guaranteed to be backed by a struct 
> page is a good starting point. You can mention SH4 as an example of an 
> architecture that will crash when calling virt_to_page() in such a case, but 
> the fix isn't specific to SH4.

Just checking since I joined late -- is the consensus that this does
not require any action/review specific to SH (in my role as maintainer
way behind on maintenance)?

Rich

Re: [RFC v2 2/2] base: dma-mapping: Postpone page_to_pfn() on mmap()

[Laurent Pinchart]

Hello,

On Monday, 9 April 2018 14:11:22 EEST Robin Murphy wrote:
> On 09/04/18 08:25, jacopo mondi wrote:
> > Hi Robin, Laurent,
> > 
> >  a long time passed, sorry about this.
> > 
> > On Wed, Nov 15, 2017 at 01:38:23PM +, Robin Murphy wrote:
> >> On 14/11/17 17:08, Jacopo Mondi wrote:
> >>> On SH4 architecture, with SPARSEMEM memory model, translating page to
> >>> pfn hangs the CPU. Post-pone translation to pfn after
> >>> dma_mmap_from_dev_coherent() function call as it succeeds and make page
> >>> translation not necessary.
> >>> 
> >>> This patch was suggested by Laurent Pinchart and he's working to submit
> >>> a proper fix mainline. Not sending for inclusion at the moment.
> >> 
> >> Y'know, I think this patch does have some merit by itself - until we know
> >> that cpu_addr *doesn't* represent some device-private memory which is not
> >> guaranteed to be backed by a struct page, calling virt_to_page() on it is
> >> arguably semantically incorrect, even if it might happen to be benign in
> >> most cases.
> > 
> > I still need to carry this patch in my trees to have a working dma memory
> > mapping on SH4 platforms. My understanding from your comment is that
> > there may be a way forward for this patch, do you still think the same?
> > Have you got any suggestion on how to improve this eventually for
> > inclusion?
> 
> As before, the change itself does seem reasonable; it might be worth
> rewording the commit message in more general terms rather than making it
> sound like an SH-specific workaround (which I really don't think it is),
> but otherwise I'd say just repost it as a non-RFC patch.

I actually can't remember any better fix I would have in mind, so this looks 
good to me :-) I agree with Robin, the commit message should be reworded. 
Robin's explanation of why virt_to_page() should be postponed until we know 
that cpu_addr represents memory that is guaranteed to be backed by a struct 
page is a good starting point. You can mention SH4 as an example of an 
architecture that will crash when calling virt_to_page() in such a case, but 
the fix isn't specific to SH4.

> >>> Suggested-by: Laurent Pinchart 
> >>> Signed-off-by: Jacopo Mondi 
> >>> ---
> >>> 
> >>>   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 e584edd..73d64d3 100644
> >>> --- a/drivers/base/dma-mapping.c
> >>> +++ b/drivers/base/dma-mapping.c
> >>> @@ -227,8 +227,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);
> >>> 
> >>> @@ -236,6 +236,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,

-- 
Regards,

Laurent Pinchart

Re: [RFC v2 2/2] base: dma-mapping: Postpone page_to_pfn() on mmap()

[Robin Murphy]

Hi Jacopo,

On 09/04/18 08:25, jacopo mondi wrote:

Hi Robin, Laurent,
 a long time passed, sorry about this.

On Wed, Nov 15, 2017 at 01:38:23PM +, Robin Murphy wrote:

On 14/11/17 17:08, Jacopo Mondi wrote:

On SH4 architecture, with SPARSEMEM memory model, translating page to
pfn hangs the CPU. Post-pone translation to pfn after
dma_mmap_from_dev_coherent() function call as it succeeds and make page
translation not necessary.

This patch was suggested by Laurent Pinchart and he's working to submit
a proper fix mainline. Not sending for inclusion at the moment.


Y'know, I think this patch does have some merit by itself - until we know
that cpu_addr *doesn't* represent some device-private memory which is not
guaranteed to be backed by a struct page, calling virt_to_page() on it is
arguably semantically incorrect, even if it might happen to be benign in
most cases.


I still need to carry this patch in my trees to have a working dma memory 
mapping
on SH4 platforms. My understanding from your comment is that there may
be a way forward for this patch, do you still think the same? Have you
got any suggestion on how to improve this eventually for inclusion?


As before, the change itself does seem reasonable; it might be worth 
rewording the commit message in more general terms rather than making it 
sound like an SH-specific workaround (which I really don't think it is), 
but otherwise I'd say just repost it as a non-RFC patch.


Robin.



Thanks
j



Robin.


Suggested-by: Laurent Pinchart 
Signed-off-by: Jacopo Mondi 
---
  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 e584edd..73d64d3 100644
--- a/drivers/base/dma-mapping.c
+++ b/drivers/base/dma-mapping.c
@@ -227,8 +227,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);

@@ -236,6 +236,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

___
iommu mailing list
io...@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [RFC v2 2/2] base: dma-mapping: Postpone page_to_pfn() on mmap()

[jacopo mondi]

Hi Robin, Laurent,
a long time passed, sorry about this.

On Wed, Nov 15, 2017 at 01:38:23PM +, Robin Murphy wrote:
> On 14/11/17 17:08, Jacopo Mondi wrote:
> >On SH4 architecture, with SPARSEMEM memory model, translating page to
> >pfn hangs the CPU. Post-pone translation to pfn after
> >dma_mmap_from_dev_coherent() function call as it succeeds and make page
> >translation not necessary.
> >
> >This patch was suggested by Laurent Pinchart and he's working to submit
> >a proper fix mainline. Not sending for inclusion at the moment.
>
> Y'know, I think this patch does have some merit by itself - until we know
> that cpu_addr *doesn't* represent some device-private memory which is not
> guaranteed to be backed by a struct page, calling virt_to_page() on it is
> arguably semantically incorrect, even if it might happen to be benign in
> most cases.

I still need to carry this patch in my trees to have a working dma memory 
mapping
on SH4 platforms. My understanding from your comment is that there may
be a way forward for this patch, do you still think the same? Have you
got any suggestion on how to improve this eventually for inclusion?

Thanks
   j

>
> Robin.
>
> >Suggested-by: Laurent Pinchart 
> >Signed-off-by: Jacopo Mondi 
> >---
> >  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 e584edd..73d64d3 100644
> >--- a/drivers/base/dma-mapping.c
> >+++ b/drivers/base/dma-mapping.c
> >@@ -227,8 +227,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);
> >
> >@@ -236,6 +236,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
> >
> >___
> >iommu mailing list
> >io...@lists.linux-foundation.org
> >https://lists.linuxfoundation.org/mailman/listinfo/iommu
> >


signature.asc
Description: PGP signature

Re: [RFC v2 2/2] base: dma-mapping: Postpone page_to_pfn() on mmap()

[Robin Murphy]

On 14/11/17 17:08, Jacopo Mondi wrote:

On SH4 architecture, with SPARSEMEM memory model, translating page to
pfn hangs the CPU. Post-pone translation to pfn after
dma_mmap_from_dev_coherent() function call as it succeeds and make page
translation not necessary.

This patch was suggested by Laurent Pinchart and he's working to submit
a proper fix mainline. Not sending for inclusion at the moment.


Y'know, I think this patch does have some merit by itself - until we 
know that cpu_addr *doesn't* represent some device-private memory which 
is not guaranteed to be backed by a struct page, calling virt_to_page() 
on it is arguably semantically incorrect, even if it might happen to be 
benign in most cases.


Robin.


Suggested-by: Laurent Pinchart 
Signed-off-by: Jacopo Mondi 
---
  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 e584edd..73d64d3 100644
--- a/drivers/base/dma-mapping.c
+++ b/drivers/base/dma-mapping.c
@@ -227,8 +227,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);

@@ -236,6 +236,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

___
iommu mailing list
io...@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu