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@xxxxxxxx>
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@xxxxxxxxxxxx>
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@xxxxxxxx>
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@xxxxxxxxxxxx>
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@xxxxxxxx>
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@xxxxxxxxxxxx>
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 ?
