RE: Re: [PATCH v3] RDMA/siw: Pass a pointer to virt_to_page()

RE: Re: [PATCH v3] RDMA/siw: Pass a pointer to virt_to_page()

[Bernard Metzler]

> -----Original Message-----
> From: Leon Romanovsky <leonro@nvidia.com>
> Sent: Sunday, 4 September 2022 09:23
> To: Linus Walleij <linus.walleij@linaro.org>
> Cc: Jason Gunthorpe <jgg@nvidia.com>; Bernard Metzler
> <BMT@zurich.ibm.com>; linux-rdma@vger.kernel.org
> Subject: [EXTERNAL] Re: [PATCH v3] RDMA/siw: Pass a pointer to
> virt_to_page()
> 
> On Fri, Sep 02, 2022 at 11:59:18PM +0200, Linus Walleij wrote:
> > 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 *).
> >
> > Fixes: b9be6f18cf9e ("rdma/siw: transmit path")
> > Cc: linux-rdma@vger.kernel.org
> > Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> > ---
> > ChangeLog v2->v3:
> > - Add Fixes: tag.
> > 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(-)
> >
> 
> Thanks, applied.

Thanks Linus, thanks Leon. I looked at this only today.

Can we easily fix the two line wraps introduced by this
patch? Without sending an explicit patch on top -- I'd
suggest adding just two line breaks to it. I'd be happy
to see siw code continues to adhere to the 80 char's
per line style.

Thanks very much!
Bernard.

diff --git a/drivers/infiniband/sw/siw/siw_qp_tx.c b/drivers/infiniband/sw/siw/siw_qp_tx.c
index 7d47b521070b..00137dd0223c 100644
--- a/drivers/infiniband/sw/siw/siw_qp_tx.c
+++ b/drivers/infiniband/sw/siw/siw_qp_tx.c
@@ -537,7 +537,8 @@ static int siw_tx_hdt(struct siw_iwarp_tx *c_tx, struct socket *s)
					 * Cast to an uintptr_t to preserve all 64 bits
					 * in sge->laddr.
					 */
-					uintptr_t va = (uintptr_t)(sge->laddr + sge_off);
+					uintptr_t va =
+						(uintptr_t)(sge->laddr + sge_off);
 
					/*
					 * virt_to_page() takes a (void *) pointer
@@ -545,7 +546,8 @@ static int siw_tx_hdt(struct siw_iwarp_tx *c_tx, struct socket *s)
					 * bits on a 64 bit platform and 32 bits on a
					 * 32 bit platform.
					 */
-					page_array[seg] = virt_to_page((void *)(va & PAGE_MASK));
+					page_array[seg] =
+						virt_to_page((void *)(va & PAGE_MASK));
					if (do_crc)
						crypto_shash_update(
							c_tx->mpa_crc_hd,

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

[Linus Walleij]

On Mon, Sep 5, 2022 at 2:02 PM Bernard Metzler <BMT@zurich.ibm.com> wrote:

> Can we easily fix the two line wraps introduced by this
> patch? Without sending an explicit patch on top --

Yeah Lean can just augment it when applying.

> I'd
> suggest adding just two line breaks to it. I'd be happy
> to see siw code continues to adhere to the 80 char's
> per line style.

You will be fighting an uphill battle since checkpatch (which is
what we use to check syntax) now accepts 100 chars/line.
commit bdc48fa11e46f867ea4d75fa59ee87a7f48be144
"checkpatch/coding-style: deprecate 80-column warning"

If there is infiniband consensus to stay with 80 chars per
line, you should send a patch to checkpatch so that it
warns for this for patches to drivers/rdma.

Yours,
Linus Walleij

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

[Leon Romanovsky]

On Mon, Sep 05, 2022 at 02:07:34PM +0200, Linus Walleij wrote:
> On Mon, Sep 5, 2022 at 2:02 PM Bernard Metzler <BMT@zurich.ibm.com> wrote:
> 
> > Can we easily fix the two line wraps introduced by this
> > patch? Without sending an explicit patch on top --
> 
> Yeah Lean can just augment it when applying.

I already promoted that patch to non-rebasable for-next.

> 
> > I'd
> > suggest adding just two line breaks to it. I'd be happy
> > to see siw code continues to adhere to the 80 char's
> > per line style.
> 
> You will be fighting an uphill battle since checkpatch (which is
> what we use to check syntax) now accepts 100 chars/line.
> commit bdc48fa11e46f867ea4d75fa59ee87a7f48be144
> "checkpatch/coding-style: deprecate 80-column warning"
> 
> If there is infiniband consensus to stay with 80 chars per
> line, you should send a patch to checkpatch so that it
> warns for this for patches to drivers/rdma.

It is not infiniband specific, many other subsystems and reviewers
continue to use 80-char limit.

The change to checkpatch came after Linus said that authors should
use their best judgment while dealing with line lengths. Unfortunately,
it was vague enough to apply it to checkpatch.

We continue to use 80 char limit, because clang formatter continues
to wrap everything to 80 chars.

Thanks

> 
> Yours,
> Linus Walleij