[PATCH] auxdisplay/cfag12864bfb.c: Replace vm_insert_page

[PATCH] auxdisplay/cfag12864bfb.c: Replace vm_insert_page

[Souptick Joarder]

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

Re: [PATCH] auxdisplay/cfag12864bfb.c: Replace vm_insert_page

[Miguel Ojeda]

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

Re: [PATCH] auxdisplay/cfag12864bfb.c: Replace vm_insert_page

[Souptick Joarder]

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.

Re: [PATCH] auxdisplay/cfag12864bfb.c: Replace vm_insert_page

[Matthew Wilcox]

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.

Re: [PATCH] auxdisplay/cfag12864bfb.c: Replace vm_insert_page

[Miguel Ojeda]

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