| Message ID | 20220901133056.570396-1-linus.walleij@linaro.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2] RDMA/siw: Pass a pointer to virt_to_page() | expand |
> -----Original Message----- > From: Linus Walleij <linus.walleij@linaro.org> > Sent: Thursday, 1 September 2022 15:31 > To: Jason Gunthorpe <jgg@nvidia.com>; Leon Romanovsky <leonro@nvidia.com>; > Bernard Metzler <BMT@zurich.ibm.com> > Cc: linux-rdma@vger.kernel.org; Linus Walleij <linus.walleij@linaro.org> > Subject: [EXTERNAL] [PATCH v2] RDMA/siw: Pass a pointer to virt_to_page() > > Functions that work on a pointer to virtual memory such as > virt_to_pfn() and users of that function such as > virt_to_page() are supposed to pass a pointer to virtual > memory, ideally a (void *) or other pointer. However since > many architectures implement virt_to_pfn() as a macro, > this function becomes polymorphic and accepts both a > (unsigned long) and a (void *). > > If we instead implement a proper virt_to_pfn(void *addr) > function the following happens (occurred on arch/arm): > > drivers/infiniband/sw/siw/siw_qp_tx.c:32:23: warning: incompatible > integer to pointer conversion passing 'dma_addr_t' (aka 'unsigned int') > to parameter of type 'const void *' [-Wint-conversion] > drivers/infiniband/sw/siw/siw_qp_tx.c:32:37: warning: passing argument > 1 of 'virt_to_pfn' makes pointer from integer without a cast > [-Wint-conversion] > drivers/infiniband/sw/siw/siw_qp_tx.c:538:36: warning: incompatible > integer to pointer conversion passing 'unsigned long long' > to parameter of type 'const void *' [-Wint-conversion] > > Fix this with an explicit cast. In one case where the SIW > SGE uses an unaligned u64 we need a double cast modifying the > virtual address (va) to a platform-specific uintptr_t before > casting to a (void *). > > Cc: linux-rdma@vger.kernel.org > Signed-off-by: Linus Walleij <linus.walleij@linaro.org> > --- > ChangeLog v1->v2: > - Change the local va variable to be uintptr_t, avoiding > double casts in two spots. > --- > drivers/infiniband/sw/siw/siw_qp_tx.c | 18 ++++++++++++++---- > 1 file changed, 14 insertions(+), 4 deletions(-) > > diff --git a/drivers/infiniband/sw/siw/siw_qp_tx.c > b/drivers/infiniband/sw/siw/siw_qp_tx.c > index 1f4e60257700..7d47b521070b 100644 > --- a/drivers/infiniband/sw/siw/siw_qp_tx.c > +++ b/drivers/infiniband/sw/siw/siw_qp_tx.c > @@ -29,7 +29,7 @@ static struct page *siw_get_pblpage(struct siw_mem *mem, > u64 addr, int *idx) > dma_addr_t paddr = siw_pbl_get_buffer(pbl, offset, NULL, idx); > > if (paddr) > - return virt_to_page(paddr); > + return virt_to_page((void *)paddr); > > return NULL; > } > @@ -533,13 +533,23 @@ static int siw_tx_hdt(struct siw_iwarp_tx *c_tx, > struct socket *s) > kunmap_local(kaddr); > } > } else { > - u64 va = sge->laddr + sge_off; > + /* > + * Cast to an uintptr_t to preserve all 64 bits > + * in sge->laddr. > + */ > + uintptr_t va = (uintptr_t)(sge->laddr + sge_off); > > - page_array[seg] = virt_to_page(va & PAGE_MASK); > + /* > + * virt_to_page() takes a (void *) pointer > + * so cast to a (void *) meaning it will be 64 > + * bits on a 64 bit platform and 32 bits on a > + * 32 bit platform. > + */ > + page_array[seg] = virt_to_page((void *)(va & > PAGE_MASK)); > if (do_crc) > crypto_shash_update( > c_tx->mpa_crc_hd, > - (void *)(uintptr_t)va, > + (void *)va, > plen); > } > > -- > 2.37.2 Thanks Linus! Code looks good to me. For a decent patch, add a Fixes line and resend. It fixes commit b9be6f18cf9e rdma/siw: transmit path. I also found the description of what your patch does a little to verbose, but that is maybe just my taste, preferring a very crisp and short description ;) Thanks, Bernard.
On Thu, Sep 1, 2022 at 3:47 PM Bernard Metzler <BMT@zurich.ibm.com> wrote: > Thanks Linus! Code looks good to me. For a decent patch, > add a Fixes line and resend. It fixes commit > b9be6f18cf9e rdma/siw: transmit path. OK added it and sent v3 > I also found the description of what your patch does > a little to verbose, but that is maybe just my taste, > preferring a very crisp and short description ;) That's a matter of taste so I wouldn't know what to write other than that you want less. Whoever applies the patch is free to edit, cut and write whatever commit message they like. Yours, Linus Walleij
diff --git a/drivers/infiniband/sw/siw/siw_qp_tx.c b/drivers/infiniband/sw/siw/siw_qp_tx.c index 1f4e60257700..7d47b521070b 100644 --- a/drivers/infiniband/sw/siw/siw_qp_tx.c +++ b/drivers/infiniband/sw/siw/siw_qp_tx.c @@ -29,7 +29,7 @@ static struct page *siw_get_pblpage(struct siw_mem *mem, u64 addr, int *idx) dma_addr_t paddr = siw_pbl_get_buffer(pbl, offset, NULL, idx); if (paddr) - return virt_to_page(paddr); + return virt_to_page((void *)paddr); return NULL; } @@ -533,13 +533,23 @@ static int siw_tx_hdt(struct siw_iwarp_tx *c_tx, struct socket *s) kunmap_local(kaddr); } } else { - u64 va = sge->laddr + sge_off; + /* + * Cast to an uintptr_t to preserve all 64 bits + * in sge->laddr. + */ + uintptr_t va = (uintptr_t)(sge->laddr + sge_off); - page_array[seg] = virt_to_page(va & PAGE_MASK); + /* + * virt_to_page() takes a (void *) pointer + * so cast to a (void *) meaning it will be 64 + * bits on a 64 bit platform and 32 bits on a + * 32 bit platform. + */ + page_array[seg] = virt_to_page((void *)(va & PAGE_MASK)); if (do_crc) crypto_shash_update( c_tx->mpa_crc_hd, - (void *)(uintptr_t)va, + (void *)va, plen); }
Functions that work on a pointer to virtual memory such as virt_to_pfn() and users of that function such as virt_to_page() are supposed to pass a pointer to virtual memory, ideally a (void *) or other pointer. However since many architectures implement virt_to_pfn() as a macro, this function becomes polymorphic and accepts both a (unsigned long) and a (void *). If we instead implement a proper virt_to_pfn(void *addr) function the following happens (occurred on arch/arm): drivers/infiniband/sw/siw/siw_qp_tx.c:32:23: warning: incompatible integer to pointer conversion passing 'dma_addr_t' (aka 'unsigned int') to parameter of type 'const void *' [-Wint-conversion] drivers/infiniband/sw/siw/siw_qp_tx.c:32:37: warning: passing argument 1 of 'virt_to_pfn' makes pointer from integer without a cast [-Wint-conversion] drivers/infiniband/sw/siw/siw_qp_tx.c:538:36: warning: incompatible integer to pointer conversion passing 'unsigned long long' to parameter of type 'const void *' [-Wint-conversion] Fix this with an explicit cast. In one case where the SIW SGE uses an unaligned u64 we need a double cast modifying the virtual address (va) to a platform-specific uintptr_t before casting to a (void *). Cc: linux-rdma@vger.kernel.org Signed-off-by: Linus Walleij <linus.walleij@linaro.org> --- ChangeLog v1->v2: - Change the local va variable to be uintptr_t, avoiding double casts in two spots. --- drivers/infiniband/sw/siw/siw_qp_tx.c | 18 ++++++++++++++---- 1 file changed, 14 insertions(+), 4 deletions(-)