[PATCH 2/2] auxdisplay/ht16k33.c: Convert to use vm_map_pages_zero()

[PATCH 2/2] auxdisplay/ht16k33.c: Convert to use vm_map_pages_zero()

[Souptick Joarder]

While using mmap, the incorrect value of length and vm_pgoff are
ignored and this driver go ahead with mapping fbdev.buffer
to user vma.

Convert vm_insert_pages() to use vm_map_pages_zero(). We could later
"fix" these drivers to behave according to the normal vm_pgoff
offsetting simply by removing the _zero suffix on the function name
and if that causes regressions, it gives us an easy way to revert.

Signed-off-by: Souptick Joarder <jrdr.linux@gmail.com>
---
 drivers/auxdisplay/ht16k33.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/auxdisplay/ht16k33.c b/drivers/auxdisplay/ht16k33.c
index 21393ec..9c0bb77 100644
--- a/drivers/auxdisplay/ht16k33.c
+++ b/drivers/auxdisplay/ht16k33.c
@@ -223,9 +223,9 @@ static int ht16k33_bl_check_fb(struct backlight_device *bl, struct fb_info *fi)
 static int ht16k33_mmap(struct fb_info *info, struct vm_area_struct *vma)
 {
 	struct ht16k33_priv *priv = info->par;
+	struct page *pages = virt_to_page(priv->fbdev.buffer);
 
-	return vm_insert_page(vma, vma->vm_start,
-			      virt_to_page(priv->fbdev.buffer));
+	return vm_map_pages_zero(vma, &pages, 1);
 }
 
 static struct fb_ops ht16k33_fb_ops = {
-- 
1.9.1

Re: [PATCH 2/2] auxdisplay/ht16k33.c: Convert to use vm_map_pages_zero()

[Robin van der Gracht]

On Mon, 20 May 2019 21:00:58 +0530
Souptick Joarder <jrdr.linux@gmail.com> wrote:

> While using mmap, the incorrect value of length and vm_pgoff are
> ignored and this driver go ahead with mapping fbdev.buffer
> to user vma.
> 
> Convert vm_insert_pages() to use vm_map_pages_zero(). We could later
> "fix" these drivers to behave according to the normal vm_pgoff
> offsetting simply by removing the _zero suffix on the function name
> and if that causes regressions, it gives us an easy way to revert.
> 
> Signed-off-by: Souptick Joarder <jrdr.linux@gmail.com>
> ---
>  drivers/auxdisplay/ht16k33.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/auxdisplay/ht16k33.c b/drivers/auxdisplay/ht16k33.c
> index 21393ec..9c0bb77 100644
> --- a/drivers/auxdisplay/ht16k33.c
> +++ b/drivers/auxdisplay/ht16k33.c
> @@ -223,9 +223,9 @@ static int ht16k33_bl_check_fb(struct backlight_device *bl, struct fb_info *fi)
>  static int ht16k33_mmap(struct fb_info *info, struct vm_area_struct *vma)
>  {
>  	struct ht16k33_priv *priv = info->par;
> +	struct page *pages = virt_to_page(priv->fbdev.buffer);
>  
> -	return vm_insert_page(vma, vma->vm_start,
> -			      virt_to_page(priv->fbdev.buffer));
> +	return vm_map_pages_zero(vma, &pages, 1);
>  }
>  
>  static struct fb_ops ht16k33_fb_ops = {

Acked-by: Robin van der Gracht <robin@protonic.nl>

Re: [PATCH 2/2] auxdisplay/ht16k33.c: Convert to use vm_map_pages_zero()

[Souptick Joarder]

On Tue, May 21, 2019 at 12:24 PM Robin van der Gracht <robin@protonic.nl> wrote:
>
> On Mon, 20 May 2019 21:00:58 +0530
> Souptick Joarder <jrdr.linux@gmail.com> wrote:
>
> > While using mmap, the incorrect value of length and vm_pgoff are
> > ignored and this driver go ahead with mapping fbdev.buffer
> > to user vma.
> >
> > Convert vm_insert_pages() to use vm_map_pages_zero(). We could later
> > "fix" these drivers to behave according to the normal vm_pgoff
> > offsetting simply by removing the _zero suffix on the function name
> > and if that causes regressions, it gives us an easy way to revert.
> >
> > Signed-off-by: Souptick Joarder <jrdr.linux@gmail.com>
> > ---
> >  drivers/auxdisplay/ht16k33.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/auxdisplay/ht16k33.c b/drivers/auxdisplay/ht16k33.c
> > index 21393ec..9c0bb77 100644
> > --- a/drivers/auxdisplay/ht16k33.c
> > +++ b/drivers/auxdisplay/ht16k33.c
> > @@ -223,9 +223,9 @@ static int ht16k33_bl_check_fb(struct backlight_device *bl, struct fb_info *fi)
> >  static int ht16k33_mmap(struct fb_info *info, struct vm_area_struct *vma)
> >  {
> >       struct ht16k33_priv *priv = info->par;
> > +     struct page *pages = virt_to_page(priv->fbdev.buffer);
> >
> > -     return vm_insert_page(vma, vma->vm_start,
> > -                           virt_to_page(priv->fbdev.buffer));
> > +     return vm_map_pages_zero(vma, &pages, 1);
> >  }
> >
> >  static struct fb_ops ht16k33_fb_ops = {
>
> Acked-by: Robin van der Gracht <robin@protonic.nl>

Miguel, Ack from Robin is missing in linux-next-20190523 when applied.

Re: [PATCH 2/2] auxdisplay/ht16k33.c: Convert to use vm_map_pages_zero()

[Miguel Ojeda]

On Thu, May 23, 2019 at 10:18 AM Souptick Joarder <jrdr.linux@gmail.com> wrote:
>
> Miguel, Ack from Robin is missing in linux-next-20190523 when applied.

Thanks for the warning! I wanted to review first but didn't get to it yet.

Taking a quick look now, by the way, why does vm_map_pages_zero() (and
__vm_map_pages() etc.) get a pointer to an array instead of a pointer
to the first element?

Cheers,
Miguel

Re: [PATCH 2/2] auxdisplay/ht16k33.c: Convert to use vm_map_pages_zero()

[Miguel Ojeda]

On Mon, May 20, 2019 at 5:26 PM Souptick Joarder <jrdr.linux@gmail.com> wrote:
>
> While using mmap, the incorrect value of length and vm_pgoff are
> ignored and this driver go ahead with mapping fbdev.buffer
> to user vma.

Typos: values*, goes* (same for the other patch)

> Convert vm_insert_pages() to use vm_map_pages_zero(). We could later
> "fix" these drivers to behave according to the normal vm_pgoff
> offsetting simply by removing the _zero suffix on the function name
> and if that causes regressions, it gives us an easy way to revert.

Would it be possible to add a "Link:" to where these new functions are
discussed/used (maybe a lore.kernel.org link?)?

Thanks for the patch!

Cheers,
Miguel

Re: [PATCH 2/2] auxdisplay/ht16k33.c: Convert to use vm_map_pages_zero()

[Miguel Ojeda]

On Thu, May 23, 2019 at 2:58 PM Miguel Ojeda
<miguel.ojeda.sandonis@gmail.com> wrote:
>
> Taking a quick look now, by the way, why does vm_map_pages_zero() (and
> __vm_map_pages() etc.) get a pointer to an array instead of a pointer
> to the first element?

Also, in __vm_map_pages(), semantically w.r.t. to the comment,
shouldn't the first check test for equality too? (i.e. for vm_pgoff ==
num)? (even if such case fails in the second test anyway).

Cheers,
Miguel

Re: [PATCH 2/2] auxdisplay/ht16k33.c: Convert to use vm_map_pages_zero()

[Souptick Joarder]

On Fri, May 24, 2019 at 9:52 AM Miguel Ojeda
<miguel.ojeda.sandonis@gmail.com> wrote:
>
> On Thu, May 23, 2019 at 2:58 PM Miguel Ojeda
> <miguel.ojeda.sandonis@gmail.com> wrote:
> >



> > Taking a quick look now, by the way, why does vm_map_pages_zero() (and
> > __vm_map_pages() etc.) get a pointer to an array instead of a pointer
> > to the first element?

For this particular driver, one page is getting mapped into vma. But
there are other
places where a entire page array ( with more than one pages) mapped into
vma. That's the reason to pass pointer to an array and do rest of the operations
inside __vm_map_pages().

https://lkml.org/lkml/2019/3/18/1265

>
> Also, in __vm_map_pages(), semantically w.r.t. to the comment,
> shouldn't the first check test for equality too? (i.e. for vm_pgoff ==
> num)? (even if such case fails in the second test anyway).

Sorry, didn't get it. Do you mean there should be a separate check for
*vm_pgoff == num* ?


> Cheers,
> Miguel

Re: [PATCH 2/2] auxdisplay/ht16k33.c: Convert to use vm_map_pages_zero()

[Souptick Joarder]

On Thu, May 23, 2019 at 6:53 PM Miguel Ojeda
<miguel.ojeda.sandonis@gmail.com> wrote:
>
> On Mon, May 20, 2019 at 5:26 PM Souptick Joarder <jrdr.linux@gmail.com> wrote:
> >
> > While using mmap, the incorrect value of length and vm_pgoff are
> > ignored and this driver go ahead with mapping fbdev.buffer
> > to user vma.
>
> Typos: values*, goes* (same for the other patch)

Ok, will add it into v2.

>
> > Convert vm_insert_pages() to use vm_map_pages_zero(). We could later
> > "fix" these drivers to behave according to the normal vm_pgoff
> > offsetting simply by removing the _zero suffix on the function name
> > and if that causes regressions, it gives us an easy way to revert.
>
> Would it be possible to add a "Link:" to where these new functions are
> discussed/used (maybe a lore.kernel.org link?)?

This might be helpful.
https://lkml.org/lkml/2018/12/24/204

>
> Thanks for the patch!
>
> Cheers,
> Miguel

Re: [PATCH 2/2] auxdisplay/ht16k33.c: Convert to use vm_map_pages_zero()

[Miguel Ojeda]

On Fri, May 24, 2019 at 8:07 AM Souptick Joarder <jrdr.linux@gmail.com> wrote:
>
> > > Taking a quick look now, by the way, why does vm_map_pages_zero() (and
> > > __vm_map_pages() etc.) get a pointer to an array instead of a pointer
> > > to the first element?
>
> For this particular driver, one page is getting mapped into vma. But
> there are other
> places where a entire page array ( with more than one pages) mapped into
> vma. That's the reason to pass pointer to an array and do rest of the operations
> inside __vm_map_pages().

Ah, "pointer to array of source kernel pages" made me think the actual
`struct page`s were the ones consecutive in memory, not the pointers.
Maybe "array of page pointers" is more clear.

> > Also, in __vm_map_pages(), semantically w.r.t. to the comment,
> > shouldn't the first check test for equality too? (i.e. for vm_pgoff ==
> > num)? (even if such case fails in the second test anyway).
>
> Sorry, didn't get it. Do you mean there should be a separate check for
> *vm_pgoff == num* ?

No, that the first check should be widened. I will send a patch.

Cheers,
Miguel

[PATCH 2/2] auxdisplay/ht16k33.c: Convert to use vm_map_pages_zero()

[Souptick Joarder]

While using mmap, the incorrect values of length and vm_pgoff are
ignored and this driver goes ahead with mapping fbdev.buffer
to user vma.

Convert vm_insert_pages() to use vm_map_pages_zero(). We could later
"fix" these drivers to behave according to the normal vm_pgoff
offsetting simply by removing the _zero suffix on the function name
and if that causes regressions, it gives us an easy way to revert.

Signed-off-by: Souptick Joarder <jrdr.linux@gmail.com>
Acked-by: Robin van der Gracht <robin@protonic.nl>
---
v2: Fixed minor typo. Acked-by tag added.

 drivers/auxdisplay/ht16k33.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/auxdisplay/ht16k33.c b/drivers/auxdisplay/ht16k33.c
index 21393ec..9c0bb77 100644
--- a/drivers/auxdisplay/ht16k33.c
+++ b/drivers/auxdisplay/ht16k33.c
@@ -223,9 +223,9 @@ static int ht16k33_bl_check_fb(struct backlight_device *bl, struct fb_info *fi)
 static int ht16k33_mmap(struct fb_info *info, struct vm_area_struct *vma)
 {
 	struct ht16k33_priv *priv = info->par;
+	struct page *pages = virt_to_page(priv->fbdev.buffer);
 
-	return vm_insert_page(vma, vma->vm_start,
-			      virt_to_page(priv->fbdev.buffer));
+	return vm_map_pages_zero(vma, &pages, 1);
 }
 
 static struct fb_ops ht16k33_fb_ops = {
-- 
1.9.1