diff mbox series

[for-next,v2,1/3] RDMA/bnxt_re: Add more flags in device init and uninit path

Message ID 1582541395-19409-2-git-send-email-selvin.xavier@broadcom.com (mailing list archive)
State Superseded
Headers show
Series RDMA/bnxt_re driver update | expand

Commit Message

Selvin Xavier Feb. 24, 2020, 10:49 a.m. UTC 
Add more flags for better granularity in in device init/uninit path

Signed-off-by: Selvin Xavier <selvin.xavier@broadcom.com>
---
 drivers/infiniband/hw/bnxt_re/bnxt_re.h | 16 +++++++++----
 drivers/infiniband/hw/bnxt_re/main.c    | 41 ++++++++++++++++-----------------
 2 files changed, 31 insertions(+), 26 deletions(-)

Comments

Jason Gunthorpe Feb. 24, 2020, 1:48 p.m. UTC | #1 
On Mon, Feb 24, 2020 at 02:49:53AM -0800, Selvin Xavier wrote:
> Add more flags for better granularity in in device init/uninit path
> 
> Signed-off-by: Selvin Xavier <selvin.xavier@broadcom.com>
>  drivers/infiniband/hw/bnxt_re/bnxt_re.h | 16 +++++++++----
>  drivers/infiniband/hw/bnxt_re/main.c    | 41 ++++++++++++++++-----------------
>  2 files changed, 31 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/infiniband/hw/bnxt_re/bnxt_re.h b/drivers/infiniband/hw/bnxt_re/bnxt_re.h
> index c736e82..0babc66 100644
> +++ b/drivers/infiniband/hw/bnxt_re/bnxt_re.h
> @@ -135,11 +135,17 @@ struct bnxt_re_dev {
>  #define BNXT_RE_FLAG_IBDEV_REGISTERED		1
>  #define BNXT_RE_FLAG_GOT_MSIX			2
>  #define BNXT_RE_FLAG_HAVE_L2_REF		3
> -#define BNXT_RE_FLAG_RCFW_CHANNEL_EN		4
> -#define BNXT_RE_FLAG_QOS_WORK_REG		5
> -#define BNXT_RE_FLAG_RESOURCES_ALLOCATED	7
> -#define BNXT_RE_FLAG_RESOURCES_INITIALIZED	8
> -#define BNXT_RE_FLAG_ISSUE_ROCE_STATS          29
> +#define BNXT_RE_FLAG_ALLOC_RCFW			4
> +#define BNXT_RE_FLAG_NET_RING_ALLOC		5
> +#define BNXT_RE_FLAG_RCFW_CHANNEL_EN		6
> +#define BNXT_RE_FLAG_ALLOC_CTX			7
> +#define BNXT_RE_FLAG_STATS_CTX_ALLOC		8
> +#define BNXT_RE_FLAG_STATS_CTX2_ALLOC		9
> +#define BNXT_RE_FLAG_RCFW_CHANNEL_INIT		10
> +#define BNXT_RE_FLAG_QOS_WORK_REG		11
> +#define BNXT_RE_FLAG_RESOURCES_ALLOCATED	12
> +#define BNXT_RE_FLAG_RESOURCES_INITIALIZED	13
> +#define BNXT_RE_FLAG_ISSUE_ROCE_STATS		29

The error unwind is better than adding these endless flags.

It is hard to understand the flow with all this needless conditional,
really if you want to change this I'd delete the test_bit scheme and
stick to the normal goto error unwind.

Jason
Selvin Xavier Feb. 25, 2020, 10:14 a.m. UTC | #2 
On Mon, Feb 24, 2020 at 7:18 PM Jason Gunthorpe <jgg@mellanox.com> wrote:
>
> On Mon, Feb 24, 2020 at 02:49:53AM -0800, Selvin Xavier wrote:
> > Add more flags for better granularity in in device init/uninit path
> >
> > Signed-off-by: Selvin Xavier <selvin.xavier@broadcom.com>
> >  drivers/infiniband/hw/bnxt_re/bnxt_re.h | 16 +++++++++----
> >  drivers/infiniband/hw/bnxt_re/main.c    | 41 ++++++++++++++++-----------------
> >  2 files changed, 31 insertions(+), 26 deletions(-)
> >
> > diff --git a/drivers/infiniband/hw/bnxt_re/bnxt_re.h b/drivers/infiniband/hw/bnxt_re/bnxt_re.h
> > index c736e82..0babc66 100644
> > +++ b/drivers/infiniband/hw/bnxt_re/bnxt_re.h
> > @@ -135,11 +135,17 @@ struct bnxt_re_dev {
> >  #define BNXT_RE_FLAG_IBDEV_REGISTERED                1
> >  #define BNXT_RE_FLAG_GOT_MSIX                        2
> >  #define BNXT_RE_FLAG_HAVE_L2_REF             3
> > -#define BNXT_RE_FLAG_RCFW_CHANNEL_EN         4
> > -#define BNXT_RE_FLAG_QOS_WORK_REG            5
> > -#define BNXT_RE_FLAG_RESOURCES_ALLOCATED     7
> > -#define BNXT_RE_FLAG_RESOURCES_INITIALIZED   8
> > -#define BNXT_RE_FLAG_ISSUE_ROCE_STATS          29
> > +#define BNXT_RE_FLAG_ALLOC_RCFW                      4
> > +#define BNXT_RE_FLAG_NET_RING_ALLOC          5
> > +#define BNXT_RE_FLAG_RCFW_CHANNEL_EN         6
> > +#define BNXT_RE_FLAG_ALLOC_CTX                       7
> > +#define BNXT_RE_FLAG_STATS_CTX_ALLOC         8
> > +#define BNXT_RE_FLAG_STATS_CTX2_ALLOC                9
> > +#define BNXT_RE_FLAG_RCFW_CHANNEL_INIT               10
> > +#define BNXT_RE_FLAG_QOS_WORK_REG            11
> > +#define BNXT_RE_FLAG_RESOURCES_ALLOCATED     12
> > +#define BNXT_RE_FLAG_RESOURCES_INITIALIZED   13
> > +#define BNXT_RE_FLAG_ISSUE_ROCE_STATS                29
>
> The error unwind is better than adding these endless flags.
>
> It is hard to understand the flow with all this needless conditional,
> really if you want to change this I'd delete the test_bit scheme and
> stick to the normal goto error unwind.
The existing code was doing few  error unwinding. I thought it would
be simple to collect everything under bnxt_re_ib_unreg function and
cleanup with these flags. I am ok with both the approaches. Maybe,
I can drop this patch and post the remaining series.

>
> Jason
diff mbox series

Patch

diff --git a/drivers/infiniband/hw/bnxt_re/bnxt_re.h b/drivers/infiniband/hw/bnxt_re/bnxt_re.h
index c736e82..0babc66 100644
--- a/drivers/infiniband/hw/bnxt_re/bnxt_re.h
+++ b/drivers/infiniband/hw/bnxt_re/bnxt_re.h
@@ -135,11 +135,17 @@  struct bnxt_re_dev {
 #define BNXT_RE_FLAG_IBDEV_REGISTERED		1
 #define BNXT_RE_FLAG_GOT_MSIX			2
 #define BNXT_RE_FLAG_HAVE_L2_REF		3
-#define BNXT_RE_FLAG_RCFW_CHANNEL_EN		4
-#define BNXT_RE_FLAG_QOS_WORK_REG		5
-#define BNXT_RE_FLAG_RESOURCES_ALLOCATED	7
-#define BNXT_RE_FLAG_RESOURCES_INITIALIZED	8
-#define BNXT_RE_FLAG_ISSUE_ROCE_STATS          29
+#define BNXT_RE_FLAG_ALLOC_RCFW			4
+#define BNXT_RE_FLAG_NET_RING_ALLOC		5
+#define BNXT_RE_FLAG_RCFW_CHANNEL_EN		6
+#define BNXT_RE_FLAG_ALLOC_CTX			7
+#define BNXT_RE_FLAG_STATS_CTX_ALLOC		8
+#define BNXT_RE_FLAG_STATS_CTX2_ALLOC		9
+#define BNXT_RE_FLAG_RCFW_CHANNEL_INIT		10
+#define BNXT_RE_FLAG_QOS_WORK_REG		11
+#define BNXT_RE_FLAG_RESOURCES_ALLOCATED	12
+#define BNXT_RE_FLAG_RESOURCES_INITIALIZED	13
+#define BNXT_RE_FLAG_ISSUE_ROCE_STATS		29
 	struct net_device		*netdev;
 	unsigned int			version, major, minor;
 	struct bnxt_qplib_chip_ctx	*chip_ctx;
diff --git a/drivers/infiniband/hw/bnxt_re/main.c b/drivers/infiniband/hw/bnxt_re/main.c
index b5128cc..1402130 100644
--- a/drivers/infiniband/hw/bnxt_re/main.c
+++ b/drivers/infiniband/hw/bnxt_re/main.c
@@ -1335,18 +1335,23 @@  static void bnxt_re_ib_unreg(struct bnxt_re_dev *rdev)
 	if (test_and_clear_bit(BNXT_RE_FLAG_RESOURCES_ALLOCATED, &rdev->flags))
 		bnxt_re_free_res(rdev);
 
-	if (test_and_clear_bit(BNXT_RE_FLAG_RCFW_CHANNEL_EN, &rdev->flags)) {
+	if (test_and_clear_bit(BNXT_RE_FLAG_RCFW_CHANNEL_INIT, &rdev->flags)) {
 		rc = bnxt_qplib_deinit_rcfw(&rdev->rcfw);
 		if (rc)
 			ibdev_warn(&rdev->ibdev,
 				   "Failed to deinitialize RCFW: %#x", rc);
+	}
+	if (test_and_clear_bit(BNXT_RE_FLAG_STATS_CTX_ALLOC, &rdev->flags))
 		bnxt_re_net_stats_ctx_free(rdev, rdev->qplib_ctx.stats.fw_id);
+	if (test_and_clear_bit(BNXT_RE_FLAG_ALLOC_CTX, &rdev->flags))
 		bnxt_qplib_free_ctx(&rdev->qplib_res, &rdev->qplib_ctx);
+	if (test_and_clear_bit(BNXT_RE_FLAG_RCFW_CHANNEL_EN, &rdev->flags))
 		bnxt_qplib_disable_rcfw_channel(&rdev->rcfw);
-		type = bnxt_qplib_get_ring_type(rdev->chip_ctx);
+	type = bnxt_qplib_get_ring_type(rdev->chip_ctx);
+	if (test_and_clear_bit(BNXT_RE_FLAG_NET_RING_ALLOC, &rdev->flags))
 		bnxt_re_net_ring_free(rdev, rdev->rcfw.creq.ring_id, type);
+	if (test_and_clear_bit(BNXT_RE_FLAG_ALLOC_RCFW, &rdev->flags))
 		bnxt_qplib_free_rcfw_channel(&rdev->rcfw);
-	}
 	if (test_and_clear_bit(BNXT_RE_FLAG_GOT_MSIX, &rdev->flags)) {
 		rc = bnxt_re_free_msix(rdev);
 		if (rc)
@@ -1430,6 +1435,7 @@  static int bnxt_re_ib_reg(struct bnxt_re_dev *rdev)
 		goto fail;
 	}
 
+	set_bit(BNXT_RE_FLAG_ALLOC_RCFW, &rdev->flags);
 	type = bnxt_qplib_get_ring_type(rdev->chip_ctx);
 	creq = &rdev->rcfw.creq;
 	rattr.dma_arr = creq->hwq.pbl[PBL_LVL_0].pg_map_arr;
@@ -1441,8 +1447,9 @@  static int bnxt_re_ib_reg(struct bnxt_re_dev *rdev)
 	rc = bnxt_re_net_ring_alloc(rdev, &rattr, &creq->ring_id);
 	if (rc) {
 		ibdev_err(&rdev->ibdev, "Failed to allocate CREQ: %#x\n", rc);
-		goto free_rcfw;
+		goto fail;
 	}
+	set_bit(BNXT_RE_FLAG_NET_RING_ALLOC, &rdev->flags);
 	db_offt = bnxt_re_get_nqdb_offset(rdev, BNXT_RE_AEQ_IDX);
 	vid = rdev->msix_entries[BNXT_RE_AEQ_IDX].vector;
 	rc = bnxt_qplib_enable_rcfw_channel(&rdev->rcfw,
@@ -1451,13 +1458,14 @@  static int bnxt_re_ib_reg(struct bnxt_re_dev *rdev)
 	if (rc) {
 		ibdev_err(&rdev->ibdev, "Failed to enable RCFW channel: %#x\n",
 			  rc);
-		goto free_ring;
+		goto fail;
 	}
 
+	set_bit(BNXT_RE_FLAG_RCFW_CHANNEL_EN, &rdev->flags);
 	rc = bnxt_qplib_get_dev_attr(&rdev->rcfw, &rdev->dev_attr,
 				     rdev->is_virtfn);
 	if (rc)
-		goto disable_rcfw;
+		goto fail;
 
 	bnxt_re_set_resource_limits(rdev);
 
@@ -1466,25 +1474,27 @@  static int bnxt_re_ib_reg(struct bnxt_re_dev *rdev)
 	if (rc) {
 		ibdev_err(&rdev->ibdev,
 			  "Failed to allocate QPLIB context: %#x\n", rc);
-		goto disable_rcfw;
+		goto fail;
 	}
+	set_bit(BNXT_RE_FLAG_ALLOC_CTX, &rdev->flags);
 	rc = bnxt_re_net_stats_ctx_alloc(rdev,
 					 rdev->qplib_ctx.stats.dma_map,
 					 &rdev->qplib_ctx.stats.fw_id);
 	if (rc) {
 		ibdev_err(&rdev->ibdev,
 			  "Failed to allocate stats context: %#x\n", rc);
-		goto free_ctx;
+		goto fail;
 	}
 
+	set_bit(BNXT_RE_FLAG_STATS_CTX_ALLOC, &rdev->flags);
 	rc = bnxt_qplib_init_rcfw(&rdev->rcfw, &rdev->qplib_ctx,
 				  rdev->is_virtfn);
 	if (rc) {
 		ibdev_err(&rdev->ibdev,
 			  "Failed to initialize RCFW: %#x\n", rc);
-		goto free_sctx;
+		goto fail;
 	}
-	set_bit(BNXT_RE_FLAG_RCFW_CHANNEL_EN, &rdev->flags);
+	set_bit(BNXT_RE_FLAG_RCFW_CHANNEL_INIT, &rdev->flags);
 
 	/* Resources based on the 'new' device caps */
 	rc = bnxt_re_alloc_res(rdev);
@@ -1532,17 +1542,6 @@  static int bnxt_re_ib_reg(struct bnxt_re_dev *rdev)
 	bnxt_re_dispatch_event(&rdev->ibdev, NULL, 1, IB_EVENT_PORT_ACTIVE);
 
 	return 0;
-free_sctx:
-	bnxt_re_net_stats_ctx_free(rdev, rdev->qplib_ctx.stats.fw_id);
-free_ctx:
-	bnxt_qplib_free_ctx(&rdev->qplib_res, &rdev->qplib_ctx);
-disable_rcfw:
-	bnxt_qplib_disable_rcfw_channel(&rdev->rcfw);
-free_ring:
-	type = bnxt_qplib_get_ring_type(rdev->chip_ctx);
-	bnxt_re_net_ring_free(rdev, rdev->rcfw.creq.ring_id, type);
-free_rcfw:
-	bnxt_qplib_free_rcfw_channel(&rdev->rcfw);
 fail:
 	if (!locked)
 		rtnl_lock();