All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH for-next v4 0/2] RDMA/bnxt_re driver update
@ 2020-02-26 15:45 Selvin Xavier
  2020-02-26 15:45 ` [PATCH for-next v4 1/2] RDMA/bnxt_re: Refactor device add/remove functionalities Selvin Xavier
                   ` (2 more replies)
  0 siblings, 3 replies; 6+ messages in thread
From: Selvin Xavier @ 2020-02-26 15:45 UTC (permalink / raw)
  To: dledford, jgg; +Cc: linux-rdma, Selvin Xavier

Includes code refactoring in the device init/deinit path and
use the new driver unregistration APIs.

Please apply to for-next.

Thanks,
Selvin

v3-> v4:
 - Added netdev state query  and report the correct link state
   during device registration
 - Removed GID event during device registration
v2 -> v3:
 - Droped the patch which was adding more state macros
 - To prevent addition of any device during driver removal,
   unregister netdev notifier and delete the driver's workqueu
   before calling ib_unregister_driver
v1-> v2:
 - Remove the patches 1,2 and 6 from the v1 series.
   They are already merged.
 - Added ASSERT_RTNL instead of comment in Patch 2
 - For Patch 3, explicitly queue the removal of the VF devices
   before calling ib_unregister_driver. This can avoid command
   timeouts seen, if the PFs gets removed before the VFs.
   Previous discussion - https://patchwork.kernel.org/patch/11260013/

Selvin Xavier (2):
  RDMA/bnxt_re: Refactor device add/remove functionalities
  RDMA/bnxt_re: Use driver_unregister and unregistration API

 drivers/infiniband/hw/bnxt_re/main.c | 213 ++++++++++++++++++-----------------
 1 file changed, 108 insertions(+), 105 deletions(-)

-- 
2.5.5


^ permalink raw reply	[flat|nested] 6+ messages in thread
* [PATCH for-next v4 1/2] RDMA/bnxt_re: Refactor device add/remove functionalities
  2020-02-26 15:45 [PATCH for-next v4 0/2] RDMA/bnxt_re driver update Selvin Xavier
@ 2020-02-26 15:45 ` Selvin Xavier
  2020-02-26 15:45 ` [PATCH for-next v4 2/2] RDMA/bnxt_re: Use driver_unregister and unregistration API Selvin Xavier
  2020-02-28 16:40 ` [PATCH for-next v4 0/2] RDMA/bnxt_re driver update Jason Gunthorpe
  2 siblings, 0 replies; 6+ messages in thread
From: Selvin Xavier @ 2020-02-26 15:45 UTC (permalink / raw)
  To: dledford, jgg; +Cc: linux-rdma, Selvin Xavier

 - bnxt_re_ib_reg() handles two main functionalities - initializing
   the device and registering with the IB stack.  Split it into 2
   functions i.e. bnxt_re_dev_init() and bnxt_re_ib_init()  to account
   for the same thereby improve modularity. Do the same for
   bnxt_re_ib_unreg()i.e. split into two functions - bnxt_re_dev_uninit()
   and  bnxt_re_ib_uninit().
 - Simplify the code by combining the different steps to add and
   remove the device into two functions.
 - Report correct netdev link state during device register

Signed-off-by: Selvin Xavier <selvin.xavier@broadcom.com>
---
 drivers/infiniband/hw/bnxt_re/main.c | 139 +++++++++++++++++++++--------------
 1 file changed, 82 insertions(+), 57 deletions(-)

diff --git a/drivers/infiniband/hw/bnxt_re/main.c b/drivers/infiniband/hw/bnxt_re/main.c
index b5128cc..5f8fd74 100644
--- a/drivers/infiniband/hw/bnxt_re/main.c
+++ b/drivers/infiniband/hw/bnxt_re/main.c
@@ -78,7 +78,8 @@ static struct list_head bnxt_re_dev_list = LIST_HEAD_INIT(bnxt_re_dev_list);
 /* Mutex to protect the list of bnxt_re devices added */
 static DEFINE_MUTEX(bnxt_re_dev_lock);
 static struct workqueue_struct *bnxt_re_wq;
-static void bnxt_re_ib_unreg(struct bnxt_re_dev *rdev);
+static void bnxt_re_remove_device(struct bnxt_re_dev *rdev);
+static void bnxt_re_ib_uninit(struct bnxt_re_dev *rdev);
 
 static void bnxt_re_destroy_chip_ctx(struct bnxt_re_dev *rdev)
 {
@@ -237,7 +238,9 @@ static void bnxt_re_shutdown(void *p)
 	if (!rdev)
 		return;
 
-	bnxt_re_ib_unreg(rdev);
+	bnxt_re_ib_uninit(rdev);
+	ASSERT_RTNL();
+	bnxt_re_remove_device(rdev);
 }
 
 static void bnxt_re_stop_irq(void *handle)
@@ -1317,7 +1320,41 @@ static void bnxt_re_query_hwrm_intf_version(struct bnxt_re_dev *rdev)
 		le16_to_cpu(resp.hwrm_intf_patch);
 }
 
-static void bnxt_re_ib_unreg(struct bnxt_re_dev *rdev)
+static void bnxt_re_ib_uninit(struct bnxt_re_dev *rdev)
+{
+	/* Cleanup ib dev */
+	if (test_bit(BNXT_RE_FLAG_IBDEV_REGISTERED, &rdev->flags)) {
+		ib_unregister_device(&rdev->ibdev);
+		clear_bit(BNXT_RE_FLAG_IBDEV_REGISTERED, &rdev->flags);
+	}
+}
+
+int bnxt_re_ib_init(struct bnxt_re_dev *rdev)
+{
+	int rc = 0;
+	u32 event;
+
+	/* Register ib dev */
+	rc = bnxt_re_register_ib(rdev);
+	if (rc) {
+		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);
+	set_bit(BNXT_RE_FLAG_ISSUE_ROCE_STATS, &rdev->flags);
+
+	event = netif_running(rdev->netdev) && netif_carrier_ok(rdev->netdev) ?
+		IB_EVENT_PORT_ACTIVE : IB_EVENT_PORT_ERR;
+
+	bnxt_re_dispatch_event(&rdev->ibdev, NULL, 1, event);
+
+	return rc;
+}
+
+static void bnxt_re_dev_uninit(struct bnxt_re_dev *rdev)
 {
 	u8 type;
 	int rc;
@@ -1373,20 +1410,15 @@ static void bnxt_re_worker(struct work_struct *work)
 	schedule_delayed_work(&rdev->worker, msecs_to_jiffies(30000));
 }
 
-static int bnxt_re_ib_reg(struct bnxt_re_dev *rdev)
+static int bnxt_re_dev_init(struct bnxt_re_dev *rdev)
 {
 	struct bnxt_qplib_creq_ctx *creq;
 	struct bnxt_re_ring_attr rattr;
 	u32 db_offt;
-	bool locked;
 	int vid;
 	u8 type;
 	int rc;
 
-	/* Acquire rtnl lock through out this function */
-	rtnl_lock();
-	locked = true;
-
 	/* Registered a new RoCE device instance to netdev */
 	memset(&rattr, 0, sizeof(rattr));
 	rc = bnxt_re_register_netdev(rdev);
@@ -1514,23 +1546,6 @@ static int bnxt_re_ib_reg(struct bnxt_re_dev *rdev)
 		schedule_delayed_work(&rdev->worker, msecs_to_jiffies(30000));
 	}
 
-	rtnl_unlock();
-	locked = false;
-
-	/* Register ib dev */
-	rc = bnxt_re_register_ib(rdev);
-	if (rc) {
-		ibdev_err(&rdev->ibdev,
-			  "Failed to register with IB: %#x\n", rc);
-		goto fail;
-	}
-	set_bit(BNXT_RE_FLAG_IBDEV_REGISTERED, &rdev->flags);
-	ibdev_info(&rdev->ibdev, "Device registered successfully");
-	ib_get_eth_speed(&rdev->ibdev, 1, &rdev->active_speed,
-			 &rdev->active_width);
-	set_bit(BNXT_RE_FLAG_ISSUE_ROCE_STATS, &rdev->flags);
-	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);
@@ -1544,10 +1559,7 @@ static int bnxt_re_ib_reg(struct bnxt_re_dev *rdev)
 free_rcfw:
 	bnxt_qplib_free_rcfw_channel(&rdev->rcfw);
 fail:
-	if (!locked)
-		rtnl_lock();
-	bnxt_re_ib_unreg(rdev);
-	rtnl_unlock();
+	bnxt_re_dev_uninit(rdev);
 
 	return rc;
 }
@@ -1589,9 +1601,35 @@ static int bnxt_re_dev_reg(struct bnxt_re_dev **rdev, struct net_device *netdev)
 	return rc;
 }
 
-static void bnxt_re_remove_one(struct bnxt_re_dev *rdev)
+static void bnxt_re_remove_device(struct bnxt_re_dev *rdev)
 {
+	bnxt_re_dev_uninit(rdev);
 	pci_dev_put(rdev->en_dev->pdev);
+	bnxt_re_dev_unreg(rdev);
+}
+
+static int bnxt_re_add_device(struct bnxt_re_dev **rdev,
+			      struct net_device *netdev)
+{
+	int rc;
+
+	rc = bnxt_re_dev_reg(rdev, netdev);
+	if (rc == -ENODEV)
+		return rc;
+	if (rc) {
+		pr_err("Failed to register with the device %s: %#x\n",
+		       netdev->name, rc);
+		return rc;
+	}
+
+	pci_dev_get((*rdev)->en_dev->pdev);
+	rc = bnxt_re_dev_init(*rdev);
+	if (rc) {
+		pci_dev_put((*rdev)->en_dev->pdev);
+		bnxt_re_dev_unreg(*rdev);
+	}
+
+	return rc;
 }
 
 /* Handle all deferred netevents tasks */
@@ -1606,16 +1644,17 @@ static void bnxt_re_task(struct work_struct *work)
 
 	if (re_work->event != NETDEV_REGISTER &&
 	    !test_bit(BNXT_RE_FLAG_IBDEV_REGISTERED, &rdev->flags))
-		return;
+		goto done;
 
 	switch (re_work->event) {
 	case NETDEV_REGISTER:
-		rc = bnxt_re_ib_reg(rdev);
+		rc = bnxt_re_ib_init(rdev);
 		if (rc) {
 			ibdev_err(&rdev->ibdev,
 				  "Failed to register with IB: %#x", rc);
-			bnxt_re_remove_one(rdev);
-			bnxt_re_dev_unreg(rdev);
+			rtnl_lock();
+			bnxt_re_remove_device(rdev);
+			rtnl_unlock();
 			goto exit;
 		}
 		break;
@@ -1638,17 +1677,13 @@ static void bnxt_re_task(struct work_struct *work)
 	default:
 		break;
 	}
+done:
 	smp_mb__before_atomic();
 	atomic_dec(&rdev->sched_count);
 exit:
 	kfree(re_work);
 }
 
-static void bnxt_re_init_one(struct bnxt_re_dev *rdev)
-{
-	pci_dev_get(rdev->en_dev->pdev);
-}
-
 /*
  * "Notifier chain callback can be invoked for the same chain from
  * different CPUs at the same time".
@@ -1686,17 +1721,9 @@ static int bnxt_re_netdev_event(struct notifier_block *notifier,
 	case NETDEV_REGISTER:
 		if (rdev)
 			break;
-		rc = bnxt_re_dev_reg(&rdev, real_dev);
-		if (rc == -ENODEV)
-			break;
-		if (rc) {
-			ibdev_err(&rdev->ibdev,
-				  "Failed to register with the device %s: %#x\n",
-				  real_dev->name, rc);
-			break;
-		}
-		bnxt_re_init_one(rdev);
-		sch_work = true;
+		rc = bnxt_re_add_device(&rdev, real_dev);
+		if (!rc)
+			sch_work = true;
 		break;
 
 	case NETDEV_UNREGISTER:
@@ -1705,9 +1732,8 @@ static int bnxt_re_netdev_event(struct notifier_block *notifier,
 		 */
 		if (atomic_read(&rdev->sched_count) > 0)
 			goto exit;
-		bnxt_re_ib_unreg(rdev);
-		bnxt_re_remove_one(rdev);
-		bnxt_re_dev_unreg(rdev);
+		bnxt_re_ib_uninit(rdev);
+		bnxt_re_remove_device(rdev);
 		break;
 
 	default:
@@ -1784,12 +1810,11 @@ static void __exit bnxt_re_mod_exit(void)
 		 */
 		flush_workqueue(bnxt_re_wq);
 		bnxt_re_dev_stop(rdev);
+		bnxt_re_ib_uninit(rdev);
 		/* Acquire the rtnl_lock as the L2 resources are freed here */
 		rtnl_lock();
-		bnxt_re_ib_unreg(rdev);
+		bnxt_re_remove_device(rdev);
 		rtnl_unlock();
-		bnxt_re_remove_one(rdev);
-		bnxt_re_dev_unreg(rdev);
 	}
 	unregister_netdevice_notifier(&bnxt_re_netdev_notifier);
 	if (bnxt_re_wq)
-- 
2.5.5


^ permalink raw reply related	[flat|nested] 6+ messages in thread
* [PATCH for-next v4 2/2] RDMA/bnxt_re: Use driver_unregister and unregistration API
  2020-02-26 15:45 [PATCH for-next v4 0/2] RDMA/bnxt_re driver update Selvin Xavier
  2020-02-26 15:45 ` [PATCH for-next v4 1/2] RDMA/bnxt_re: Refactor device add/remove functionalities Selvin Xavier
@ 2020-02-26 15:45 ` Selvin Xavier
  2020-02-28 16:35   ` Jason Gunthorpe
  2020-02-28 16:40 ` [PATCH for-next v4 0/2] RDMA/bnxt_re driver update Jason Gunthorpe
  2 siblings, 1 reply; 6+ messages in thread
From: Selvin Xavier @ 2020-02-26 15:45 UTC (permalink / raw)
  To: dledford, jgg; +Cc: linux-rdma, Selvin Xavier

Using the new unregister APIs provided by the core.
Provide the dealloc_driver hook for the core to callback at the time
of device un-registration.

bnxt_re VF resources are created by the corresponding PF driver.
During ib_unregister_driver, PF might get removed before VF
and this could cause failure when VFs are removed. Driver
is explicitly queuing the removal of VF devices before
calling ib_unregister_driver.

Signed-off-by: Selvin Xavier <selvin.xavier@broadcom.com>
---
 drivers/infiniband/hw/bnxt_re/main.c | 106 ++++++++++++++---------------------
 1 file changed, 42 insertions(+), 64 deletions(-)

diff --git a/drivers/infiniband/hw/bnxt_re/main.c b/drivers/infiniband/hw/bnxt_re/main.c
index 5f8fd74..415693f 100644
--- a/drivers/infiniband/hw/bnxt_re/main.c
+++ b/drivers/infiniband/hw/bnxt_re/main.c
@@ -79,7 +79,8 @@ static struct list_head bnxt_re_dev_list = LIST_HEAD_INIT(bnxt_re_dev_list);
 static DEFINE_MUTEX(bnxt_re_dev_lock);
 static struct workqueue_struct *bnxt_re_wq;
 static void bnxt_re_remove_device(struct bnxt_re_dev *rdev);
-static void bnxt_re_ib_uninit(struct bnxt_re_dev *rdev);
+static void bnxt_re_dealloc_driver(struct ib_device *ib_dev);
+static void bnxt_re_stop_irq(void *handle);
 
 static void bnxt_re_destroy_chip_ctx(struct bnxt_re_dev *rdev)
 {
@@ -237,10 +238,10 @@ static void bnxt_re_shutdown(void *p)
 
 	if (!rdev)
 		return;
-
-	bnxt_re_ib_uninit(rdev);
 	ASSERT_RTNL();
-	bnxt_re_remove_device(rdev);
+	/* Release the MSIx vectors before queuing unregister */
+	bnxt_re_stop_irq(rdev);
+	ib_unregister_device_queued(&rdev->ibdev);
 }
 
 static void bnxt_re_stop_irq(void *handle)
@@ -542,17 +543,12 @@ static bool is_bnxt_re_dev(struct net_device *netdev)
 
 static struct bnxt_re_dev *bnxt_re_from_netdev(struct net_device *netdev)
 {
-	struct bnxt_re_dev *rdev;
+	struct ib_device *ibdev =
+		ib_device_get_by_netdev(netdev, RDMA_DRIVER_BNXT_RE);
+	if (!ibdev)
+		return NULL;
 
-	rcu_read_lock();
-	list_for_each_entry_rcu(rdev, &bnxt_re_dev_list, list) {
-		if (rdev->netdev == netdev) {
-			rcu_read_unlock();
-			return rdev;
-		}
-	}
-	rcu_read_unlock();
-	return NULL;
+	return container_of(ibdev, struct bnxt_re_dev, ibdev);
 }
 
 static void bnxt_re_dev_unprobe(struct net_device *netdev,
@@ -626,11 +622,6 @@ static const struct attribute_group bnxt_re_dev_attr_group = {
 	.attrs = bnxt_re_attributes,
 };
 
-static void bnxt_re_unregister_ib(struct bnxt_re_dev *rdev)
-{
-	ib_unregister_device(&rdev->ibdev);
-}
-
 static const struct ib_device_ops bnxt_re_dev_ops = {
 	.owner = THIS_MODULE,
 	.driver_id = RDMA_DRIVER_BNXT_RE,
@@ -645,6 +636,7 @@ static const struct ib_device_ops bnxt_re_dev_ops = {
 	.create_cq = bnxt_re_create_cq,
 	.create_qp = bnxt_re_create_qp,
 	.create_srq = bnxt_re_create_srq,
+	.dealloc_driver = bnxt_re_dealloc_driver,
 	.dealloc_pd = bnxt_re_dealloc_pd,
 	.dealloc_ucontext = bnxt_re_dealloc_ucontext,
 	.del_gid = bnxt_re_del_gid,
@@ -741,15 +733,11 @@ static void bnxt_re_dev_remove(struct bnxt_re_dev *rdev)
 {
 	dev_put(rdev->netdev);
 	rdev->netdev = NULL;
-
 	mutex_lock(&bnxt_re_dev_lock);
 	list_del_rcu(&rdev->list);
 	mutex_unlock(&bnxt_re_dev_lock);
 
 	synchronize_rcu();
-
-	ib_dealloc_device(&rdev->ibdev);
-	/* rdev is gone */
 }
 
 static struct bnxt_re_dev *bnxt_re_dev_add(struct net_device *netdev,
@@ -1320,15 +1308,6 @@ static void bnxt_re_query_hwrm_intf_version(struct bnxt_re_dev *rdev)
 		le16_to_cpu(resp.hwrm_intf_patch);
 }
 
-static void bnxt_re_ib_uninit(struct bnxt_re_dev *rdev)
-{
-	/* Cleanup ib dev */
-	if (test_bit(BNXT_RE_FLAG_IBDEV_REGISTERED, &rdev->flags)) {
-		ib_unregister_device(&rdev->ibdev);
-		clear_bit(BNXT_RE_FLAG_IBDEV_REGISTERED, &rdev->flags);
-	}
-}
-
 int bnxt_re_ib_init(struct bnxt_re_dev *rdev)
 {
 	int rc = 0;
@@ -1359,10 +1338,6 @@ static void bnxt_re_dev_uninit(struct bnxt_re_dev *rdev)
 	u8 type;
 	int rc;
 
-	if (test_and_clear_bit(BNXT_RE_FLAG_IBDEV_REGISTERED, &rdev->flags)) {
-		/* Cleanup ib dev */
-		bnxt_re_unregister_ib(rdev);
-	}
 	if (test_and_clear_bit(BNXT_RE_FLAG_QOS_WORK_REG, &rdev->flags))
 		cancel_delayed_work_sync(&rdev->worker);
 
@@ -1632,6 +1607,19 @@ static int bnxt_re_add_device(struct bnxt_re_dev **rdev,
 	return rc;
 }
 
+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();
+	bnxt_re_remove_device(rdev);
+	rtnl_unlock();
+}
+
 /* Handle all deferred netevents tasks */
 static void bnxt_re_task(struct work_struct *work)
 {
@@ -1706,6 +1694,7 @@ static int bnxt_re_netdev_event(struct notifier_block *notifier,
 	struct bnxt_re_dev *rdev;
 	int rc = 0;
 	bool sch_work = false;
+	bool release = true;
 
 	real_dev = rdma_vlan_dev_real_dev(netdev);
 	if (!real_dev)
@@ -1713,7 +1702,8 @@ static int bnxt_re_netdev_event(struct notifier_block *notifier,
 
 	rdev = bnxt_re_from_netdev(real_dev);
 	if (!rdev && event != NETDEV_REGISTER)
-		goto exit;
+		return NOTIFY_OK;
+
 	if (real_dev != netdev)
 		goto exit;
 
@@ -1724,6 +1714,7 @@ static int bnxt_re_netdev_event(struct notifier_block *notifier,
 		rc = bnxt_re_add_device(&rdev, real_dev);
 		if (!rc)
 			sch_work = true;
+		release = false;
 		break;
 
 	case NETDEV_UNREGISTER:
@@ -1732,8 +1723,7 @@ static int bnxt_re_netdev_event(struct notifier_block *notifier,
 		 */
 		if (atomic_read(&rdev->sched_count) > 0)
 			goto exit;
-		bnxt_re_ib_uninit(rdev);
-		bnxt_re_remove_device(rdev);
+		ib_unregister_device_queued(&rdev->ibdev);
 		break;
 
 	default:
@@ -1755,6 +1745,8 @@ static int bnxt_re_netdev_event(struct notifier_block *notifier,
 	}
 
 exit:
+	if (rdev && release)
+		ib_device_put(&rdev->ibdev);
 	return NOTIFY_DONE;
 }
 
@@ -1790,35 +1782,21 @@ static int __init bnxt_re_mod_init(void)
 
 static void __exit bnxt_re_mod_exit(void)
 {
-	struct bnxt_re_dev *rdev, *next;
-	LIST_HEAD(to_be_deleted);
+	struct bnxt_re_dev *rdev;
 
-	mutex_lock(&bnxt_re_dev_lock);
-	/* Free all adapter allocated resources */
-	if (!list_empty(&bnxt_re_dev_list))
-		list_splice_init(&bnxt_re_dev_list, &to_be_deleted);
-	mutex_unlock(&bnxt_re_dev_lock);
-       /*
-	* Cleanup the devices in reverse order so that the VF device
-	* cleanup is done before PF cleanup
-	*/
-	list_for_each_entry_safe_reverse(rdev, next, &to_be_deleted, list) {
-		ibdev_info(&rdev->ibdev, "Unregistering Device");
-		/*
-		 * Flush out any scheduled tasks before destroying the
-		 * resources
-		 */
-		flush_workqueue(bnxt_re_wq);
-		bnxt_re_dev_stop(rdev);
-		bnxt_re_ib_uninit(rdev);
-		/* Acquire the rtnl_lock as the L2 resources are freed here */
-		rtnl_lock();
-		bnxt_re_remove_device(rdev);
-		rtnl_unlock();
-	}
 	unregister_netdevice_notifier(&bnxt_re_netdev_notifier);
 	if (bnxt_re_wq)
 		destroy_workqueue(bnxt_re_wq);
+	list_for_each_entry(rdev, &bnxt_re_dev_list, list) {
+		/* VF device removal should be called before the removal
+		 * of PF device. Queue VFs unregister first, so that VFs
+		 * shall be removed before the PF during the call of
+		 * ib_unregister_driver.
+		 */
+		if (rdev->is_virtfn)
+			ib_unregister_device(&rdev->ibdev);
+	}
+	ib_unregister_driver(RDMA_DRIVER_BNXT_RE);
 }
 
 module_init(bnxt_re_mod_init);
-- 
2.5.5


^ permalink raw reply related	[flat|nested] 6+ messages in thread
* Re: [PATCH for-next v4 2/2] RDMA/bnxt_re: Use driver_unregister and unregistration API
  2020-02-26 15:45 ` [PATCH for-next v4 2/2] RDMA/bnxt_re: Use driver_unregister and unregistration API Selvin Xavier
@ 2020-02-28 16:35   ` Jason Gunthorpe
  2020-03-02  4:42     ` Selvin Xavier
  0 siblings, 1 reply; 6+ messages in thread
From: Jason Gunthorpe @ 2020-02-28 16:35 UTC (permalink / raw)
  To: Selvin Xavier; +Cc: dledford, linux-rdma

On Wed, Feb 26, 2020 at 07:45:32AM -0800, Selvin Xavier wrote:
> @@ -1724,6 +1714,7 @@ static int bnxt_re_netdev_event(struct notifier_block *notifier,
>  		rc = bnxt_re_add_device(&rdev, real_dev);
>  		if (!rc)
>  			sch_work = true;
> +		release = false;
>  		break;
>  
>  	case NETDEV_UNREGISTER:
> @@ -1732,8 +1723,7 @@ static int bnxt_re_netdev_event(struct notifier_block *notifier,
>  		 */
>  		if (atomic_read(&rdev->sched_count) > 0)
>  			goto exit;

This sched_count stuff needs cleaning too.

krefs should be used properly, carry the kref on the ib_device into
the work and use the registration lock on the ib device to serialize
instead of this sched_count stuff.

This all sounds so familiar.. Oh I tried to fix this once - maybe the
below will help you:

commit 33d88c818d155ffb2ef4b12e72107f628c70404c
Author: Jason Gunthorpe <jgg@ziepe.ca>
Date:   Thu Jan 10 12:05:19 2019 -0700

    RDMA/bnxt_re: Use ib_device_get_by_netdev() instead of open coding
    
    The core API handles the locking correctly and is faster if there
    are multiple devices.
    
    Signed-off-by: Jason Gunthorpe <jgg@mellanox.com>

diff --git a/drivers/infiniband/hw/bnxt_re/main.c b/drivers/infiniband/hw/bnxt_re/main.c
index fa539608ffbbe0..bd67a31937ec65 100644
--- a/drivers/infiniband/hw/bnxt_re/main.c
+++ b/drivers/infiniband/hw/bnxt_re/main.c
@@ -504,21 +504,6 @@ static bool is_bnxt_re_dev(struct net_device *netdev)
 	return false;
 }
 
-static struct bnxt_re_dev *bnxt_re_from_netdev(struct net_device *netdev)
-{
-	struct bnxt_re_dev *rdev;
-
-	rcu_read_lock();
-	list_for_each_entry_rcu(rdev, &bnxt_re_dev_list, list) {
-		if (rdev->netdev == netdev) {
-			rcu_read_unlock();
-			return rdev;
-		}
-	}
-	rcu_read_unlock();
-	return NULL;
-}
-
 static void bnxt_re_dev_unprobe(struct net_device *netdev,
 				struct bnxt_en_dev *en_dev)
 {
@@ -1616,23 +1601,26 @@ static int bnxt_re_netdev_event(struct notifier_block *notifier,
 {
 	struct net_device *real_dev, *netdev = netdev_notifier_info_to_dev(ptr);
 	struct bnxt_re_work *re_work;
-	struct bnxt_re_dev *rdev;
+	struct bnxt_re_dev *rdev = NULL;
+	struct ib_device *ibdev;
 	int rc = 0;
 	bool sch_work = false;
 
 	real_dev = rdma_vlan_dev_real_dev(netdev);
 	if (!real_dev)
 		real_dev = netdev;
-
-	rdev = bnxt_re_from_netdev(real_dev);
-	if (!rdev && event != NETDEV_REGISTER)
-		goto exit;
 	if (real_dev != netdev)
 		goto exit;
 
+	ibdev = ib_device_get_by_netdev(real_dev, RDMA_DRIVER_BNXT_RE);
+	if (!ibdev && event != NETDEV_REGISTER)
+		goto exit;
+	if (ibdev)
+		rdev = container_of(ibdev, struct bnxt_re_dev, ibdev);
+
 	switch (event) {
 	case NETDEV_REGISTER:
-		if (rdev)
+		if (ibdev)
 			break;
 		rc = bnxt_re_dev_reg(&rdev, real_dev);
 		if (rc == -ENODEV)
@@ -1676,6 +1664,9 @@ static int bnxt_re_netdev_event(struct notifier_block *notifier,
 		}
 	}
 
+	if (ibdev)
+		ib_device_put(ibdev);
+
 exit:
 	return NOTIFY_DONE;
 }

commit 6c617f08e749ee0f6c7be6763ea92e49ae484712
Author: Jason Gunthorpe <jgg@ziepe.ca>
Date:   Thu Jan 10 14:40:16 2019 -0700

    RDMA/bnxt_re: Use ib_device_try_get()
    
    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>

diff --git a/drivers/infiniband/hw/bnxt_re/main.c b/drivers/infiniband/hw/bnxt_re/main.c
index 666897596218d3..fa539608ffbbe0 100644
--- a/drivers/infiniband/hw/bnxt_re/main.c
+++ b/drivers/infiniband/hw/bnxt_re/main.c
@@ -1137,12 +1137,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) {
 		dev_err(rdev_to_dev(rdev), "QPLIB: SGID table not allocated");
-		return -EINVAL;
+		rc = -EINVAL;
+		goto out;
 	}
 
 	for (index = 0; index < sgid_tbl->active; index++) {
@@ -1163,6 +1164,8 @@ static int bnxt_re_update_gid(struct bnxt_re_dev *rdev)
 					    rdev->qplib_res.netdev->dev_addr);
 	}
 
+out:
+	ib_device_put(&rdev->ibdev);
 	return rc;
 }
 
@@ -1545,12 +1548,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 exit;
-
-	switch (re_work->event) {
-	case NETDEV_REGISTER:
+	if (re_work->event == NETDEV_REGISTER) {
 		rc = bnxt_re_ib_reg(rdev);
 		if (rc) {
 			dev_err(rdev_to_dev(rdev),
@@ -1559,7 +1557,13 @@ static void bnxt_re_task(struct work_struct *work)
 			bnxt_re_dev_unreg(rdev);
 			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);
@@ -1579,6 +1583,8 @@ static void bnxt_re_task(struct work_struct *work)
 	default:
 		break;
 	}
+
+	ib_device_put(&rdev->ibdev);
 	smp_mb__before_atomic();
 	atomic_dec(&rdev->sched_count);
 exit:

commit e64da98a182a2cae3338f28f6e581f189b5f8674
Author: Jason Gunthorpe <jgg@ziepe.ca>
Date:   Thu Jan 10 12:02:11 2019 -0700

    RDMA/bnxt_re: Fix lifetimes in bnxt_re_task
    
    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.
    
    Also, every single work queue call has an allocated memory, and the kfree
    of this memory was missed sometimes.
    
    Fixes: 1ac5a4047975 ("RDMA/bnxt_re: Add bnxt_re RoCE driver")
    Signed-off-by: Jason Gunthorpe <jgg@mellanox.com>

diff --git a/drivers/infiniband/hw/bnxt_re/main.c b/drivers/infiniband/hw/bnxt_re/main.c
index 814f959c7db965..666897596218d3 100644
--- a/drivers/infiniband/hw/bnxt_re/main.c
+++ b/drivers/infiniband/hw/bnxt_re/main.c
@@ -1547,7 +1547,7 @@ static void bnxt_re_task(struct work_struct *work)
 
 	if (re_work->event != NETDEV_REGISTER &&
 	    !test_bit(BNXT_RE_FLAG_IBDEV_REGISTERED, &rdev->flags))
-		return;
+		goto exit;
 
 	switch (re_work->event) {
 	case NETDEV_REGISTER:
@@ -1582,6 +1582,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);
 }
 
@@ -1658,6 +1659,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 ?

^ permalink raw reply related	[flat|nested] 6+ messages in thread
* Re: [PATCH for-next v4 0/2] RDMA/bnxt_re driver update
  2020-02-26 15:45 [PATCH for-next v4 0/2] RDMA/bnxt_re driver update Selvin Xavier
  2020-02-26 15:45 ` [PATCH for-next v4 1/2] RDMA/bnxt_re: Refactor device add/remove functionalities Selvin Xavier
  2020-02-26 15:45 ` [PATCH for-next v4 2/2] RDMA/bnxt_re: Use driver_unregister and unregistration API Selvin Xavier
@ 2020-02-28 16:40 ` Jason Gunthorpe
  2 siblings, 0 replies; 6+ messages in thread
From: Jason Gunthorpe @ 2020-02-28 16:40 UTC (permalink / raw)
  To: Selvin Xavier; +Cc: dledford, linux-rdma

On Wed, Feb 26, 2020 at 07:45:30AM -0800, Selvin Xavier wrote:
> Includes code refactoring in the device init/deinit path and
> use the new driver unregistration APIs.
> 
> Please apply to for-next.
> 
> Thanks,
> Selvin
> 
> v3-> v4:
>  - Added netdev state query  and report the correct link state
>    during device registration
>  - Removed GID event during device registration
> v2 -> v3:
>  - Droped the patch which was adding more state macros
>  - To prevent addition of any device during driver removal,
>    unregister netdev notifier and delete the driver's workqueu
>    before calling ib_unregister_driver
> v1-> v2:
>  - Remove the patches 1,2 and 6 from the v1 series.
>    They are already merged.
>  - Added ASSERT_RTNL instead of comment in Patch 2
>  - For Patch 3, explicitly queue the removal of the VF devices
>    before calling ib_unregister_driver. This can avoid command
>    timeouts seen, if the PFs gets removed before the VFs.
>    Previous discussion - https://patchwork.kernel.org/patch/11260013/
> 
> Selvin Xavier (2):
>   RDMA/bnxt_re: Refactor device add/remove functionalities
>   RDMA/bnxt_re: Use driver_unregister and unregistration API

Aside from the ugly sched_count this is an improvement, so applied to
for-next

Thanks,
Jason

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH for-next v4 2/2] RDMA/bnxt_re: Use driver_unregister and unregistration API
  2020-02-28 16:35   ` Jason Gunthorpe
@ 2020-03-02  4:42     ` Selvin Xavier
  0 siblings, 0 replies; 6+ messages in thread
From: Selvin Xavier @ 2020-03-02  4:42 UTC (permalink / raw)
  To: Jason Gunthorpe; +Cc: Doug Ledford, linux-rdma

On Fri, Feb 28, 2020 at 10:05 PM Jason Gunthorpe <jgg@ziepe.ca> wrote:
>
> On Wed, Feb 26, 2020 at 07:45:32AM -0800, Selvin Xavier wrote:
> > @@ -1724,6 +1714,7 @@ static int bnxt_re_netdev_event(struct notifier_block *notifier,
> >               rc = bnxt_re_add_device(&rdev, real_dev);
> >               if (!rc)
> >                       sch_work = true;
> > +             release = false;
> >               break;
> >
> >       case NETDEV_UNREGISTER:
> > @@ -1732,8 +1723,7 @@ static int bnxt_re_netdev_event(struct notifier_block *notifier,
> >                */
> >               if (atomic_read(&rdev->sched_count) > 0)
> >                       goto exit;
>
> This sched_count stuff needs cleaning too.
>
> krefs should be used properly, carry the kref on the ib_device into
> the work and use the registration lock on the ib device to serialize
> instead of this sched_count stuff.
>
> This all sounds so familiar.. Oh I tried to fix this once - maybe the
> below will help you:
>
Thanks Jason for the patches. Changes in first patch is already
taken care in my series. Will test  your other two patches and will
get back.

Thanks,
Selvin
> commit 33d88c818d155ffb2ef4b12e72107f628c70404c
> Author: Jason Gunthorpe <jgg@ziepe.ca>
> Date:   Thu Jan 10 12:05:19 2019 -0700
>
>     RDMA/bnxt_re: Use ib_device_get_by_netdev() instead of open coding
>
>     The core API handles the locking correctly and is faster if there
>     are multiple devices.
>
>     Signed-off-by: Jason Gunthorpe <jgg@mellanox.com>
>
> diff --git a/drivers/infiniband/hw/bnxt_re/main.c b/drivers/infiniband/hw/bnxt_re/main.c
> index fa539608ffbbe0..bd67a31937ec65 100644
> --- a/drivers/infiniband/hw/bnxt_re/main.c
> +++ b/drivers/infiniband/hw/bnxt_re/main.c
> @@ -504,21 +504,6 @@ static bool is_bnxt_re_dev(struct net_device *netdev)
>         return false;
>  }
>
> -static struct bnxt_re_dev *bnxt_re_from_netdev(struct net_device *netdev)
> -{
> -       struct bnxt_re_dev *rdev;
> -
> -       rcu_read_lock();
> -       list_for_each_entry_rcu(rdev, &bnxt_re_dev_list, list) {
> -               if (rdev->netdev == netdev) {
> -                       rcu_read_unlock();
> -                       return rdev;
> -               }
> -       }
> -       rcu_read_unlock();
> -       return NULL;
> -}
> -
>  static void bnxt_re_dev_unprobe(struct net_device *netdev,
>                                 struct bnxt_en_dev *en_dev)
>  {
> @@ -1616,23 +1601,26 @@ static int bnxt_re_netdev_event(struct notifier_block *notifier,
>  {
>         struct net_device *real_dev, *netdev = netdev_notifier_info_to_dev(ptr);
>         struct bnxt_re_work *re_work;
> -       struct bnxt_re_dev *rdev;
> +       struct bnxt_re_dev *rdev = NULL;
> +       struct ib_device *ibdev;
>         int rc = 0;
>         bool sch_work = false;
>
>         real_dev = rdma_vlan_dev_real_dev(netdev);
>         if (!real_dev)
>                 real_dev = netdev;
> -
> -       rdev = bnxt_re_from_netdev(real_dev);
> -       if (!rdev && event != NETDEV_REGISTER)
> -               goto exit;
>         if (real_dev != netdev)
>                 goto exit;
>
> +       ibdev = ib_device_get_by_netdev(real_dev, RDMA_DRIVER_BNXT_RE);
> +       if (!ibdev && event != NETDEV_REGISTER)
> +               goto exit;
> +       if (ibdev)
> +               rdev = container_of(ibdev, struct bnxt_re_dev, ibdev);
> +
>         switch (event) {
>         case NETDEV_REGISTER:
> -               if (rdev)
> +               if (ibdev)
>                         break;
>                 rc = bnxt_re_dev_reg(&rdev, real_dev);
>                 if (rc == -ENODEV)
> @@ -1676,6 +1664,9 @@ static int bnxt_re_netdev_event(struct notifier_block *notifier,
>                 }
>         }
>
> +       if (ibdev)
> +               ib_device_put(ibdev);
> +
>  exit:
>         return NOTIFY_DONE;
>  }
>
> commit 6c617f08e749ee0f6c7be6763ea92e49ae484712
> Author: Jason Gunthorpe <jgg@ziepe.ca>
> Date:   Thu Jan 10 14:40:16 2019 -0700
>
>     RDMA/bnxt_re: Use ib_device_try_get()
>
>     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>
>
> diff --git a/drivers/infiniband/hw/bnxt_re/main.c b/drivers/infiniband/hw/bnxt_re/main.c
> index 666897596218d3..fa539608ffbbe0 100644
> --- a/drivers/infiniband/hw/bnxt_re/main.c
> +++ b/drivers/infiniband/hw/bnxt_re/main.c
> @@ -1137,12 +1137,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) {
>                 dev_err(rdev_to_dev(rdev), "QPLIB: SGID table not allocated");
> -               return -EINVAL;
> +               rc = -EINVAL;
> +               goto out;
>         }
>
>         for (index = 0; index < sgid_tbl->active; index++) {
> @@ -1163,6 +1164,8 @@ static int bnxt_re_update_gid(struct bnxt_re_dev *rdev)
>                                             rdev->qplib_res.netdev->dev_addr);
>         }
>
> +out:
> +       ib_device_put(&rdev->ibdev);
>         return rc;
>  }
>
> @@ -1545,12 +1548,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 exit;
> -
> -       switch (re_work->event) {
> -       case NETDEV_REGISTER:
> +       if (re_work->event == NETDEV_REGISTER) {
>                 rc = bnxt_re_ib_reg(rdev);
>                 if (rc) {
>                         dev_err(rdev_to_dev(rdev),
> @@ -1559,7 +1557,13 @@ static void bnxt_re_task(struct work_struct *work)
>                         bnxt_re_dev_unreg(rdev);
>                         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);
> @@ -1579,6 +1583,8 @@ static void bnxt_re_task(struct work_struct *work)
>         default:
>                 break;
>         }
> +
> +       ib_device_put(&rdev->ibdev);
>         smp_mb__before_atomic();
>         atomic_dec(&rdev->sched_count);
>  exit:
>
> commit e64da98a182a2cae3338f28f6e581f189b5f8674
> Author: Jason Gunthorpe <jgg@ziepe.ca>
> Date:   Thu Jan 10 12:02:11 2019 -0700
>
>     RDMA/bnxt_re: Fix lifetimes in bnxt_re_task
>
>     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.
>
>     Also, every single work queue call has an allocated memory, and the kfree
>     of this memory was missed sometimes.
>
>     Fixes: 1ac5a4047975 ("RDMA/bnxt_re: Add bnxt_re RoCE driver")
>     Signed-off-by: Jason Gunthorpe <jgg@mellanox.com>
>
> diff --git a/drivers/infiniband/hw/bnxt_re/main.c b/drivers/infiniband/hw/bnxt_re/main.c
> index 814f959c7db965..666897596218d3 100644
> --- a/drivers/infiniband/hw/bnxt_re/main.c
> +++ b/drivers/infiniband/hw/bnxt_re/main.c
> @@ -1547,7 +1547,7 @@ static void bnxt_re_task(struct work_struct *work)
>
>         if (re_work->event != NETDEV_REGISTER &&
>             !test_bit(BNXT_RE_FLAG_IBDEV_REGISTERED, &rdev->flags))
> -               return;
> +               goto exit;
>
>         switch (re_work->event) {
>         case NETDEV_REGISTER:
> @@ -1582,6 +1582,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);
>  }
>
> @@ -1658,6 +1659,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 ?

^ permalink raw reply	[flat|nested] 6+ messages in thread
end of thread, other threads:[~2020-03-02  4:43 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-02-26 15:45 [PATCH for-next v4 0/2] RDMA/bnxt_re driver update Selvin Xavier
2020-02-26 15:45 ` [PATCH for-next v4 1/2] RDMA/bnxt_re: Refactor device add/remove functionalities Selvin Xavier
2020-02-26 15:45 ` [PATCH for-next v4 2/2] RDMA/bnxt_re: Use driver_unregister and unregistration API Selvin Xavier
2020-02-28 16:35   ` Jason Gunthorpe
2020-03-02  4:42     ` Selvin Xavier
2020-02-28 16:40 ` [PATCH for-next v4 0/2] RDMA/bnxt_re driver update Jason Gunthorpe

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.