Re: [PATCH 2/2] auxdisplay/ht16k33.c: Convert to use vm_map_pages_zero()
On Fri, May 24, 2019 at 8:07 AM Souptick Joarder 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()
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
Acked-by: Robin van der Gracht
---
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, , 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()
On Thu, May 23, 2019 at 6:53 PM Miguel Ojeda wrote: > > On Mon, May 20, 2019 at 5:26 PM Souptick Joarder 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()
On Fri, May 24, 2019 at 9:52 AM Miguel Ojeda wrote: > > On Thu, May 23, 2019 at 2:58 PM Miguel Ojeda > 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()
On Thu, May 23, 2019 at 2:58 PM Miguel Ojeda 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()
On Mon, May 20, 2019 at 5:26 PM Souptick Joarder 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()
On Thu, May 23, 2019 at 10:18 AM Souptick Joarder 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()
On Tue, May 21, 2019 at 12:24 PM Robin van der Gracht wrote:
>
> On Mon, 20 May 2019 21:00:58 +0530
> Souptick Joarder 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
> > ---
> > 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, , 1);
> > }
> >
> > static struct fb_ops ht16k33_fb_ops = {
>
> Acked-by: Robin van der Gracht
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()
On Mon, 20 May 2019 21:00:58 +0530
Souptick Joarder 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
> ---
> 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, , 1);
> }
>
> static struct fb_ops ht16k33_fb_ops = {
Acked-by: Robin van der Gracht
[PATCH 2/2] auxdisplay/ht16k33.c: Convert to use vm_map_pages_zero()
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
---
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, , 1);
}
static struct fb_ops ht16k33_fb_ops = {
--
1.9.1
