[PATCH v2 for-next 0/3] RDMA/bnxt_re: Fixes for handling device references

[PATCH v2 for-next 0/3] RDMA/bnxt_re: Fixes for handling device references

[Selvin Xavier]

Avoid the driver's internal mechanisms to check
for device registered state and counting the work queue
scheduling.

v1-> v2:
  - Removed all reference of BNXT_RE_FLAG_IBDEV_REGISTERED
    from patch 1
  - Removed smp_mb__before_atomic call from patch 3

Jason Gunthorpe (2):
  RDMA/bnxt_re: Use ib_device_try_get()
  RDMA/bnxt_re: Fix lifetimes in bnxt_re_task

Selvin Xavier (1):
  RDMA/bnxt_re: Remove unnecessary sched count

 drivers/infiniband/hw/bnxt_re/bnxt_re.h |  2 --
 drivers/infiniband/hw/bnxt_re/main.c    | 37 ++++++++++++++-------------------
 2 files changed, 16 insertions(+), 23 deletions(-)

-- 
2.5.5

[PATCH v2 for-next 1/3] RDMA/bnxt_re: Use ib_device_try_get()

[Selvin Xavier]

From: Jason Gunthorpe <jgg@mellanox.com>

There are a couple places in this driver running from a work queue that
need the ib_device to be registered. Instead of using a broken internal
bit rely on the new core code to guarantee device registration.

Signed-off-by: Jason Gunthorpe <jgg@mellanox.com>
Signed-off-by: Selvin Xavier <selvin.xavier@broadcom.com>
---
 drivers/infiniband/hw/bnxt_re/bnxt_re.h |  1 -
 drivers/infiniband/hw/bnxt_re/main.c    | 27 ++++++++++++++-------------
 2 files changed, 14 insertions(+), 14 deletions(-)

diff --git a/drivers/infiniband/hw/bnxt_re/bnxt_re.h b/drivers/infiniband/hw/bnxt_re/bnxt_re.h
index c736e82..407141e 100644
--- a/drivers/infiniband/hw/bnxt_re/bnxt_re.h
+++ b/drivers/infiniband/hw/bnxt_re/bnxt_re.h
@@ -132,7 +132,6 @@ struct bnxt_re_dev {
 	struct list_head		list;
 	unsigned long			flags;
 #define BNXT_RE_FLAG_NETDEV_REGISTERED		0
-#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
diff --git a/drivers/infiniband/hw/bnxt_re/main.c b/drivers/infiniband/hw/bnxt_re/main.c
index 415693f..885127c 100644
--- a/drivers/infiniband/hw/bnxt_re/main.c
+++ b/drivers/infiniband/hw/bnxt_re/main.c
@@ -1171,12 +1171,13 @@ static int bnxt_re_update_gid(struct bnxt_re_dev *rdev)
 	u16 gid_idx, index;
 	int rc = 0;
 
-	if (!test_bit(BNXT_RE_FLAG_IBDEV_REGISTERED, &rdev->flags))
+	if (!ib_device_try_get(&rdev->ibdev))
 		return 0;
 
 	if (!sgid_tbl) {
 		ibdev_err(&rdev->ibdev, "QPLIB: SGID table not allocated");
-		return -EINVAL;
+		rc = -EINVAL;
+		goto out;
 	}
 
 	for (index = 0; index < sgid_tbl->active; index++) {
@@ -1196,7 +1197,8 @@ static int bnxt_re_update_gid(struct bnxt_re_dev *rdev)
 		rc = bnxt_qplib_update_sgid(sgid_tbl, &gid, gid_idx,
 					    rdev->qplib_res.netdev->dev_addr);
 	}
-
+out:
+	ib_device_put(&rdev->ibdev);
 	return rc;
 }
 
@@ -1319,7 +1321,6 @@ int bnxt_re_ib_init(struct bnxt_re_dev *rdev)
 		pr_err("Failed to register with IB: %#x\n", rc);
 		return rc;
 	}
-	set_bit(BNXT_RE_FLAG_IBDEV_REGISTERED, &rdev->flags);
 	dev_info(rdev_to_dev(rdev), "Device registered successfully");
 	ib_get_eth_speed(&rdev->ibdev, 1, &rdev->active_speed,
 			 &rdev->active_width);
@@ -1612,7 +1613,6 @@ static void bnxt_re_dealloc_driver(struct ib_device *ib_dev)
 	struct bnxt_re_dev *rdev =
 		container_of(ib_dev, struct bnxt_re_dev, ibdev);
 
-	clear_bit(BNXT_RE_FLAG_IBDEV_REGISTERED, &rdev->flags);
 	dev_info(rdev_to_dev(rdev), "Unregistering Device");
 
 	rtnl_lock();
@@ -1630,12 +1630,7 @@ static void bnxt_re_task(struct work_struct *work)
 	re_work = container_of(work, struct bnxt_re_work, work);
 	rdev = re_work->rdev;
 
-	if (re_work->event != NETDEV_REGISTER &&
-	    !test_bit(BNXT_RE_FLAG_IBDEV_REGISTERED, &rdev->flags))
-		goto done;
-
-	switch (re_work->event) {
-	case NETDEV_REGISTER:
+	if (re_work->event == NETDEV_REGISTER) {
 		rc = bnxt_re_ib_init(rdev);
 		if (rc) {
 			ibdev_err(&rdev->ibdev,
@@ -1645,7 +1640,13 @@ static void bnxt_re_task(struct work_struct *work)
 			rtnl_unlock();
 			goto exit;
 		}
-		break;
+		goto exit;
+	}
+
+	if (!ib_device_try_get(&rdev->ibdev))
+		goto exit;
+
+	switch (re_work->event) {
 	case NETDEV_UP:
 		bnxt_re_dispatch_event(&rdev->ibdev, NULL, 1,
 				       IB_EVENT_PORT_ACTIVE);
@@ -1665,7 +1666,7 @@ static void bnxt_re_task(struct work_struct *work)
 	default:
 		break;
 	}
-done:
+	ib_device_put(&rdev->ibdev);
 	smp_mb__before_atomic();
 	atomic_dec(&rdev->sched_count);
 exit:
-- 
2.5.5

[PATCH v2 for-next 2/3] RDMA/bnxt_re: Fix lifetimes in bnxt_re_task

[Selvin Xavier]

From: Jason Gunthorpe <jgg@mellanox.com>

A work queue cannot just rely on the ib_device not being freed, it must
hold a kref on the memory so that the BNXT_RE_FLAG_IBDEV_REGISTERED check
works.

Fixes: 1ac5a4047975 ("RDMA/bnxt_re: Add bnxt_re RoCE driver")
Signed-off-by: Jason Gunthorpe <jgg@mellanox.com>
Signed-off-by: Selvin Xavier <selvin.xavier@broadcom.com>
---
 drivers/infiniband/hw/bnxt_re/main.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/infiniband/hw/bnxt_re/main.c b/drivers/infiniband/hw/bnxt_re/main.c
index 885127c..c494e11 100644
--- a/drivers/infiniband/hw/bnxt_re/main.c
+++ b/drivers/infiniband/hw/bnxt_re/main.c
@@ -1670,6 +1670,7 @@ static void bnxt_re_task(struct work_struct *work)
 	smp_mb__before_atomic();
 	atomic_dec(&rdev->sched_count);
 exit:
+	put_device(&rdev->ibdev.dev);
 	kfree(re_work);
 }
 
@@ -1735,6 +1736,7 @@ static int bnxt_re_netdev_event(struct notifier_block *notifier,
 		/* Allocate for the deferred task */
 		re_work = kzalloc(sizeof(*re_work), GFP_ATOMIC);
 		if (re_work) {
+			get_device(&rdev->ibdev.dev);
 			re_work->rdev = rdev;
 			re_work->event = event;
 			re_work->vlan_dev = (real_dev == netdev ?
-- 
2.5.5

[PATCH v2 for-next 3/3] RDMA/bnxt_re: Remove unnecessary sched count

[Selvin Xavier]

Since the lifetime of bnxt_re_task is controlled by
the kref of device, sched_count is no longer required.
Remove it.

Signed-off-by: Selvin Xavier <selvin.xavier@broadcom.com>
---
 drivers/infiniband/hw/bnxt_re/bnxt_re.h | 1 -
 drivers/infiniband/hw/bnxt_re/main.c    | 8 --------
 2 files changed, 9 deletions(-)

diff --git a/drivers/infiniband/hw/bnxt_re/bnxt_re.h b/drivers/infiniband/hw/bnxt_re/bnxt_re.h
index 407141e..a300588 100644
--- a/drivers/infiniband/hw/bnxt_re/bnxt_re.h
+++ b/drivers/infiniband/hw/bnxt_re/bnxt_re.h
@@ -176,7 +176,6 @@ struct bnxt_re_dev {
 	atomic_t			srq_count;
 	atomic_t			mr_count;
 	atomic_t			mw_count;
-	atomic_t			sched_count;
 	/* Max of 2 lossless traffic class supported per port */
 	u16				cosq[2];
 
diff --git a/drivers/infiniband/hw/bnxt_re/main.c b/drivers/infiniband/hw/bnxt_re/main.c
index c494e11..4a8fb1a 100644
--- a/drivers/infiniband/hw/bnxt_re/main.c
+++ b/drivers/infiniband/hw/bnxt_re/main.c
@@ -1667,8 +1667,6 @@ static void bnxt_re_task(struct work_struct *work)
 		break;
 	}
 	ib_device_put(&rdev->ibdev);
-	smp_mb__before_atomic();
-	atomic_dec(&rdev->sched_count);
 exit:
 	put_device(&rdev->ibdev.dev);
 	kfree(re_work);
@@ -1720,11 +1718,6 @@ static int bnxt_re_netdev_event(struct notifier_block *notifier,
 		break;
 
 	case NETDEV_UNREGISTER:
-		/* netdev notifier will call NETDEV_UNREGISTER again later since
-		 * we are still holding the reference to the netdev
-		 */
-		if (atomic_read(&rdev->sched_count) > 0)
-			goto exit;
 		ib_unregister_device_queued(&rdev->ibdev);
 		break;
 
@@ -1742,7 +1735,6 @@ static int bnxt_re_netdev_event(struct notifier_block *notifier,
 			re_work->vlan_dev = (real_dev == netdev ?
 					     NULL : netdev);
 			INIT_WORK(&re_work->work, bnxt_re_task);
-			atomic_inc(&rdev->sched_count);
 			queue_work(bnxt_re_wq, &re_work->work);
 		}
 	}
-- 
2.5.5

Re: [PATCH v2 for-next 0/3] RDMA/bnxt_re: Fixes for handling device references

[Jason Gunthorpe]

On Fri, Mar 13, 2020 at 09:33:24AM -0700, Selvin Xavier wrote:
> Avoid the driver's internal mechanisms to check
> for device registered state and counting the work queue
> scheduling.
> 
> v1-> v2:
>   - Removed all reference of BNXT_RE_FLAG_IBDEV_REGISTERED
>     from patch 1
>   - Removed smp_mb__before_atomic call from patch 3
> 
> Jason Gunthorpe (2):
>   RDMA/bnxt_re: Use ib_device_try_get()
>   RDMA/bnxt_re: Fix lifetimes in bnxt_re_task
> 
> Selvin Xavier (1):
>   RDMA/bnxt_re: Remove unnecessary sched count

Applied to for-next, thanks

Jason