siw: Fix pointer-to-int-cast warning in siw_rx_pbl()

PATCH Applied Tom Seewald 3 participants 11 messages Jun 10, 2020 – Jun 16, 2020

Subsystems (2)

InfiniBand Soft-iWARP driver (siw)

lore

Share & tools

lore

Tom Seewald

author

2020-06-10 17:48 UTC #0 lore raw

PATCH 1/1 siw: Fix pointer-to-int-cast warning in siw_rx_pbl()

The variable buf_addr is type dma_addr_t, which may not be the same size as a pointer. To ensure it is the correct size, cast to a uintptr_t.

Signed-off-by Tom Seewald <[email protected]>

---
 drivers/infiniband/sw/siw/siw_qp_rx.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

drivers/infiniband/sw/siw/siw_qp_rx.c +2 -1

index 650520244ed0..7271d705f4b0 100644
@@ -139,7 +139,8 @@ static int siw_rx_pbl(struct siw_rx_stream *srx, int *pbl_idx,
 			break;
 
 		bytes = min(bytes, len);
-		if (siw_rx_kva(srx, (void *)buf_addr, bytes) == bytes) {
+		if (siw_rx_kva(srx, (void *)(uintptr_t)buf_addr, bytes) ==
+		    bytes) {
 			copied += bytes;
 			offset += bytes;
 			len -= bytes;

Signature · 1 line

2.20.1

Jason Gunthorpe

2020-06-10 17:50 UTC #1 lore raw

Quoted 11 lines · Tom Seewald

The variable buf_addr is type dma_addr_t, which may not be the same size as a pointer. To ensure it is the correct size, cast to a uintptr_t.
Signed-off-by Tom Seewald <[email protected]>
 drivers/infiniband/sw/siw/siw_qp_rx.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
drivers/infiniband/sw/siw/siw_qp_rx.c +1 -0
index 650520244ed0..7271d705f4b0 100644
+++ b/drivers/infiniband/sw/siw/siw_qp_rx.c

drivers/infiniband/sw/siw/siw_qp_rx.c static int siw_rx_pbl(struct siw_rx_stream *srx, int *pbl_idx, · line 143

Quoted 6 lines

drivers/infiniband/sw/siw/siw_qp_rx.c +2 -1

@@ -139,7 +139,8 @@ static int siw_rx_pbl(struct siw_rx_stream *srx, int *pbl_idx,
 			break;
 
 		bytes = min(bytes, len);
-		if (siw_rx_kva(srx, (void *)buf_addr, bytes) == bytes) {
+		if (siw_rx_kva(srx, (void *)(uintptr_t)buf_addr, bytes) ==
+		    bytes) {

How is a dma_addr_t being cast to a void *? That can't be right? Bernard??

Jason

Tom Seewald

author

2020-06-10 18:21 UTC #2 lore raw

Quoted 2 lines · Jason Gunthorpe

How is a dma_addr_t being cast to a void *? That can't be right? Bernard??

I was also confused about that, as I couldn't tell how/if it is able to guarantee the bus address is the same as the cpu address. I just assumed it was my lack of familiarity with the codebase.

Bernard Metzler

2020-06-11 10:22 UTC #3 lore raw

In reply to Jason Gunthorpe

-----"Jason Gunthorpe" <[email protected]> wrote: -----

Quoted 34 lines

To: "Tom Seewald" <[email protected]>
From: "Jason Gunthorpe" <[email protected]>
Date: 06/10/2020 07:50PM
Cc: [email protected], "Bernard Metzler" <[email protected]>, "Doug Ledford" <[email protected]>
Subject: [EXTERNAL] Re: [PATCH next] siw: Fix pointer-to-int-cast warning in siw_rx_pbl()
Quoted 1 line · Tom Seewald
The variable buf_addr is type dma_addr_t, which may not be the same
size
Quoted 1 line
as a pointer. To ensure it is the correct size, cast to a
uintptr_t.
Review
Quoted 6 lines
Signed-off-by Tom Seewald <[email protected]>
 drivers/infiniband/sw/siw/siw_qp_rx.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
(quoted diff) +0 -0
b/drivers/infiniband/sw/siw/siw_qp_rx.c
Review static int siw_rx_pbl(struct siw_rx_stream
Quoted 3 lines
index 650520244ed0..7271d705f4b0 100644
+++ b/drivers/infiniband/sw/siw/siw_qp_rx.c
(quoted diff) +0 -0
@@ -139,7 +139,8 @@ static int siw_rx_pbl(struct siw_rx_stream
*srx, int *pbl_idx,
drivers/infiniband/sw/siw/siw_qp_rx.c static int siw_rx_pbl(struct siw_rx_stream *srx, int *pbl_idx, · line 143
Quoted 6 lines
drivers/infiniband/sw/siw/siw_qp_rx.c +2 -1
 			break;
 
 		bytes = min(bytes, len);
-		if (siw_rx_kva(srx, (void *)buf_addr, bytes) == bytes) {
+		if (siw_rx_kva(srx, (void *)(uintptr_t)buf_addr, bytes) ==
+		    bytes) {
How is a dma_addr_t being cast to a void *? That can't be right? Bernard??

Quoted 2 lines

Jason

Hi Tom, Hi Jason,

Thanks for looking into that.

siw_rx_kva() calls skb_copy_bits() to move data to its kernel clients destination. It expects a void * target address. This is why I chose it for siw_rx_kva() as well. One could say siw_rx_kva() should better get an uintptr_t as target argument, which would probably make it look more clean. And we rename it to siw_rx_kbuf(), and we cast from uintptr_t to (void *) just for
skb_copy_bits(skb *, off, (void *)dest, len)

This would avoid all those nasty (void *) casting at all (!) the places we are calling siw_rx_kva().

Makes sense?

Thanks,
Bernard.

Jason Gunthorpe

2020-06-11 11:35 UTC #4 lore raw

Quoted 50 lines · Bernard Metzler

Quoted 35 lines
To: "Tom Seewald" <[email protected]>
From: "Jason Gunthorpe" <[email protected]>
Date: 06/10/2020 07:50PM
Cc: [email protected], "Bernard Metzler" <[email protected]>, "Doug Ledford" <[email protected]>
Subject: [EXTERNAL] Re: [PATCH next] siw: Fix pointer-to-int-cast warning in siw_rx_pbl()
Quoted 1 line · Tom Seewald
The variable buf_addr is type dma_addr_t, which may not be the same
size
Quoted 1 line
as a pointer. To ensure it is the correct size, cast to a
uintptr_t.
Review
Quoted 6 lines
Signed-off-by Tom Seewald <[email protected]>
 drivers/infiniband/sw/siw/siw_qp_rx.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
(quoted diff) +0 -0
b/drivers/infiniband/sw/siw/siw_qp_rx.c
Review static int siw_rx_pbl(struct siw_rx_stream
Quoted 3 lines
index 650520244ed0..7271d705f4b0 100644
+++ b/drivers/infiniband/sw/siw/siw_qp_rx.c
(quoted diff) +0 -0
@@ -139,7 +139,8 @@ static int siw_rx_pbl(struct siw_rx_stream
*srx, int *pbl_idx,
drivers/infiniband/sw/siw/siw_qp_rx.c static int siw_rx_pbl(struct siw_rx_stream *srx, int *pbl_idx, · line 143
Quoted 6 lines
drivers/infiniband/sw/siw/siw_qp_rx.c +2 -1
 			break;
 
 		bytes = min(bytes, len);
-		if (siw_rx_kva(srx, (void *)buf_addr, bytes) == bytes) {
+		if (siw_rx_kva(srx, (void *)(uintptr_t)buf_addr, bytes) ==
+		    bytes) {
How is a dma_addr_t being cast to a void *? That can't be right? Bernard??
Quoted 2 lines
Jason
Hi Tom, Hi Jason,
Thanks for looking into that.
siw_rx_kva() calls skb_copy_bits() to move data to its kernel clients destination. It expects a void * target address. This is why I chose it for siw_rx_kva() as well. One could say siw_rx_kva() should better get an uintptr_t as target argument, which would probably make it look more clean. And we rename it to siw_rx_kbuf(), and we cast from uintptr_t to (void *) just for
skb_copy_bits(skb *, off, (void *)dest, len)

Quoted 2 lines

This would avoid all those nasty (void *) casting at all (!) the places we are calling siw_rx_kva().

But where did the dma_addr_t come from?

Jason

Bernard Metzler

2020-06-11 14:11 UTC #5 lore raw

-----"Jason Gunthorpe" <[email protected]> wrote: -----

Quoted 60 lines

To: "Bernard Metzler" <[email protected]>
From: "Jason Gunthorpe" <[email protected]>
Date: 06/11/2020 01:35PM
Cc: "Tom Seewald" <[email protected]>, [email protected], "Doug Ledford" <[email protected]>
Subject: [EXTERNAL] Re: Re: [PATCH next] siw: Fix pointer-to-int-cast warning in siw_rx_pbl()
Quoted 11 lines · Bernard Metzler
Quoted 11 lines
To: "Tom Seewald" <[email protected]>
From: "Jason Gunthorpe" <[email protected]>
Date: 06/10/2020 07:50PM
Cc: [email protected], "Bernard Metzler" <[email protected]>, "Doug Ledford" <[email protected]>
Subject: [EXTERNAL] Re: [PATCH next] siw: Fix pointer-to-int-cast warning in siw_rx_pbl()
Quoted 1 line · Tom Seewald
The variable buf_addr is type dma_addr_t, which may not be the
same
Quoted 39 lines
Quoted 24 lines
size
Quoted 1 line
as a pointer. To ensure it is the correct size, cast to a
uintptr_t.
Review
Quoted 6 lines
Signed-off-by Tom Seewald <[email protected]>
 drivers/infiniband/sw/siw/siw_qp_rx.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
(quoted diff) +0 -0
b/drivers/infiniband/sw/siw/siw_qp_rx.c
Review static int siw_rx_pbl(struct siw_rx_stream
Quoted 3 lines
index 650520244ed0..7271d705f4b0 100644
+++ b/drivers/infiniband/sw/siw/siw_qp_rx.c
(quoted diff) +0 -0
@@ -139,7 +139,8 @@ static int siw_rx_pbl(struct siw_rx_stream
*srx, int *pbl_idx,
drivers/infiniband/sw/siw/siw_qp_rx.c static int siw_rx_pbl(struct siw_rx_stream *srx, int *pbl_idx, · line 143
Quoted 6 lines
drivers/infiniband/sw/siw/siw_qp_rx.c +2 -1
 			break;
 
 		bytes = min(bytes, len);
-		if (siw_rx_kva(srx, (void *)buf_addr, bytes) == bytes) {
+		if (siw_rx_kva(srx, (void *)(uintptr_t)buf_addr, bytes) ==
+		    bytes) {
How is a dma_addr_t being cast to a void *? That can't be right? Bernard??
Quoted 2 lines
Jason
Hi Tom, Hi Jason,
Thanks for looking into that.
siw_rx_kva() calls skb_copy_bits() to move data to its kernel clients destination. It expects a void * target address. This is why I chose it for siw_rx_kva() as well. One could say siw_rx_kva() should better get an uintptr_t as target argument, which would probably make it look more clean. And we rename it to siw_rx_kbuf(), and we cast from uintptr_t to (void *) just for
skb_copy_bits(skb *, off, (void *)dest, len)

Quoted 5 lines

Quoted 2 lines
This would avoid all those nasty (void *) casting at all (!) the places we are calling siw_rx_kva().
But where did the dma_addr_t come from?

It initially comes from the scatterlist provided by the kernel user via drivers .map_mr_sg() method. There we get a dma_addr_t describing the users buffer.

Thanks,
Bernard.

Jason Gunthorpe

2020-06-11 14:23 UTC #6 lore raw

Quoted 61 lines · Bernard Metzler

Quoted 61 lines
To: "Bernard Metzler" <[email protected]>
From: "Jason Gunthorpe" <[email protected]>
Date: 06/11/2020 01:35PM
Cc: "Tom Seewald" <[email protected]>, [email protected], "Doug Ledford" <[email protected]>
Subject: [EXTERNAL] Re: Re: [PATCH next] siw: Fix pointer-to-int-cast warning in siw_rx_pbl()
Quoted 11 lines · Bernard Metzler
Quoted 11 lines
To: "Tom Seewald" <[email protected]>
From: "Jason Gunthorpe" <[email protected]>
Date: 06/10/2020 07:50PM
Cc: [email protected], "Bernard Metzler" <[email protected]>, "Doug Ledford" <[email protected]>
Subject: [EXTERNAL] Re: [PATCH next] siw: Fix pointer-to-int-cast warning in siw_rx_pbl()
Quoted 1 line · Tom Seewald
The variable buf_addr is type dma_addr_t, which may not be the
same
Quoted 39 lines
Quoted 24 lines
size
Quoted 1 line
as a pointer. To ensure it is the correct size, cast to a
uintptr_t.
Review
Quoted 6 lines
Signed-off-by Tom Seewald <[email protected]>
 drivers/infiniband/sw/siw/siw_qp_rx.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
(quoted diff) +0 -0
b/drivers/infiniband/sw/siw/siw_qp_rx.c
Review static int siw_rx_pbl(struct siw_rx_stream
Quoted 3 lines
index 650520244ed0..7271d705f4b0 100644
+++ b/drivers/infiniband/sw/siw/siw_qp_rx.c
(quoted diff) +0 -0
@@ -139,7 +139,8 @@ static int siw_rx_pbl(struct siw_rx_stream
*srx, int *pbl_idx,
drivers/infiniband/sw/siw/siw_qp_rx.c static int siw_rx_pbl(struct siw_rx_stream *srx, int *pbl_idx, · line 143
Quoted 6 lines
drivers/infiniband/sw/siw/siw_qp_rx.c +2 -1
 			break;
 
 		bytes = min(bytes, len);
-		if (siw_rx_kva(srx, (void *)buf_addr, bytes) == bytes) {
+		if (siw_rx_kva(srx, (void *)(uintptr_t)buf_addr, bytes) ==
+		    bytes) {
How is a dma_addr_t being cast to a void *? That can't be right? Bernard??
Quoted 2 lines
Jason
Hi Tom, Hi Jason,
Thanks for looking into that.
siw_rx_kva() calls skb_copy_bits() to move data to its kernel clients destination. It expects a void * target address. This is why I chose it for siw_rx_kva() as well. One could say siw_rx_kva() should better get an uintptr_t as target argument, which would probably make it look more clean. And we rename it to siw_rx_kbuf(), and we cast from uintptr_t to (void *) just for
skb_copy_bits(skb *, off, (void *)dest, len)

Quoted 8 lines

Quoted 5 lines
Quoted 2 lines
This would avoid all those nasty (void *) casting at all (!) the places we are calling siw_rx_kva().
But where did the dma_addr_t come from?
It initially comes from the scatterlist provided by the kernel user via drivers .map_mr_sg() method. There we get a dma_addr_t describing the users buffer.

For the SW dma maps you have to convert the dma_addr_t to a kva using kmap, it cannot just be casted.

Jason

Bernard Metzler

2020-06-11 15:06 UTC #7 lore raw

-----"Jason Gunthorpe" <[email protected]> wrote: -----

Quoted 79 lines

To: "Bernard Metzler" <[email protected]>
From: "Jason Gunthorpe" <[email protected]>
Date: 06/11/2020 04:24PM
Cc: "Tom Seewald" <[email protected]>, [email protected], "Doug Ledford" <[email protected]>
Subject: [EXTERNAL] Re: Re: [PATCH next] siw: Fix pointer-to-int-cast warning in siw_rx_pbl()
Quoted 5 lines · Bernard Metzler
Quoted 5 lines
To: "Bernard Metzler" <[email protected]>
From: "Jason Gunthorpe" <[email protected]>
Date: 06/11/2020 01:35PM
Cc: "Tom Seewald" <[email protected]>,
[email protected],
Quoted 2 lines
Quoted 2 lines
"Doug Ledford" <[email protected]>
Subject: [EXTERNAL] Re: Re: [PATCH next] siw: Fix
pointer-to-int-cast
Quoted 10 lines
Quoted 10 lines
warning in siw_rx_pbl()
Quoted 7 lines · Bernard Metzler
Quoted 7 lines
To: "Tom Seewald" <[email protected]>
From: "Jason Gunthorpe" <[email protected]>
Date: 06/10/2020 07:50PM
Cc: [email protected], "Bernard Metzler" <[email protected]>, "Doug Ledford" <[email protected]>
Subject: [EXTERNAL] Re: [PATCH next] siw: Fix
pointer-to-int-cast
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
warning in siw_rx_pbl()
Quoted 1 line · Tom Seewald
The variable buf_addr is type dma_addr_t, which may not be
the
Quoted 9 lines
Quoted 9 lines
same
Quoted 8 lines
Quoted 8 lines
size
Quoted 1 line
as a pointer. To ensure it is the correct size, cast to a
uintptr_t.
Quoted 5 lines
Signed-off-by Tom Seewald <[email protected]>
 drivers/infiniband/sw/siw/siw_qp_rx.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Review
Quoted 1 line
(quoted diff) +0 -0
b/drivers/infiniband/sw/siw/siw_qp_rx.c
Review static int siw_rx_pbl(struct
Quoted 3 lines
index 650520244ed0..7271d705f4b0 100644
+++ b/drivers/infiniband/sw/siw/siw_qp_rx.c
(quoted diff) +0 -0
@@ -139,7 +139,8 @@ static int siw_rx_pbl(struct
siw_rx_stream
Quoted 9 lines
Quoted 9 lines
Quoted 9 lines
Quoted 9 lines
*srx, int *pbl_idx,
drivers/infiniband/sw/siw/siw_qp_rx.c static int siw_rx_pbl(struct siw_rx_stream *srx, int *pbl_idx, · line 143
Quoted 6 lines
drivers/infiniband/sw/siw/siw_qp_rx.c +2 -1
 			break;
 
 		bytes = min(bytes, len);
-		if (siw_rx_kva(srx, (void *)buf_addr, bytes) == bytes) {
+		if (siw_rx_kva(srx, (void *)(uintptr_t)buf_addr, bytes) ==
+		    bytes) {
How is a dma_addr_t being cast to a void *? That can't be
right?
Quoted 20 lines
Quoted 20 lines
Quoted 19 lines
Quoted 4 lines
Bernard??
Jason
Hi Tom, Hi Jason,
Thanks for looking into that.
siw_rx_kva() calls skb_copy_bits() to move data to its kernel clients destination. It expects a void * target address. This is why I chose it for siw_rx_kva() as well. One could say siw_rx_kva() should better get an uintptr_t as target argument, which would probably make it look more clean. And we rename it to siw_rx_kbuf(), and we cast from uintptr_t to (void *) just for
skb_copy_bits(skb *, off, (void *)dest, len)
This would avoid all those nasty (void *) casting at all (!) the places we are calling siw_rx_kva().

Quoted 9 lines

Quoted 5 lines
Quoted 2 lines
But where did the dma_addr_t come from?
It initially comes from the scatterlist provided by the kernel user via drivers .map_mr_sg() method. There we get a dma_addr_t describing the users buffer.
For the SW dma maps you have to convert the dma_addr_t to a kva using kmap, it cannot just be casted.

True for a real dma addr. But here the user initially came with an address it got from dma_virt_ops.dma_virt_map_page(), which provides the virtual address of the page referenced, casted to dma_addr_t. So we mimic some great dma_addr_t stuff we do not need for a SW driver and in the end we even have to call heavy kmap_atomic() to just get the very same addr again. This I don't want. It's why I just casted it back to void *. hmmm.

Thanks,
Bernard.

Jason Gunthorpe

2020-06-11 17:05 UTC #8 lore raw

Quoted 11 lines · Bernard Metzler

Quoted 7 lines
Quoted 3 lines
It initially comes from the scatterlist provided by the kernel user via drivers .map_mr_sg() method. There we get a dma_addr_t describing the users buffer.
For the SW dma maps you have to convert the dma_addr_t to a kva using kmap, it cannot just be casted.
True for a real dma addr. But here the user initially came with an address it got from dma_virt_ops.dma_virt_map_page(), which provides the virtual address of the page referenced, casted to dma_addr_t.

Oh, that's curiously broken on highmem systems, but OK siw is fine with it like that, though I think it would have been better to have some helper function connected to dma_virt do this cast.

Jason

Bernard Metzler

Reviewed-by

2020-06-12 15:56 UTC #9 lore raw

-----"Jason Gunthorpe" <[email protected]> wrote: -----

Quoted 22 lines

To: "Bernard Metzler" <[email protected]>
From: "Jason Gunthorpe" <[email protected]>
Date: 06/11/2020 07:05PM
Cc: "Tom Seewald" <[email protected]>, [email protected], "Doug Ledford" <[email protected]>
Subject: [EXTERNAL] Re: Re: [PATCH next] siw: Fix pointer-to-int-cast warning in siw_rx_pbl()
Quoted 5 lines · Bernard Metzler
Quoted 5 lines
Quoted 3 lines
It initially comes from the scatterlist provided by the kernel user via drivers .map_mr_sg() method. There we get a dma_addr_t describing the users buffer.
For the SW dma maps you have to convert the dma_addr_t to a kva
using
Quoted 6 lines
Quoted 2 lines
kmap, it cannot just be casted.
True for a real dma addr. But here the user initially came with an address it got from dma_virt_ops.dma_virt_map_page(), which provides the virtual address of the page referenced, casted to dma_addr_t.

Quoted 6 lines

Oh, that's curiously broken on highmem systems, but OK siw is fine with it like that, though I think it would have been better to have some helper function connected to dma_virt do this cast.
Jason

With that, lets take Tom's simple patch for now.

Reviewed-by Bernard Metzler <[email protected]>

Jason Gunthorpe

Applied

2020-06-16 14:03 UTC #10 lore raw

In reply to Tom Seewald

Quoted 7 lines · Tom Seewald

The variable buf_addr is type dma_addr_t, which may not be the same size as a pointer. To ensure it is the correct size, cast to a uintptr_t.
Signed-off-by Tom Seewald <[email protected]>
Reviewed-by Bernard Metzler <[email protected]>
 drivers/infiniband/sw/siw/siw_qp_rx.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Applied to for-rc, thanks

Jason