Re: [PATCH] RDMA/rxe: Pass a pointer to virt_to_page()

[Linus Walleij <linus.walleij@linaro.org>]

On Fri, Mar 24, 2023 at 3:00 PM Jason Gunthorpe <jgg@nvidia.com> wrote:
> On Fri, Mar 24, 2023 at 11:32:52AM +0100, Linus Walleij wrote:
> > Like the other calls in this function virt_to_page() expects
> > a pointer, not an integer.
> >
> > However since many architectures implement virt_to_pfn() as
> > a macro, this function becomes polymorphic and accepts both a
> > (unsigned long) and a (void *).
> >
> > Fix this up with an explicit cast.
> >
> > Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> > ---
> >  drivers/infiniband/sw/rxe/rxe_mr.c | 8 ++++----
> >  1 file changed, 4 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/infiniband/sw/rxe/rxe_mr.c b/drivers/infiniband/sw/rxe/rxe_mr.c
> > index b10aa1580a64..5c90d83002f0 100644
> > --- a/drivers/infiniband/sw/rxe/rxe_mr.c
> > +++ b/drivers/infiniband/sw/rxe/rxe_mr.c
> > @@ -213,7 +213,7 @@ int rxe_mr_init_fast(int max_pages, struct rxe_mr *mr)
> >  static int rxe_set_page(struct ib_mr *ibmr, u64 iova)
> >  {
>
> All these functions have the wrong names, they are kva not IOVA.

This escalated quickly. :D

I'm trying to do the right thing, I try to fix the technical issues first,
and I can follow up with a rename patch if you want.

> > @@ -288,7 +288,7 @@ static void rxe_mr_copy_dma(struct rxe_mr *mr, u64 iova, void *addr,
> >       u8 *va;
>
> >       while (length) {
> > -             page = virt_to_page(iova & mr->page_mask);
> > +             page = virt_to_page((void *)(iova & mr->page_mask));
> >               bytes = min_t(unsigned int, length,
> >                               PAGE_SIZE - page_offset);
>
> This is actually a bug, this function is only called on IB_MR_TYPE_DMA
> and in that case 'iova' is actually a phys addr
>
> So iova should be called phys and the above should be:
>
>                 page = pfn_to_page(phys >> PAGE_SHIFT);

I tried to make a patch fixing all of these up and prepended to v2 of this
patch (which also needed adjustments).

This is a bit tricky so bear with me, also I have no idea how to test this
so hoping for some help there.

I'm a bit puzzled: could the above code (which exist in
three instances in the driver) even work as it is? Or is it not used?
Or is there some failover from DMA to something else that is constantly
happening?

Yours,
Linus Walleij