diff mbox

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

Message ID 1510679286-6988-3-git-send-email-jacopo+renesas@jmondi.org (mailing list archive)
State Under Review
Delegated to: Geert Uytterhoeven
Headers show

Commit Message

Jacopo Mondi Nov. 14, 2017, 5:08 p.m. UTC 
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.

Suggested-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
---
 drivers/base/dma-mapping.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

--
2.7.4

Comments

Robin Murphy Nov. 15, 2017, 1:38 p.m. UTC | #1 
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 <laurent.pinchart@ideasonboard.com>
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> ---
>   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
> iommu@lists.linux-foundation.org
> https://lists.linuxfoundation.org/mailman/listinfo/iommu
>
Jacopo Mondi April 9, 2018, 7:25 a.m. UTC | #2 
Hi Robin, Laurent,
    a long time passed, sorry about this.

On Wed, Nov 15, 2017 at 01:38:23PM +0000, 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 <laurent.pinchart@ideasonboard.com>
> >Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> >---
> >  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
> >iommu@lists.linux-foundation.org
> >https://lists.linuxfoundation.org/mailman/listinfo/iommu
> >
Robin Murphy April 9, 2018, 11:11 a.m. UTC | #3 
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 +0000, 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 <laurent.pinchart@ideasonboard.com>
>>> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
>>> ---
>>>   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
>>> iommu@lists.linux-foundation.org
>>> https://lists.linuxfoundation.org/mailman/listinfo/iommu
>>>
Laurent Pinchart April 9, 2018, 1:06 p.m. UTC | #4 
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 +0000, 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 <laurent.pinchart@ideasonboard.com>
> >>> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> >>> ---
> >>> 
> >>>   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,
Rich Felker April 9, 2018, 3:11 p.m. UTC | #5 
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 +0000, 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
Laurent Pinchart April 9, 2018, 3:18 p.m. UTC | #6 
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 +0000, 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 :-)
diff mbox

Patch

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,