[PATCH v1 rdma-core 0/2] libbnxt_re bug fixes

[PATCH v1 rdma-core 0/2] libbnxt_re bug fixes

[Devesh Sharma]

This patch series contains important bug fixes.
Patch 1 address the memory barrier changes Jason
asked while reviewing a similar change in bnxt_re
driver.

Patch 2 is another fix found during internal testing.

Devesh Sharma (2):
  bnxt_re/lib: fix the memory barrier call during poll-cq
  bnxt_re/lib:increment psn in case of 0 length packets

 providers/bnxt_re/main.h  | 8 ++++++--
 providers/bnxt_re/verbs.c | 2 ++
 2 files changed, 8 insertions(+), 2 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 v1 rdma-core 1/2] bnxt_re/lib: fix the memory barrier call during poll-cq

[Devesh Sharma]

Putting a read barrier before issuing a read on valid bit
is incorrect. When checking for the validity of CQE in the
CQ buffer the code must wait for the read-barrier to finish
after issuing a read operation on CQE valid bit.

Signed-off-by: Devesh Sharma <devesh.sharma-dY08KVG/lbpWk0Htik3J/w@public.gmane.org>
---
 providers/bnxt_re/main.h | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/providers/bnxt_re/main.h b/providers/bnxt_re/main.h
index 9688fec..82c8948 100644
--- a/providers/bnxt_re/main.h
+++ b/providers/bnxt_re/main.h
@@ -366,9 +366,13 @@ static inline uint8_t bnxt_re_to_ibv_wc_status(uint8_t bnxt_wcst,
 static inline uint8_t bnxt_re_is_cqe_valid(struct bnxt_re_cq *cq,
 					   struct bnxt_re_bcqe *hdr)
 {
+	uint8_t valid = 0;
+
+	valid = ((le32toh(hdr->flg_st_typ_ph) &
+		  BNXT_RE_BCQE_PH_MASK) == cq->phase);
 	udma_from_device_barrier();
-	return ((le32toh(hdr->flg_st_typ_ph) &
-		 BNXT_RE_BCQE_PH_MASK) == cq->phase);
+
+	return valid;
 }
 
 static inline void bnxt_re_change_cq_phase(struct bnxt_re_cq *cq)
-- 
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 v1 rdma-core 1/2] bnxt_re/lib: fix the memory barrier call during poll-cq

[Jason Gunthorpe]

On Thu, Nov 09, 2017 at 06:10:39AM -0500, Devesh Sharma wrote:
> Putting a read barrier before issuing a read on valid bit
> is incorrect. When checking for the validity of CQE in the
> CQ buffer the code must wait for the read-barrier to finish
> after issuing a read operation on CQE valid bit.
> 
> Signed-off-by: Devesh Sharma <devesh.sharma-dY08KVG/lbpWk0Htik3J/w@public.gmane.org>
>  providers/bnxt_re/main.h | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)

This is probably fine

> +	uint8_t valid = 0;
> +
> +	valid = ((le32toh(hdr->flg_st_typ_ph) &
> +		  BNXT_RE_BCQE_PH_MASK) == cq->phase);

Hrm,

Techincally this should be something like

le32toh(atomic_read(hdr->flg_st_typ_ph))

And the the kernel version of this should be using READ_ONCE()

In user space we should probably create a

 udma_from_device_read_once(x)

That incorporates the barrier and the 'access_once' semantics..

Jason
--
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 v1 rdma-core 1/2] bnxt_re/lib: fix the memory barrier call during poll-cq

[Devesh Sharma]

On Thu, Nov 9, 2017 at 11:35 PM, Jason Gunthorpe <jgg-uk2M96/98Pc@public.gmane.org> wrote:
> On Thu, Nov 09, 2017 at 06:10:39AM -0500, Devesh Sharma wrote:
>> Putting a read barrier before issuing a read on valid bit
>> is incorrect. When checking for the validity of CQE in the
>> CQ buffer the code must wait for the read-barrier to finish
>> after issuing a read operation on CQE valid bit.
>>
>> Signed-off-by: Devesh Sharma <devesh.sharma-dY08KVG/lbpWk0Htik3J/w@public.gmane.org>
>>  providers/bnxt_re/main.h | 8 ++++++--
>>  1 file changed, 6 insertions(+), 2 deletions(-)
>
> This is probably fine
>
>> +     uint8_t valid = 0;
>> +
>> +     valid = ((le32toh(hdr->flg_st_typ_ph) &
>> +               BNXT_RE_BCQE_PH_MASK) == cq->phase);
>
> Hrm,
>
> Techincally this should be something like
>
> le32toh(atomic_read(hdr->flg_st_typ_ph))
>
> And the the kernel version of this should be using READ_ONCE()

Okay, we will fix this in our driver. So, READ_ONCE(CQE.valid) should
be enough right?
>
> In user space we should probably create a
>
>  udma_from_device_read_once(x)
>
> That incorporates the barrier and the 'access_once' semantics..

Do you want us to pull all the defined in kernel space compiler.h
verbitum to rdma-core
I think there would be few changes correct..

>
> Jason
--
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 v1 rdma-core 1/2] bnxt_re/lib: fix the memory barrier call during poll-cq

[Jason Gunthorpe]

On Fri, Nov 10, 2017 at 05:01:42PM +0530, Devesh Sharma wrote:

> > And the the kernel version of this should be using READ_ONCE()
> 
> Okay, we will fix this in our driver. So, READ_ONCE(CQE.valid) should
> be enough right?

Yes, I think so.

> > In user space we should probably create a
> >
> >  udma_from_device_read_once(x)
> >
> > That incorporates the barrier and the 'access_once' semantics..
> 
> Do you want us to pull all the defined in kernel space compiler.h
> verbitum to rdma-core

No.. kernel is using a quite different approach. We would want to use
C11 atomic_read in user space for most architectures.

Jason
--
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 v1 rdma-core 2/2] bnxt_re/lib: increment psn in case of 0 length packets

[Devesh Sharma]

If application posts a 0 length packet, post send routine
is skipping to increment the psn number. This will cause
PSN number to go out of sync and eventually connection would
terminate due to sequence error.

post_send routine must increment the psn number by 1 even
for zero length packets.

Signed-off-by: Devesh Sharma <devesh.sharma-dY08KVG/lbpWk0Htik3J/w@public.gmane.org>
---
 providers/bnxt_re/verbs.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/providers/bnxt_re/verbs.c b/providers/bnxt_re/verbs.c
index 4d9b044..9d4e02b 100644
--- a/providers/bnxt_re/verbs.c
+++ b/providers/bnxt_re/verbs.c
@@ -1048,6 +1048,8 @@ static void bnxt_re_fill_psns(struct bnxt_re_qp *qp, struct bnxt_re_psns *psns,
 		pkt_cnt = (len / qp->mtu);
 		if (len % qp->mtu)
 			pkt_cnt++;
+		if (len == 0)
+			pkt_cnt = 1;
 		nxt_psn = ((qp->sq_psn + pkt_cnt) & BNXT_RE_PSNS_NPSN_MASK);
 		psns->flg_npsn = htole32(nxt_psn);
 		qp->sq_psn = nxt_psn;
-- 
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 v1 rdma-core 0/2] libbnxt_re bug fixes

[Leon Romanovsky]

[-- Attachment #1: Type: text/plain, Size: 577 bytes --]

On Thu, Nov 09, 2017 at 06:10:38AM -0500, Devesh Sharma wrote:
> This patch series contains important bug fixes.
> Patch 1 address the memory barrier changes Jason
> asked while reviewing a similar change in bnxt_re
> driver.
>
> Patch 2 is another fix found during internal testing.
>
> Devesh Sharma (2):
>   bnxt_re/lib: fix the memory barrier call during poll-cq
>   bnxt_re/lib:increment psn in case of 0 length packets
>
>  providers/bnxt_re/main.h  | 8 ++++++--
>  providers/bnxt_re/verbs.c | 2 ++
>  2 files changed, 8 insertions(+), 2 deletions(-)
>

Thanks, applied.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]