Re: [PATCH for-next v4 2/2] RDMA/bnxt_re: Use driver_unregister and unregistration API

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Fri, Feb 28, 2020 at 10:05 PM Jason Gunthorpe <jgg@xxxxxxxx> 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@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 ?



[Index of Archives]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Photo]     [Yosemite News]     [Yosemite Photos]     [Linux Kernel]     [Linux SCSI]     [XFree86]

  Powered by Linux