* [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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).