On 3/29/23 09:28, Linus Walleij wrote:
> On Fri, Mar 24, 2023 at 3:00 PM Jason Gunthorpe <jgg@xxxxxxxxxx> 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@xxxxxxxxxx>
>>> ---
>>> 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
The driver is a software only emulation of an RDMA device. So there isn't any 'real' DMA going on
just a bunch of memcpy()s.
Bob
