Skip to contentSkip to conversation

RDMA/irdma: Fix typo in SQ completions generation

PATCH Applied Cyrill Gorcunov 4 participants 10 messages May 28 – Jun 4

Subsystems (2)
lore
Share & tools
lore
#0 lore raw

PATCH 1/1 RDMA/irdma: Fix typo in SQ completions generation

When we generate completion for SQ the opcode while being properly read from ring buffer is ignored when written back to completion. Seems to be a simple typo.

---
Hopefully I didn't miss something obvious here, found it while been fighting with unrelated issue.

 drivers/infiniband/hw/irdma/utils.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/infiniband/hw/irdma/utils.c +1 -1
Index: linux-tip.git/drivers/infiniband/hw/irdma/utils.c
===================================================================
@@ -2442,7 +2442,7 @@ void irdma_generate_flush_completions(st
 			cmpl->cpi.wr_id = qp->sq_wrtrk_array[wqe_idx].wrid;
 			sw_wqe = qp->sq_base[wqe_idx].elem;
 			get_64bit_val(sw_wqe, 24, &wqe_qword);
-			cmpl->cpi.op_type = (u8)FIELD_GET(IRDMAQPSQ_OPCODE, IRDMAQPSQ_OPCODE);
+			cmpl->cpi.op_type = (u8)FIELD_GET(IRDMAQPSQ_OPCODE, wqe_qword);
 			cmpl->cpi.q_type = IRDMA_CQE_QTYPE_SQ;
 			/* remove the SQ WR by moving SQ tail*/
 			IRDMA_RING_SET_TAIL(*sq_ring,
Reviewed-by on v1 1/1
#1 lore raw
Quoted 2 lines

Hopefully I didn't miss something obvious here, found it while been fighting with unrelated issue.

Nice find. I took a look and your fix seems valid to me.

I guess prior to this fix, it could potentially generate flush completions for the NOP/pad WQEs which I can see being a problem since the WR ID would be totally bogus.

#2 lore raw

Thanks a lot for the review, Jacob! I miss to add that the nit came in with commit 81091d7696ae71627ff80bbf2c6b0986d2c1cce3,
which is 5.18 series, so I guess we might need to fetch it to stable tree later.

#6 lore raw

Sorry, didn't mean to open up a can of worms. I personally would like to move in the other direction - let's get all of these fixes upstream :)

Quoted 2 lines

i'm loosing completions on posted operations when
hardware in reset mode

I think this has come up before. For example, I vaguely recall an async VF reset during heavy RDMA CM activity resulting in some timeout firing (cm_destroy_id_wait_timeout), which I think was due to completions getting lost.

You may be on to something regarding flushing that WQ. I'll give it a try on my end.

- Jake

Quoted 2 lines

At
moment I suspect that we need to *flush* queue here instead of cancelling it

#7 lore raw
In reply to Jacob Moroni
Quoted 3 lines · Jacob Moroni

BTW, your fix matches the OOT driver code:
https://github.com/intel/ethernet-linux-irdma-and-idpf/blob/main/rdma-driver/src/irdma/utils.c#L3561

Wow, didn't know about OOT driver :-) Thanks for pointing!

Quoted 2 lines

Regarding the "unrelated issue" - is it an irdma issue? Anything we can help with?

Well, the issue I'm dealing with is messy yet (i mean i'm not sure if it is irdma issue or not -- i'm loosing completions on posted operations when hardware in reset mode and same time the card is physically removed). Look, once I manage to collect all pieces of a problem I'll back with report ) At moment I suspect that we need to *flush* queue here instead of cancelling it

---
static int irdma_destroy_qp(struct ib_qp *ibqp, struct ib_udata *udata)

{
	struct irdma_qp *iwqp = to_iwqp(ibqp);
	struct irdma_device *iwdev = iwqp->iwdev;

	iwqp->sc_qp.qp_uk.destroy_pending = true;

	if (iwqp->iwarp_state == IRDMA_QP_STATE_RTS)
		irdma_modify_qp_to_err(&iwqp->sc_qp);

	if (!iwqp->user_mode)

--> cancel_delayed_work_sync(&iwqp->dwork_flush);

maintainer Applied
#8 lore raw
In reply to Cyrill Gorcunov
Quoted 9 lines · Cyrill Gorcunov

When we generate completion for SQ the opcode while being properly read from ring buffer is ignored when written back to completion. Seems to be a simple typo.

---
Hopefully I didn't miss something obvious here, found it while been fighting with unrelated issue.

Applied to for-next, thanks

Jason

#9 lore raw
Quoted 5 lines · Jason Gunthorpe
Quoted 4 lines · Cyrill Gorcunov

When we generate completion for SQ the opcode while being properly read from ring buffer is ignored when written back to completion. Seems to be a simple typo.

Quoted 7 lines
Quoted 5 lines

---
Hopefully I didn't miss something obvious here, found it while been fighting with unrelated issue.

Applied to for-next, thanks

Thanks a huge, Jason! What about the series https://lore.kernel.org/netdev/[email protected]/ ? Guys, could you take a glance please?

Cyrill