[PATCH libmlx5 0/6] libmlx5: fix various coverity/clang issues

[PATCH libmlx5 0/6] libmlx5: fix various coverity/clang issues

[Jarod Wilson]

Part of Red Hat's release process involves running packages through a
scan in coverity, and inspecting anything that pops up. Well, with version
1.2.1, coverity was a bit chatty, so I started investigating, and have
attempted to come up with fixes for at least most of what it reported.

Jarod Wilson (6):
  fix size in malloc of qp->sq.wr_data
  fix coverity buffer overrun warning
  fix buffer overrun copying inline header
  fix check of mlx5_store_uidx return
  fix alloc of mlx5_resource table
  fix undefined uuar_index value assignment

CC: Yishai Hadas <yishaih-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>

 src/mlx5.c  |  2 +-
 src/qp.c    |  2 +-
 src/verbs.c | 14 +++++++++-----
 src/wqe.h   |  2 +-
 4 files changed, 12 insertions(+), 8 deletions(-)

-- 
1.8.3.1

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH libmlx5 1/6] fix size in malloc of qp->sq.wr_data

[Jarod Wilson]

Coverity complaint:

1. libmlx5-1.2.1/src/verbs.c:989: suspicious_sizeof: Passing argument
"qp->sq.wqe_cnt * 8UL /* sizeof (qp->sq.wr_data) */" to function "malloc"
and then casting the return value to "uint32_t *" is suspicious.
2. libmlx5-1.2.1/src/verbs.c:989: remediation: Did you intend to use
"sizeof (*qp->sq.wr_data)" instead of "sizeof (qp->sq.wr_data)"?

 #   987|   		}
 #   988|
 #   989|-> 		qp->sq.wr_data = malloc(qp->sq.wqe_cnt * sizeof(qp->sq.wr_data));
 #   990|   		if (!qp->sq.wr_data) {
 #   991|   			errno = ENOMEM;

The other mallocs in this same area properly use sizeof(*foo), this one
should too.

CC: Yishai Hadas <yishaih-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
Signed-off-by: Jarod Wilson <jarod-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
 src/verbs.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/verbs.c b/src/verbs.c
index 40f66c6..7ed394e 100644
--- a/src/verbs.c
+++ b/src/verbs.c
@@ -986,7 +986,7 @@ static int mlx5_alloc_qp_buf(struct ibv_context *context,
 			return err;
 		}
 
-		qp->sq.wr_data = malloc(qp->sq.wqe_cnt * sizeof(qp->sq.wr_data));
+		qp->sq.wr_data = malloc(qp->sq.wqe_cnt * sizeof(*qp->sq.wr_data));
 		if (!qp->sq.wr_data) {
 			errno = ENOMEM;
 			err = -1;
-- 
1.8.3.1

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH libmlx5 1/6] fix size in malloc of qp->sq.wr_data

[Yishai Hadas]

On 7/27/2016 10:17 PM, Jarod Wilson wrote:
> Coverity complaint:
>
> 1. libmlx5-1.2.1/src/verbs.c:989: suspicious_sizeof: Passing argument
> "qp->sq.wqe_cnt * 8UL /* sizeof (qp->sq.wr_data) */" to function "malloc"
> and then casting the return value to "uint32_t *" is suspicious.
> 2. libmlx5-1.2.1/src/verbs.c:989: remediation: Did you intend to use
> "sizeof (*qp->sq.wr_data)" instead of "sizeof (qp->sq.wr_data)"?
>  #   987|   		}
>  #   988|
>  #   989|-> 		qp->sq.wr_data = malloc(qp->sq.wqe_cnt * sizeof(qp->sq.wr_data));
>  #   990|   		if (!qp->sq.wr_data) {
>  #   991|   			errno = ENOMEM;
>
> The other mallocs in this same area properly use sizeof(*foo), this one
> should too.
>
> CC: Yishai Hadas <yishaih-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
> Signed-off-by: Jarod Wilson <jarod-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> ---
>  src/verbs.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/src/verbs.c b/src/verbs.c
> index 40f66c6..7ed394e 100644
> --- a/src/verbs.c
> +++ b/src/verbs.c
> @@ -986,7 +986,7 @@ static int mlx5_alloc_qp_buf(struct ibv_context *context,
>  			return err;
>  		}
>
> -		qp->sq.wr_data = malloc(qp->sq.wqe_cnt * sizeof(qp->sq.wr_data));
> +		qp->sq.wr_data = malloc(qp->sq.wqe_cnt * sizeof(*qp->sq.wr_data));

Actually the driver allocates more memory than really required, will 
take your fix, thanks.

>  		if (!qp->sq.wr_data) {
>  			errno = ENOMEM;
>  			err = -1;
>

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH libmlx5 2/6] fix coverity buffer overrun warning

[Jarod Wilson]

In set_umr_data_seg, there's a union between a 16-byte struct and a
64-byte array, named data. The code then makes a memset() call on the
struct that is sizeof(array) - sizeof(struct) long, which results in
writing 48 bytes to a 16 byte container. Technically, we know this is
actually fine, because of the union, but to silence the warning, we can
just do the memset on the array instead. Same address, same result, but no
warning spew from coverity.

CC: Yishai Hadas <yishaih-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
Signed-off-by: Jarod Wilson <jarod-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
 src/qp.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/qp.c b/src/qp.c
index 51e1176..8bb66be 100644
--- a/src/qp.c
+++ b/src/qp.c
@@ -426,7 +426,7 @@ static void set_umr_data_seg(struct mlx5_qp *qp, enum ibv_mw_type type,
 	data->klm.mkey = htonl(bind_info->mr->lkey);
 	data->klm.address = htonll(bind_info->addr);
 
-	memset(&data->klm + 1, 0, sizeof(data->reserved) -
+	memset(&data->reserved + 1, 0, sizeof(data->reserved) -
 	       sizeof(data->klm));
 
 	*seg += sizeof(*data);
-- 
1.8.3.1

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH libmlx5 2/6] fix coverity buffer overrun warning

[Yishai Hadas]

On 7/27/2016 10:17 PM, Jarod Wilson wrote:
> In set_umr_data_seg, there's a union between a 16-byte struct and a
> 64-byte array, named data. The code then makes a memset() call on the
> struct that is sizeof(array) - sizeof(struct) long, which results in
> writing 48 bytes to a 16 byte container. Technically, we know this is
> actually fine, because of the union, but to silence the warning, we can
> just do the memset on the array instead. Same address, same result, but no
> warning spew from coverity.
>
> CC: Yishai Hadas <yishaih-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
> Signed-off-by: Jarod Wilson <jarod-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> ---
>  src/qp.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/src/qp.c b/src/qp.c
> index 51e1176..8bb66be 100644
> --- a/src/qp.c
> +++ b/src/qp.c
> @@ -426,7 +426,7 @@ static void set_umr_data_seg(struct mlx5_qp *qp, enum ibv_mw_type type,
>  	data->klm.mkey = htonl(bind_info->mr->lkey);
>  	data->klm.address = htonll(bind_info->addr);
>
> -	memset(&data->klm + 1, 0, sizeof(data->reserved) -
> +	memset(&data->reserved + 1, 0, sizeof(data->reserved) -
>  	       sizeof(data->klm));

As you pointed out this is false alarm, code is correct.

Your suggestion seems wrong as it skipped size of 'reserved' instead of 
size of 'klm' (i.e. 16 bytes), isn't it ?

>  	*seg += sizeof(*data);
>

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH libmlx5 2/6] fix coverity buffer overrun warning

[Jarod Wilson]

On Thu, Jul 28, 2016 at 05:46:16PM +0300, Yishai Hadas wrote:
> On 7/27/2016 10:17 PM, Jarod Wilson wrote:
> >In set_umr_data_seg, there's a union between a 16-byte struct and a
> >64-byte array, named data. The code then makes a memset() call on the
> >struct that is sizeof(array) - sizeof(struct) long, which results in
> >writing 48 bytes to a 16 byte container. Technically, we know this is
> >actually fine, because of the union, but to silence the warning, we can
> >just do the memset on the array instead. Same address, same result, but no
> >warning spew from coverity.
> >
> >CC: Yishai Hadas <yishaih-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
> >Signed-off-by: Jarod Wilson <jarod-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> >---
> > src/qp.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> >diff --git a/src/qp.c b/src/qp.c
> >index 51e1176..8bb66be 100644
> >--- a/src/qp.c
> >+++ b/src/qp.c
> >@@ -426,7 +426,7 @@ static void set_umr_data_seg(struct mlx5_qp *qp, enum ibv_mw_type type,
> > 	data->klm.mkey = htonl(bind_info->mr->lkey);
> > 	data->klm.address = htonll(bind_info->addr);
> >
> >-	memset(&data->klm + 1, 0, sizeof(data->reserved) -
> >+	memset(&data->reserved + 1, 0, sizeof(data->reserved) -
> > 	       sizeof(data->klm));
> 
> As you pointed out this is false alarm, code is correct.
> 
> Your suggestion seems wrong as it skipped size of 'reserved' instead
> of size of 'klm' (i.e. 16 bytes), isn't it ?

&data->klm + 1 and &data->reserved + 1 should point to the same location,
no? Must admit, I'm a little hazy on how a union pointer works.

Regardless, I think this might be much more straight-forward if we did
something like this:

>From 43e845cc1055e4f86a2e9c78e5a3eae2c1e915c4 Mon Sep 17 00:00:00 2001
From: Jarod Wilson <jarod-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
Date: Wed, 27 Jul 2016 11:35:02 -0400
Subject: [PATCH v2 libmlx5 2/6] fix coverity buffer overrun warning

In set_umr_data_seg, there's a union between a 16-byte struct and a
64-byte array, named data. The code then makes a memset() call on the
struct that is sizeof(array) - sizeof(struct) long, which results in
writing 48 bytes to a 16 byte container. Technically, we know this is
actually fine, because of the union, but to silence the warning and
simplify the pointer math and union gymnastics, simply zero out the entire
storage space before assigning klm bits.

Signed-off-by: Jarod Wilson <jarod-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
 src/qp.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/src/qp.c b/src/qp.c
index 51e1176..17f2c81 100644
--- a/src/qp.c
+++ b/src/qp.c
@@ -422,13 +422,12 @@ static void set_umr_data_seg(struct mlx5_qp *qp, enum ibv_mw_type type,
 		uint8_t				reserved[64];
 	} *data = *seg;
 
+	memset(&data, 0, sizeof(*data));
+
 	data->klm.byte_count = htonl(bind_info->length);
 	data->klm.mkey = htonl(bind_info->mr->lkey);
 	data->klm.address = htonll(bind_info->addr);
 
-	memset(&data->klm + 1, 0, sizeof(data->reserved) -
-	       sizeof(data->klm));
-
 	*seg += sizeof(*data);
 	*size += (sizeof(*data) / 16);
 }
-- 
1.8.3.1


-- 
Jarod Wilson
jarod-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH libmlx5 2/6] fix coverity buffer overrun warning

[Jarod Wilson]

On Thu, Jul 28, 2016 at 12:37:14PM -0400, Jarod Wilson wrote:
> On Thu, Jul 28, 2016 at 05:46:16PM +0300, Yishai Hadas wrote:
> > On 7/27/2016 10:17 PM, Jarod Wilson wrote:
> > >In set_umr_data_seg, there's a union between a 16-byte struct and a
> > >64-byte array, named data. The code then makes a memset() call on the
> > >struct that is sizeof(array) - sizeof(struct) long, which results in
> > >writing 48 bytes to a 16 byte container. Technically, we know this is
> > >actually fine, because of the union, but to silence the warning, we can
> > >just do the memset on the array instead. Same address, same result, but no
> > >warning spew from coverity.
> > >
> > >CC: Yishai Hadas <yishaih-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
> > >Signed-off-by: Jarod Wilson <jarod-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> > >---
> > > src/qp.c | 2 +-
> > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > >diff --git a/src/qp.c b/src/qp.c
> > >index 51e1176..8bb66be 100644
> > >--- a/src/qp.c
> > >+++ b/src/qp.c
> > >@@ -426,7 +426,7 @@ static void set_umr_data_seg(struct mlx5_qp *qp, enum ibv_mw_type type,
> > > 	data->klm.mkey = htonl(bind_info->mr->lkey);
> > > 	data->klm.address = htonll(bind_info->addr);
> > >
> > >-	memset(&data->klm + 1, 0, sizeof(data->reserved) -
> > >+	memset(&data->reserved + 1, 0, sizeof(data->reserved) -
> > > 	       sizeof(data->klm));
> > 
> > As you pointed out this is false alarm, code is correct.
> > 
> > Your suggestion seems wrong as it skipped size of 'reserved' instead
> > of size of 'klm' (i.e. 16 bytes), isn't it ?
> 
> &data->klm + 1 and &data->reserved + 1 should point to the same location,
> no? Must admit, I'm a little hazy on how a union pointer works.
> 
> Regardless, I think this might be much more straight-forward if we did
> something like this:
> 
> From 43e845cc1055e4f86a2e9c78e5a3eae2c1e915c4 Mon Sep 17 00:00:00 2001
> From: Jarod Wilson <jarod-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> Date: Wed, 27 Jul 2016 11:35:02 -0400
> Subject: [PATCH v2 libmlx5 2/6] fix coverity buffer overrun warning
> 
> In set_umr_data_seg, there's a union between a 16-byte struct and a
> 64-byte array, named data. The code then makes a memset() call on the
> struct that is sizeof(array) - sizeof(struct) long, which results in
> writing 48 bytes to a 16 byte container. Technically, we know this is
> actually fine, because of the union, but to silence the warning and
> simplify the pointer math and union gymnastics, simply zero out the entire
> storage space before assigning klm bits.
> 
> Signed-off-by: Jarod Wilson <jarod-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> ---
>  src/qp.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/src/qp.c b/src/qp.c
> index 51e1176..17f2c81 100644
> --- a/src/qp.c
> +++ b/src/qp.c
> @@ -422,13 +422,12 @@ static void set_umr_data_seg(struct mlx5_qp *qp, enum ibv_mw_type type,
>  		uint8_t				reserved[64];
>  	} *data = *seg;
>  
> +	memset(&data, 0, sizeof(*data));

Nope, this is wrong, should be just data, not &data, I think. Or perhaps
memset(&data->reserved, 0, sizeof(data->reserved));. Urgh, pointers...

-- 
Jarod Wilson
jarod-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH libmlx5 3/6] fix buffer overrun copying inline header

[Jarod Wilson]

At present, the size of eseg->inline_hdr_start is 16 bits, while
MLX5_ETH_L2_INLINE_HEADER_SIZE is 18, so there are attempts made to copy
18 bits into 16 bits of storage. The mlx5_dbg() statement in
copy_eth_inline_header() suggests that perhaps
MLX5_ETH_L2_INLINE_HEADER_SIZE should be only 16, not 18. So either that
needs to be changed, or the inline_hdr_start array needs to be bumped up
to 3 bytes instead of 2.

CC: Yishai Hadas <yishaih-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
Signed-off-by: Jarod Wilson <jarod-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
 src/wqe.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/wqe.h b/src/wqe.h
index c2622d5..c0e176d 100644
--- a/src/wqe.h
+++ b/src/wqe.h
@@ -77,7 +77,7 @@ struct mlx5_eqe_qp_srq {
 };
 
 enum {
-	MLX5_ETH_L2_INLINE_HEADER_SIZE	= 18,
+	MLX5_ETH_L2_INLINE_HEADER_SIZE	= 16,
 };
 
 enum {
-- 
1.8.3.1

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH libmlx5 3/6] fix buffer overrun copying inline header

[Jarod Wilson]

On Wed, Jul 27, 2016 at 03:17:24PM -0400, Jarod Wilson wrote:
> At present, the size of eseg->inline_hdr_start is 16 bits, while
> MLX5_ETH_L2_INLINE_HEADER_SIZE is 18, so there are attempts made to copy
> 18 bits into 16 bits of storage. The mlx5_dbg() statement in
> copy_eth_inline_header() suggests that perhaps
> MLX5_ETH_L2_INLINE_HEADER_SIZE should be only 16, not 18. So either that
> needs to be changed, or the inline_hdr_start array needs to be bumped up
> to 3 bytes instead of 2.

Ugh. Now I see what's going on. The copy is actually designed to copy 18
_bytes_, not bits, into inline_hdr_start[2] and inline_hdr[16]. Is there a
particular reason those two aren't just a single array?

-- 
Jarod Wilson
jarod-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH libmlx5 3/6] fix buffer overrun copying inline header

[Jarod Wilson]

On Wed, Jul 27, 2016 at 05:26:10PM -0400, Jarod Wilson wrote:
> On Wed, Jul 27, 2016 at 03:17:24PM -0400, Jarod Wilson wrote:
> > At present, the size of eseg->inline_hdr_start is 16 bits, while
> > MLX5_ETH_L2_INLINE_HEADER_SIZE is 18, so there are attempts made to copy
> > 18 bits into 16 bits of storage. The mlx5_dbg() statement in
> > copy_eth_inline_header() suggests that perhaps
> > MLX5_ETH_L2_INLINE_HEADER_SIZE should be only 16, not 18. So either that
> > needs to be changed, or the inline_hdr_start array needs to be bumped up
> > to 3 bytes instead of 2.
> 
> Ugh. Now I see what's going on. The copy is actually designed to copy 18
> _bytes_, not bits, into inline_hdr_start[2] and inline_hdr[16]. Is there a
> particular reason those two aren't just a single array?

Drop this one. I've got a new patch together that just merges the two.
I've looked over the code, and can't see anything that actually uses
inline_hdr separate from inline_hdr_start.

-- 
Jarod Wilson
jarod-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH libmlx5 4/6] fix check of mlx5_store_uidx return

[Jarod Wilson]

mlx5_store_uidx() returns an int32_t, but create_qp was storing the return
in a uint32_t, and then checking for a value less than 0, which is
impossible with the uint32_t. Use a local int32_t for the return, check
for < 0, then cast to uint32_t and save the result for later use.

CC: Yishai Hadas <yishaih-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
Signed-off-by: Jarod Wilson <jarod-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
 src/verbs.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/src/verbs.c b/src/verbs.c
index 7ed394e..d64e406 100644
--- a/src/verbs.c
+++ b/src/verbs.c
@@ -1224,11 +1224,13 @@ struct ibv_qp *create_qp(struct ibv_context *context,
 		cmd.uidx = 0xffffff;
 		pthread_mutex_lock(&ctx->qp_table_mutex);
 	} else if (!is_xrc_tgt(attr->qp_type)) {
-		usr_idx = mlx5_store_uidx(ctx, qp);
-		if (usr_idx < 0) {
+		int32_t uidx_ret;
+		uidx_ret = mlx5_store_uidx(ctx, qp);
+		if (uidx_ret < 0) {
 			mlx5_dbg(fp, MLX5_DBG_QP, "Couldn't find free user index\n");
 			goto err_rq_db;
 		}
+		usr_idx = (uint32_t)uidx_ret;
 
 		cmd.uidx = usr_idx;
 	}
-- 
1.8.3.1

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH libmlx5 4/6] fix check of mlx5_store_uidx return

[Yishai Hadas]

On 7/27/2016 10:17 PM, Jarod Wilson wrote:
> mlx5_store_uidx() returns an int32_t, but create_qp was storing the return
> in a uint32_t, and then checking for a value less than 0, which is
> impossible with the uint32_t. Use a local int32_t for the return, check
> for < 0, then cast to uint32_t and save the result for later use.

Good point, thanks.

Actually the change can be to use int 'usr_idx' instead of uint32_t. See 
same flow as part of mlx5_create_xrc_srq which works as expected by 
using int.

> CC: Yishai Hadas <yishaih-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
> Signed-off-by: Jarod Wilson <jarod-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> ---
>  src/verbs.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/src/verbs.c b/src/verbs.c
> index 7ed394e..d64e406 100644
> --- a/src/verbs.c
> +++ b/src/verbs.c
> @@ -1224,11 +1224,13 @@ struct ibv_qp *create_qp(struct ibv_context *context,
>  		cmd.uidx = 0xffffff;
>  		pthread_mutex_lock(&ctx->qp_table_mutex);
>  	} else if (!is_xrc_tgt(attr->qp_type)) {
> -		usr_idx = mlx5_store_uidx(ctx, qp);
> -		if (usr_idx < 0) {
> +		int32_t uidx_ret;
> +		uidx_ret = mlx5_store_uidx(ctx, qp);
> +		if (uidx_ret < 0) {
>  			mlx5_dbg(fp, MLX5_DBG_QP, "Couldn't find free user index\n");
>  			goto err_rq_db;
>  		}
> +		usr_idx = (uint32_t)uidx_ret;
>
>  		cmd.uidx = usr_idx;
>  	}
>

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH libmlx5 5/6] fix alloc of mlx5_resource table

[Jarod Wilson]

Defect type: CLANG_WARNING

libmlx5-1.2.1/src/mlx5.c:184:33: note: Result of 'calloc' is converted
to a pointer of type 'struct mlx5_resource *', which is incompatible with
sizeof operand type 'void *'

 #                 ctx->uidx_table[tind].table = calloc(MLX5_UIDX_TABLE_MASK + 1,
 #                                               ^~~~~~
 #   182|
 #   183|   	if (!ctx->uidx_table[tind].refcnt) {
 #   184|-> 		ctx->uidx_table[tind].table = calloc(MLX5_UIDX_TABLE_MASK + 1,
 #   185|   						     sizeof(void *));
 #   186|   		if (!ctx->uidx_table[tind].table)

Use sizeof(struct mlx5_resource *) for calloc size argument instead.

CC: Yishai Hadas <yishaih-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
Signed-off-by: Jarod Wilson <jarod-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
 src/mlx5.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/mlx5.c b/src/mlx5.c
index a5f8daf..99221e6 100644
--- a/src/mlx5.c
+++ b/src/mlx5.c
@@ -182,7 +182,7 @@ int32_t mlx5_store_uidx(struct mlx5_context *ctx, void *rsc)
 
 	if (!ctx->uidx_table[tind].refcnt) {
 		ctx->uidx_table[tind].table = calloc(MLX5_UIDX_TABLE_MASK + 1,
-						     sizeof(void *));
+						     sizeof(struct mlx5_resource *));
 		if (!ctx->uidx_table[tind].table)
 			goto out;
 	}
-- 
1.8.3.1

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH libmlx5 5/6] fix alloc of mlx5_resource table

[Yishai Hadas]

On 7/27/2016 10:17 PM, Jarod Wilson wrote:
> Defect type: CLANG_WARNING
>
> libmlx5-1.2.1/src/mlx5.c:184:33: note: Result of 'calloc' is converted
> to a pointer of type 'struct mlx5_resource *', which is incompatible with
> sizeof operand type 'void *'
>
>  #                 ctx->uidx_table[tind].table = calloc(MLX5_UIDX_TABLE_MASK + 1,
>  #                                               ^~~~~~
>  #   182|
>  #   183|   	if (!ctx->uidx_table[tind].refcnt) {
>  #   184|-> 		ctx->uidx_table[tind].table = calloc(MLX5_UIDX_TABLE_MASK + 1,
>  #   185|   						     sizeof(void *));
>  #   186|   		if (!ctx->uidx_table[tind].table)
>
> Use sizeof(struct mlx5_resource *) for calloc size argument instead.
>
> CC: Yishai Hadas <yishaih-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
> Signed-off-by: Jarod Wilson <jarod-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> ---
>  src/mlx5.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/src/mlx5.c b/src/mlx5.c
> index a5f8daf..99221e6 100644
> --- a/src/mlx5.c
> +++ b/src/mlx5.c
> @@ -182,7 +182,7 @@ int32_t mlx5_store_uidx(struct mlx5_context *ctx, void *rsc)
>
>  	if (!ctx->uidx_table[tind].refcnt) {
>  		ctx->uidx_table[tind].table = calloc(MLX5_UIDX_TABLE_MASK + 1,
> -						     sizeof(void *));
> +						     sizeof(struct mlx5_resource *));

As both are pointers the allocation size is equal in both cases and code 
is safe. However, to prevent this CLANG WARNING will take this change, 
thanks.

>  		if (!ctx->uidx_table[tind].table)
>  			goto out;
>  	}
>

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH libmlx5 6/6] fix undefined uuar_index value assignment

[Jarod Wilson]

In the case of (attr->comp_mask & MLX5_CREATE_QP_EX2_COMP_MASK) being not
true, uuar_index gets set to resp.uuar_index, but nothing ever initializes
resp.uuar_index.

That said, both this case, and the true case, it looks like uuar_index
never gets assigned to anything but 0. In the true path, resp_ex gets
memset to 0, and then nothing ever sets uuar_index. Not sure what the
intended use was here, but ultimately, uuar_index is always going to be 0
with this patch (0 or undetermined garbage before).

Additionally, I'm not sure if the cmd and resp size parameters passed to
ibv_cmd_create_qp_ex() are correct, but they're at least larger than they
might be, which should be fine. I think. But I'm just guessing here.

CC: Yishai Hadas <yishaih-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
Signed-off-by: Jarod Wilson <jarod-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
 src/verbs.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/src/verbs.c b/src/verbs.c
index d64e406..e88253e 100644
--- a/src/verbs.c
+++ b/src/verbs.c
@@ -1235,12 +1235,14 @@ struct ibv_qp *create_qp(struct ibv_context *context,
 		cmd.uidx = usr_idx;
 	}
 
-	if (attr->comp_mask & MLX5_CREATE_QP_EX2_COMP_MASK)
+	if (attr->comp_mask & MLX5_CREATE_QP_EX2_COMP_MASK) {
 		ret = mlx5_cmd_create_qp_ex(context, attr, &cmd, qp, &resp_ex);
-	else
+	} else {
+		memset(&resp, 0, sizeof(resp));
 		ret = ibv_cmd_create_qp_ex(context, &qp->verbs_qp, sizeof(qp->verbs_qp),
 					   attr, &cmd.ibv_cmd, sizeof(cmd),
 					   &resp.ibv_resp, sizeof(resp));
+	}
 	if (ret) {
 		mlx5_dbg(fp, MLX5_DBG_QP, "ret %d\n", ret);
 		goto err_free_uidx;
-- 
1.8.3.1

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH libmlx5 6/6] fix undefined uuar_index value assignment

[Jarod Wilson]

On Wed, Jul 27, 2016 at 03:17:27PM -0400, Jarod Wilson wrote:
> In the case of (attr->comp_mask & MLX5_CREATE_QP_EX2_COMP_MASK) being not
> true, uuar_index gets set to resp.uuar_index, but nothing ever initializes
> resp.uuar_index.
> 
> That said, both this case, and the true case, it looks like uuar_index
> never gets assigned to anything but 0. In the true path, resp_ex gets
> memset to 0, and then nothing ever sets uuar_index. Not sure what the
> intended use was here, but ultimately, uuar_index is always going to be 0
> with this patch (0 or undetermined garbage before).
> 
> Additionally, I'm not sure if the cmd and resp size parameters passed to
> ibv_cmd_create_qp_ex() are correct, but they're at least larger than they
> might be, which should be fine. I think. But I'm just guessing here.

Coverity actually still complains about this version, largely because it
thinks that perhaps the comp_mask could change between checks, and thus
you could still get an uninitialized resp. I'm reworking this one a bit to
solve this in a way that is cleaner.

-- 
Jarod Wilson
jarod-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 libmlx5 6/6] fix undefined uuar_index value assignment

[Jarod Wilson]

In the case of (attr->comp_mask & MLX5_CREATE_QP_EX2_COMP_MASK) being not
true, uuar_index gets set to resp.uuar_index, but nothing ever initializes
resp.uuar_index.

That said, both this case, and the true case, it looks like uuar_index
never gets assigned to anything but 0. In the true path, resp_ex gets
memset to 0, and then nothing ever sets uuar_index. Not sure what the
intended use was here, but ultimately, uuar_index is always going to be 0
with this patch (0 or undetermined garbage before).

Additionally, I'm not sure if the cmd and resp size parameters passed to
ibv_cmd_create_qp_ex() are correct, but they're at least larger than they
might be, which should be fine. I think. But I'm just guessing here.

v2: only check flag once, save to local var, memset() resp and resp_ex
accordingly within this function.

CC: Yishai Hadas <yishaih-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
Signed-off-by: Jarod Wilson <jarod-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
 src/verbs.c | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

diff --git a/src/verbs.c b/src/verbs.c
index d64e406..c68864a 100644
--- a/src/verbs.c
+++ b/src/verbs.c
@@ -1098,7 +1098,6 @@ static int mlx5_cmd_create_qp_ex(struct ibv_context *context,
 	struct mlx5_create_qp_ex cmd_ex;
 	int ret;
 
-	memset(resp, 0, sizeof(*resp));
 	memset(&cmd_ex, 0, sizeof(cmd_ex));
 	memcpy(&cmd_ex.ibv_cmd.base, &cmd->ibv_cmd.user_handle,
 	       offsetof(typeof(cmd->ibv_cmd), is_srq) +
@@ -1140,6 +1139,7 @@ struct ibv_qp *create_qp(struct ibv_context *context,
 	struct ibv_qp		       *ibqp;
 	uint32_t			usr_idx = 0;
 	uint32_t			uuar_index;
+	uint8_t				use_ex2 = 0;
 #ifdef MLX5_DEBUG
 	FILE *fp = ctx->dbg_fp;
 #endif
@@ -1147,6 +1147,9 @@ struct ibv_qp *create_qp(struct ibv_context *context,
 	if (attr->comp_mask & ~MLX5_CREATE_QP_SUP_COMP_MASK)
 		return NULL;
 
+	if (attr->comp_mask & MLX5_CREATE_QP_EX2_COMP_MASK)
+		use_ex2 = 1;
+
 	qp = calloc(1, sizeof(*qp));
 	if (!qp) {
 		mlx5_dbg(fp, MLX5_DBG_QP, "\n");
@@ -1156,6 +1159,10 @@ struct ibv_qp *create_qp(struct ibv_context *context,
 	qp->ibv_qp = ibqp;
 
 	memset(&cmd, 0, sizeof(cmd));
+	if (use_ex2)
+		memset(&resp_ex, 0, sizeof(resp_ex));
+	else
+		memset(&resp, 0, sizeof(resp));
 
 	qp->wq_sig = qp_sig_enabled();
 	if (qp->wq_sig)
@@ -1235,7 +1242,7 @@ struct ibv_qp *create_qp(struct ibv_context *context,
 		cmd.uidx = usr_idx;
 	}
 
-	if (attr->comp_mask & MLX5_CREATE_QP_EX2_COMP_MASK)
+	if (use_ex2)
 		ret = mlx5_cmd_create_qp_ex(context, attr, &cmd, qp, &resp_ex);
 	else
 		ret = ibv_cmd_create_qp_ex(context, &qp->verbs_qp, sizeof(qp->verbs_qp),
@@ -1246,8 +1253,8 @@ struct ibv_qp *create_qp(struct ibv_context *context,
 		goto err_free_uidx;
 	}
 
-	uuar_index = (attr->comp_mask & MLX5_CREATE_QP_EX2_COMP_MASK) ?
-			resp_ex.uuar_index : resp.uuar_index;
+	uuar_index = use_ex2 ? resp_ex.uuar_index : resp.uuar_index;
+
 	if (!ctx->cqe_version) {
 		if (qp->sq.wqe_cnt || qp->rq.wqe_cnt) {
 			ret = mlx5_store_qp(ctx, ibqp->qp_num, qp);
-- 
1.8.3.1

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 libmlx5 6/6] fix undefined uuar_index value assignment

[Yishai Hadas]

On 7/28/2016 4:31 AM, Jarod Wilson wrote:
> In the case of (attr->comp_mask & MLX5_CREATE_QP_EX2_COMP_MASK) being not
> true, uuar_index gets set to resp.uuar_index, but nothing ever initializes
> resp.uuar_index.
>
> That said, both this case, and the true case, it looks like uuar_index
> never gets assigned to anything but 0. In the true path, resp_ex gets
> memset to 0, and then nothing ever sets uuar_index. Not sure what the
> intended use was here, but ultimately, uuar_index is always going to be 0
> with this patch (0 or undetermined garbage before).
>
> Additionally, I'm not sure if the cmd and resp size parameters passed to
> ibv_cmd_create_qp_ex() are correct, but they're at least larger than they
> might be, which should be fine. I think. But I'm just guessing here.

In both cases the data comes back from the kernel driver in the vendor 
channel path and uuar_index gets a real value. That's why 
ibv_cmd_create_qp_ex gets resp_size which is really larger than struct 
ibv_create_qp_resp which holds the output from the IB layer.

No change is needed here.

> v2: only check flag once, save to local var, memset() resp and resp_ex
> accordingly within this function.
>
> CC: Yishai Hadas <yishaih-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
> Signed-off-by: Jarod Wilson <jarod-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> ---
>  src/verbs.c | 15 +++++++++++----
>  1 file changed, 11 insertions(+), 4 deletions(-)
>
> diff --git a/src/verbs.c b/src/verbs.c
> index d64e406..c68864a 100644
> --- a/src/verbs.c
> +++ b/src/verbs.c
> @@ -1098,7 +1098,6 @@ static int mlx5_cmd_create_qp_ex(struct ibv_context *context,
>  	struct mlx5_create_qp_ex cmd_ex;
>  	int ret;
>
> -	memset(resp, 0, sizeof(*resp));
>  	memset(&cmd_ex, 0, sizeof(cmd_ex));
>  	memcpy(&cmd_ex.ibv_cmd.base, &cmd->ibv_cmd.user_handle,
>  	       offsetof(typeof(cmd->ibv_cmd), is_srq) +
> @@ -1140,6 +1139,7 @@ struct ibv_qp *create_qp(struct ibv_context *context,
>  	struct ibv_qp		       *ibqp;
>  	uint32_t			usr_idx = 0;
>  	uint32_t			uuar_index;
> +	uint8_t				use_ex2 = 0;
>  #ifdef MLX5_DEBUG
>  	FILE *fp = ctx->dbg_fp;
>  #endif
> @@ -1147,6 +1147,9 @@ struct ibv_qp *create_qp(struct ibv_context *context,
>  	if (attr->comp_mask & ~MLX5_CREATE_QP_SUP_COMP_MASK)
>  		return NULL;
>
> +	if (attr->comp_mask & MLX5_CREATE_QP_EX2_COMP_MASK)
> +		use_ex2 = 1;
> +
>  	qp = calloc(1, sizeof(*qp));
>  	if (!qp) {
>  		mlx5_dbg(fp, MLX5_DBG_QP, "\n");
> @@ -1156,6 +1159,10 @@ struct ibv_qp *create_qp(struct ibv_context *context,
>  	qp->ibv_qp = ibqp;
>
>  	memset(&cmd, 0, sizeof(cmd));
> +	if (use_ex2)
> +		memset(&resp_ex, 0, sizeof(resp_ex));
> +	else
> +		memset(&resp, 0, sizeof(resp));
>
>  	qp->wq_sig = qp_sig_enabled();
>  	if (qp->wq_sig)
> @@ -1235,7 +1242,7 @@ struct ibv_qp *create_qp(struct ibv_context *context,
>  		cmd.uidx = usr_idx;
>  	}
>
> -	if (attr->comp_mask & MLX5_CREATE_QP_EX2_COMP_MASK)
> +	if (use_ex2)
>  		ret = mlx5_cmd_create_qp_ex(context, attr, &cmd, qp, &resp_ex);
>  	else
>  		ret = ibv_cmd_create_qp_ex(context, &qp->verbs_qp, sizeof(qp->verbs_qp),
> @@ -1246,8 +1253,8 @@ struct ibv_qp *create_qp(struct ibv_context *context,
>  		goto err_free_uidx;
>  	}
>
> -	uuar_index = (attr->comp_mask & MLX5_CREATE_QP_EX2_COMP_MASK) ?
> -			resp_ex.uuar_index : resp.uuar_index;
> +	uuar_index = use_ex2 ? resp_ex.uuar_index : resp.uuar_index;
> +
>  	if (!ctx->cqe_version) {
>  		if (qp->sq.wqe_cnt || qp->rq.wqe_cnt) {
>  			ret = mlx5_store_qp(ctx, ibqp->qp_num, qp);
>

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 libmlx5 6/6] fix undefined uuar_index value assignment

[Jarod Wilson]

On Thu, Jul 28, 2016 at 06:53:36PM +0300, Yishai Hadas wrote:
> On 7/28/2016 4:31 AM, Jarod Wilson wrote:
> >In the case of (attr->comp_mask & MLX5_CREATE_QP_EX2_COMP_MASK) being not
> >true, uuar_index gets set to resp.uuar_index, but nothing ever initializes
> >resp.uuar_index.
> >
> >That said, both this case, and the true case, it looks like uuar_index
> >never gets assigned to anything but 0. In the true path, resp_ex gets
> >memset to 0, and then nothing ever sets uuar_index. Not sure what the
> >intended use was here, but ultimately, uuar_index is always going to be 0
> >with this patch (0 or undetermined garbage before).
> >
> >Additionally, I'm not sure if the cmd and resp size parameters passed to
> >ibv_cmd_create_qp_ex() are correct, but they're at least larger than they
> >might be, which should be fine. I think. But I'm just guessing here.
> 
> In both cases the data comes back from the kernel driver in the
> vendor channel path and uuar_index gets a real value. That's why
> ibv_cmd_create_qp_ex gets resp_size which is really larger than
> struct ibv_create_qp_resp which holds the output from the IB layer.
> 
> No change is needed here.

Hm. Well, coverity and clang dislike the current state, and this patch
does make them happy, but I suppose I can be persuaded to ignore them.

-- 
Jarod Wilson
jarod-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH libmlx5 7/6] combine inline_hdr and inline_hdr_start

[Jarod Wilson]

I can't see any good reason why inline_hdr and inline_hdr_start should be
two separate arrays in struct mlx5_wqe_eth_seg. The only time I see
anything actually accessed by dereferencing either inline_hdr or
inline_hdr_start is when the MLX5_ETH_L2_INLINE_HEADER_SIZE or less bytes
are copied into inline_hdr_start. By default, it's 18 bytes, copied to
what is a 2-byte and a 16-byte array back to back in the struct, and
coverity and clang both sound alarms because the code says "just write 18
bytes into that 2-byte array", which in practice is ultimately fine, but
again, why?...

I propose to just add two bytes to inline_hdr and drop inline_hdr_start.

CC: Yishai Hadas <yishaih-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
Signed-off-by: Jarod Wilson <jarod-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
 src/qp.c  | 4 ++--
 src/wqe.h | 3 +--
 2 files changed, 3 insertions(+), 4 deletions(-)

diff --git a/src/qp.c b/src/qp.c
index 8bb66be..f4a22be 100644
--- a/src/qp.c
+++ b/src/qp.c
@@ -369,14 +369,14 @@ static inline int copy_eth_inline_headers(struct ibv_qp *ibqp,
 
 	if (likely(wr->sg_list[0].length >= MLX5_ETH_L2_INLINE_HEADER_SIZE)) {
 		inl_hdr_copy_size = MLX5_ETH_L2_INLINE_HEADER_SIZE;
-		memcpy(eseg->inline_hdr_start,
+		memcpy(eseg->inline_hdr,
 		       (void *)(uintptr_t)wr->sg_list[0].addr,
 		       inl_hdr_copy_size);
 	} else {
 		for (j = 0; j < wr->num_sge && inl_hdr_size > 0; ++j) {
 			inl_hdr_copy_size = min(wr->sg_list[j].length,
 						inl_hdr_size);
-			memcpy(eseg->inline_hdr_start +
+			memcpy(eseg->inline_hdr +
 			       (MLX5_ETH_L2_INLINE_HEADER_SIZE - inl_hdr_size),
 			       (void *)(uintptr_t)wr->sg_list[j].addr,
 			       inl_hdr_copy_size);
diff --git a/src/wqe.h b/src/wqe.h
index c2622d5..d979232 100644
--- a/src/wqe.h
+++ b/src/wqe.h
@@ -92,8 +92,7 @@ struct mlx5_wqe_eth_seg {
 	uint16_t	mss;
 	uint32_t	rsvd2;
 	uint16_t	inline_hdr_sz;
-	uint8_t		inline_hdr_start[2];
-	uint8_t		inline_hdr[16];
+	uint8_t		inline_hdr[MLX5_ETH_L2_INLINE_HEADER_SIZE];
 };
 
 struct mlx5_wqe_ctrl_seg {
-- 
1.8.3.1

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH libmlx5 7/6] combine inline_hdr and inline_hdr_start

[Jarod Wilson]

On Wed, Jul 27, 2016 at 09:32:34PM -0400, Jarod Wilson wrote:
> I can't see any good reason why inline_hdr and inline_hdr_start should be
> two separate arrays in struct mlx5_wqe_eth_seg. The only time I see
> anything actually accessed by dereferencing either inline_hdr or
> inline_hdr_start is when the MLX5_ETH_L2_INLINE_HEADER_SIZE or less bytes
> are copied into inline_hdr_start. By default, it's 18 bytes, copied to
> what is a 2-byte and a 16-byte array back to back in the struct, and
> coverity and clang both sound alarms because the code says "just write 18
> bytes into that 2-byte array", which in practice is ultimately fine, but
> again, why?...
> 
> I propose to just add two bytes to inline_hdr and drop inline_hdr_start.

Heh, okay, so I see "Add TSO support for RAW Ethernet QP" posted today,
which does actually make use of those bits. Still not sure the split is
actually required though, that patch could probably be reworked to
operate on a single array as well.

-- 
Jarod Wilson
jarod-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH libmlx5 7/6] combine inline_hdr and inline_hdr_start

[Yishai Hadas]

On 7/28/2016 5:27 PM, Jarod Wilson wrote:
> On Wed, Jul 27, 2016 at 09:32:34PM -0400, Jarod Wilson wrote:
>> I can't see any good reason why inline_hdr and inline_hdr_start should be
>> two separate arrays in struct mlx5_wqe_eth_seg. The only time I see
>> anything actually accessed by dereferencing either inline_hdr or
>> inline_hdr_start is when the MLX5_ETH_L2_INLINE_HEADER_SIZE or less bytes
>> are copied into inline_hdr_start. By default, it's 18 bytes, copied to
>> what is a 2-byte and a 16-byte array back to back in the struct, and
>> coverity and clang both sound alarms because the code says "just write 18
>> bytes into that 2-byte array", which in practice is ultimately fine, but
>> again, why?...
>>
>> I propose to just add two bytes to inline_hdr and drop inline_hdr_start.
>
> Heh, okay, so I see "Add TSO support for RAW Ethernet QP" posted today,
> which does actually make use of those bits. Still not sure the split is
> actually required though, that patch could probably be reworked to
> operate on a single array as well.
>

The reason for that is that from HW/PRM point of view the 
mlx5_wqe_eth_seg structure defined to include only 2 bytes for the 
inline header start to be 16 bytes structure aligned with its previous 
fields. The extra 16 bytes field was added to ease the implementation 
for copying the extra inline header.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html