* [PATCH] auxdisplay/cfag12864bfb.c: Replace vm_insert_page @ 2018-09-20 20:23 Souptick Joarder 2018-09-20 21:26 ` Miguel Ojeda 2018-09-22 12:21 ` Matthew Wilcox 0 siblings, 2 replies; 5+ messages in thread From: Souptick Joarder @ 2018-09-20 20:23 UTC (permalink / raw) To: willy, miguel.ojeda.sandonis; +Cc: linux-kernel, akpm There is a plan to remove vm_insert_page permanently and replace it with new API vmf_insert_page which will return vm_fault_t type. As part of it vm_insert_page is removed from this driver. remap_pfn_range() will be used to map kernel memory to user vma. Signed-off-by: Souptick Joarder <jrdr.linux@gmail.com> --- drivers/auxdisplay/cfag12864bfb.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/drivers/auxdisplay/cfag12864bfb.c b/drivers/auxdisplay/cfag12864bfb.c index 40c8a55..3b4411d 100644 --- a/drivers/auxdisplay/cfag12864bfb.c +++ b/drivers/auxdisplay/cfag12864bfb.c @@ -52,8 +52,12 @@ static int cfag12864bfb_mmap(struct fb_info *info, struct vm_area_struct *vma) { - return vm_insert_page(vma, vma->vm_start, - virt_to_page(cfag12864b_buffer)); + struct page *page; + unsigned long size = vma->vm_end - vma->vm_start; + + page = virt_to_page(cfag12864b_buffer); + return remap_pfn_range(vma, vma->vm_start, page_to_pfn(page), + size, vma->vm_page_prot); } static struct fb_ops cfag12864bfb_ops = { -- 1.9.1 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] auxdisplay/cfag12864bfb.c: Replace vm_insert_page 2018-09-20 20:23 [PATCH] auxdisplay/cfag12864bfb.c: Replace vm_insert_page Souptick Joarder @ 2018-09-20 21:26 ` Miguel Ojeda 2018-09-21 11:07 ` Souptick Joarder 2018-09-22 12:21 ` Matthew Wilcox 1 sibling, 1 reply; 5+ messages in thread From: Miguel Ojeda @ 2018-09-20 21:26 UTC (permalink / raw) To: Souptick Joarder; +Cc: willy, linux-kernel, Andrew Morton On Thu, Sep 20, 2018 at 10:23 PM, Souptick Joarder <jrdr.linux@gmail.com> wrote: > There is a plan to remove vm_insert_page permanently > and replace it with new API vmf_insert_page which will > return vm_fault_t type. As part of it vm_insert_page > is removed from this driver. A link to the discussion/plan would be nice. The commit 1c8f422059ae5 ("mm: change return type to vm_fault_t") explains a bit, but has a broken link :( Googling for the stuff returns many of the patches, but not the actual discussion... > static int cfag12864bfb_mmap(struct fb_info *info, struct vm_area_struct *vma) > { > - return vm_insert_page(vma, vma->vm_start, > - virt_to_page(cfag12864b_buffer)); > + struct page *page; > + unsigned long size = vma->vm_end - vma->vm_start; > + > + page = virt_to_page(cfag12864b_buffer); > + return remap_pfn_range(vma, vma->vm_start, page_to_pfn(page), > + size, vma->vm_page_prot); I am out of the loop on these mm changes, so please indulge me, but: * Why is there no documentation on vmf_insert_page() while vm_insert_page() had it? (specially since it seems you want to remove vm_insert_page()). * Shouldn't we have a simple remap_page() or remap_kernel_page() to fit this use case and avoid that dance? (another driver in auxdisplay will require the same change, and I guess others in the kernel as well). Thanks! Cheers, Miguel ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] auxdisplay/cfag12864bfb.c: Replace vm_insert_page 2018-09-20 21:26 ` Miguel Ojeda @ 2018-09-21 11:07 ` Souptick Joarder 2018-09-22 13:39 ` Miguel Ojeda 0 siblings, 1 reply; 5+ messages in thread From: Souptick Joarder @ 2018-09-21 11:07 UTC (permalink / raw) To: Miguel Ojeda Cc: Matthew Wilcox, linux-kernel, Andrew Morton, Michal Hocko, Linux-MM On Fri, Sep 21, 2018 at 2:56 AM Miguel Ojeda <miguel.ojeda.sandonis@gmail.com> wrote: > > On Thu, Sep 20, 2018 at 10:23 PM, Souptick Joarder <jrdr.linux@gmail.com> wrote: > > There is a plan to remove vm_insert_page permanently > > and replace it with new API vmf_insert_page which will > > return vm_fault_t type. As part of it vm_insert_page > > is removed from this driver. > > A link to the discussion/plan would be nice. The commit 1c8f422059ae5 > ("mm: change return type to vm_fault_t") explains a bit, but has a > broken link :( Googling for the stuff returns many of the patches, but > not the actual discussion... This might be helpful. https://marc.info/?l=linux-mm&m=152054772413234&w=4 > > > static int cfag12864bfb_mmap(struct fb_info *info, struct vm_area_struct *vma) > > { > > - return vm_insert_page(vma, vma->vm_start, > > - virt_to_page(cfag12864b_buffer)); > > + struct page *page; > > + unsigned long size = vma->vm_end - vma->vm_start; > > + > > + page = virt_to_page(cfag12864b_buffer); > > + return remap_pfn_range(vma, vma->vm_start, page_to_pfn(page), > > + size, vma->vm_page_prot); > > I am out of the loop on these mm changes, so please indulge me, but: > > * Why is there no documentation on vmf_insert_page() while > vm_insert_page() had it? (specially since it seems you want to remove > vm_insert_page()). The plan is to convert vm_insert_{page,pfn,mixed} to vmf_insert_{page,pfn,mixed}. As a good intermediate steps inline wrapper vmf_insert_{pfn,page,mixed} are introduced. After all the drivers converted, we will convert vm_insert_page to vmf_insert_page and remove the inline wrapper and update the document at the same time. > > * Shouldn't we have a simple remap_page() or remap_kernel_page() to > fit this use case and avoid that dance? (another driver in auxdisplay > will require the same change, and I guess others in the kernel as > well). There are few drivers similar like auxdisplay where straight forward conversion from vm_insert_page to vmf_insert_page is not possible. So I mapped the kernel memory to user vma using remap_pfn_range and remove vm_insert_page in this driver. Other way, is to replace vm_insert_page with vmf_insert_page() and then convert VM_FAULT_CODE back to errno. But as part of vm_fault_t migration we have already removed/cleanup most the errno to VM_FAULT_CODE mapping from drivers. So I prefer not to take this option. Third, we can introduce a similar API like vm_insert_page say, vm_insert_kmem_page() and use it for same scenarios like this. If there is a better way to do this, we can discuss it. ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] auxdisplay/cfag12864bfb.c: Replace vm_insert_page 2018-09-21 11:07 ` Souptick Joarder @ 2018-09-22 13:39 ` Miguel Ojeda 0 siblings, 0 replies; 5+ messages in thread From: Miguel Ojeda @ 2018-09-22 13:39 UTC (permalink / raw) To: Souptick Joarder Cc: Matthew Wilcox, linux-kernel, Andrew Morton, Michal Hocko, Linux-MM On Fri, Sep 21, 2018 at 1:07 PM, Souptick Joarder <jrdr.linux@gmail.com> wrote: > On Fri, Sep 21, 2018 at 2:56 AM Miguel Ojeda > <miguel.ojeda.sandonis@gmail.com> wrote: >> >> A link to the discussion/plan would be nice. The commit 1c8f422059ae5 >> ("mm: change return type to vm_fault_t") explains a bit, but has a >> broken link :( Googling for the stuff returns many of the patches, but >> not the actual discussion... > > This might be helpful. > https://marc.info/?l=linux-mm&m=152054772413234&w=4 Thank you, although that does not explain why the changes are happening, only that you are introducing the new API so that you can start converting users (which is the normal way of doing it, no?). What I meant is the discussion that led to commit 1c8f422059ae5 itself (which has a link inside, but is broken). >> I am out of the loop on these mm changes, so please indulge me, but: >> >> * Why is there no documentation on vmf_insert_page() while >> vm_insert_page() had it? (specially since it seems you want to remove >> vm_insert_page()). > > The plan is to convert vm_insert_{page,pfn,mixed} to > vmf_insert_{page,pfn,mixed}. As a good intermediate > steps inline wrapper vmf_insert_{pfn,page,mixed} are > introduced. After all the drivers converted, we will convert > vm_insert_page to vmf_insert_page and remove the inline > wrapper and update the document at the same time. Yeah, that is what 1c8f422059ae5 ("mm: change return type to vm_fault_t") seems to say at the end (thanks for clarifying). Still, that does not explain why the documentation was not added at the same time as soon the new API is introduced. I don't see how it matters that they are wrappers. Actually, I think the wrappers should have been the final functions already in memory.c, their declarations in mm.h, etc. That way you would minimize the code changes later on: you would be only removing dead code, rather than changing code again. Even if you forward the calls for the moment, it would have been a much smaller change later on. > >> >> * Shouldn't we have a simple remap_page() or remap_kernel_page() to >> fit this use case and avoid that dance? (another driver in auxdisplay >> will require the same change, and I guess others in the kernel as >> well). > > > There are few drivers similar like auxdisplay where straight forward > conversion from vm_insert_page to vmf_insert_page is not possible. > > So I mapped the kernel memory to user vma using remap_pfn_range > and remove vm_insert_page in this driver. > > Other way, is to replace vm_insert_page with vmf_insert_page() and > then convert VM_FAULT_CODE back to errno. But as part of vm_fault_t > migration we have already removed/cleanup most the errno to VM_FAULT_CODE > mapping from drivers. So I prefer not to take this option. > > Third, we can introduce a similar API like vm_insert_page say, > vm_insert_kmem_page() and use it for same scenarios like this. Yep, I think that is the best, unless there are only a couple of users and you think nobody should be using it in the future. Cheers, Miguel ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] auxdisplay/cfag12864bfb.c: Replace vm_insert_page 2018-09-20 20:23 [PATCH] auxdisplay/cfag12864bfb.c: Replace vm_insert_page Souptick Joarder 2018-09-20 21:26 ` Miguel Ojeda @ 2018-09-22 12:21 ` Matthew Wilcox 1 sibling, 0 replies; 5+ messages in thread From: Matthew Wilcox @ 2018-09-22 12:21 UTC (permalink / raw) To: Souptick Joarder; +Cc: miguel.ojeda.sandonis, linux-kernel, akpm On Fri, Sep 21, 2018 at 01:53:16AM +0530, Souptick Joarder wrote: > There is a plan to remove vm_insert_page permanently > and replace it with new API vmf_insert_page which will > return vm_fault_t type. As part of it vm_insert_page > is removed from this driver. > > remap_pfn_range() will be used to map kernel memory to > user vma. I think this patch is dangerous. It's not buggy *for this driver*, but it gives the impression that it's safe to use remap_pfn_range() for this *kind* of driver. ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2018-09-22 13:39 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-09-20 20:23 [PATCH] auxdisplay/cfag12864bfb.c: Replace vm_insert_page Souptick Joarder 2018-09-20 21:26 ` Miguel Ojeda 2018-09-21 11:07 ` Souptick Joarder 2018-09-22 13:39 ` Miguel Ojeda 2018-09-22 12:21 ` Matthew Wilcox
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).