Re: [PATCH] IB/mlx5: Convert to use vm_map_pages_zero()

[Souptick Joarder]

On Tue, Aug 27, 2019 at 9:19 PM Jason Gunthorpe <jgg@xxxxxxxx> wrote:
>
> On Tue, Aug 27, 2019 at 01:48:57AM +0530, Souptick Joarder wrote:
> > On Mon, Aug 26, 2019 at 5:50 PM Jason Gunthorpe <jgg@xxxxxxxx> wrote:
> > >
> > > On Mon, Aug 26, 2019 at 01:32:09AM +0530, Souptick Joarder wrote:
> > > > On Mon, Aug 26, 2019 at 1:13 AM Jason Gunthorpe <jgg@xxxxxxxx> wrote:
> > > > >
> > > > > On Sun, Aug 25, 2019 at 11:37:27AM +0530, Souptick Joarder wrote:
> > > > > > First, length passed to mmap is checked explicitly against
> > > > > > PAGE_SIZE.
> > > > > >
> > > > > > Second, if vma->vm_pgoff is passed as non zero, it would return
> > > > > > error. It appears like driver is expecting vma->vm_pgoff to
> > > > > > be passed as 0 always.
> > > > >
> > > > > ? pg_off is not zero
> > > >
> > > > Sorry, I mean, driver has a check against non zero to return error -EOPNOTSUPP
> > > > which means in true scenario driver is expecting vma->vm_pgoff should be passed
> > > > as 0.
> > >
> > > get_index is masking vm_pgoff, it is not 0
> >
> > Sorry, I missed this part. Further looking into code,
> > in mlx5_ib_mmap(), vma_vm_pgoff is used to get command and
> > inside mlx5_ib_mmap_clock_info_page() entire *dev->mdev->clock_info*
> > is mapped.
> >
> > Consider that, the below modification will only take care of vma length
> > error check inside vm_map_pages_zero() and an extra check for vma
> > length is not needed.
>
> What is the point of vm_map_pages_zero() Is there some reason we should
> prefer it for mapping a single page?

vm_map_pages_zero() can be used to map single/ multiple pages both.
There were drivers previously which either check length and pg_off explicitly
or didn't check for incorrect value of length /pg_off passed to it at
all. Calling
vm_map_pages_zero() in those places were more appropriate as it has
internal check for both.

Now considering this patch, avoiding an extra check for length
explicitly is the only
part which can be avoided if converted to use vm_map_pages_zero()
because pg_off
is used in different context ( to identify command). So yes,
improvement wise convert
to use vm_map_pages_zero() is not making much difference here.