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 +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