On Fri, Sep 27, 2019 at 02:28:41PM +0000, Saleem, Shiraz wrote:

quoted

Subject: Re: [RFC 12/20] RDMA/irdma: Implement device supported verb APIs

On Thu, Sep 26, 2019 at 07:49:52PM +0000, Saleem, Shiraz wrote:

quoted

quoted

Subject: Re: [RFC 12/20] RDMA/irdma: Implement device supported verb
APIs

On Thu, Sep 26, 2019 at 09:45:11AM -0700, Jeff Kirsher wrote:

quoted

From: Mustafa Ismail <redacted>

Implement device supported verb APIs. The supported APIs vary
based on the underlying transport the ibdev is registered as (i.e.
iWARP or RoCEv2).

Signed-off-by: Mustafa Ismail <redacted>
Signed-off-by: Shiraz Saleem <redacted>
---
 drivers/infiniband/hw/irdma/verbs.c      | 4346 ++++++++++++++++++++++
 drivers/infiniband/hw/irdma/verbs.h      |  199 +
 include/uapi/rdma/rdma_user_ioctl_cmds.h |    1 +
 3 files changed, 4546 insertions(+)  create mode 100644
drivers/infiniband/hw/irdma/verbs.c
 create mode 100644 drivers/infiniband/hw/irdma/verbs.h

diff --git a/drivers/infiniband/hw/irdma/verbs.c

b/drivers/infiniband/hw/irdma/verbs.c
new file mode 100644
index 000000000000..025c21c722e2

--- /dev/null
+++ b/drivers/infiniband/hw/irdma/verbs.c

@@ -0,0 +1,4346 @@
+// SPDX-License-Identifier: GPL-2.0 or Linux-OpenIB
+/* Copyright (c) 2019, Intel Corporation. */

<...>

quoted

+
+	size = sqdepth * sizeof(struct irdma_sq_uk_wr_trk_info) +
+	       (rqdepth << 3);
+	iwqp->kqp.wrid_mem = kzalloc(size, GFP_KERNEL);
+	if (!iwqp->kqp.wrid_mem)
+		return -ENOMEM;
+
+	ukinfo->sq_wrtrk_array = (struct irdma_sq_uk_wr_trk_info *)
+				 iwqp->kqp.wrid_mem;
+	if (!ukinfo->sq_wrtrk_array)
+		return -ENOMEM;

You are leaking resources here, forgot to do proper error unwinding.

irdma_free_qp_rsrc() will free up that memory in case of an error.

I'm talking about kqp.wrid_mem you allocated a couple of lines above and didn't
free in case of sq_wrtrk_array allocation failed.

Yes, I am referring to kqp.wrid_mem as well
In case of err, all memory resources setup for
the QP is freed in the common utility irdma_free_qp_rsrc()
including the kqp.wrid_mem.

I see it as an anti-pattern, you have function to setup and it shouldn't
return half initialized state and rely on some other function to clean
the mess.

Current code is written in a way that makes very hard to check for
unwinding errors.

Thanks