Re: [PATCH for-next 2/7] RDMA/bnxt_re: Replace chip context structure with pointer

[Devesh Sharma <devesh.sharma@broadcom.com>]

On Sat, Jan 25, 2020 at 11:33 PM Jason Gunthorpe <jgg@mellanox.com> wrote:
>
> On Fri, Jan 24, 2020 at 12:52:40AM -0500, Devesh Sharma wrote:
> >  static void bnxt_re_destroy_chip_ctx(struct bnxt_re_dev *rdev)
> >  {
> > +     struct bnxt_qplib_chip_ctx *chip_ctx;
> > +
> > +     if (!rdev->chip_ctx)
> > +             return;
> > +     chip_ctx = rdev->chip_ctx;
> > +     rdev->chip_ctx = NULL;
> >       rdev->rcfw.res = NULL;
> >       rdev->qplib_res.cctx = NULL;
> > +     kfree(chip_ctx);
> >  }
>
> Are you sure this kfree is late enough? I couldn't deduce if it was
> really safe to NULL chip_ctx here.
With the current design its okay to free this here because
bnxt_re_destroy_chip_ctx is indeed the last deallocation performed
before ib_device_dealloc() in any exit path. Further, the call to
bnxt_re_destroy_chip_ctx is protected by rtnl.
following is the exit sequence anyewere in the driver control path
bnxt_re_ib_unreg(rdev); --->> the last deallocation in this func is
destroy_chip_ctx().
bnxt_re_remove_one(rdev); -->> this is a single line function just to
put pci device reference
bnxt_re_dev_unreg(rdev); -->> the first deallocation in this func is
ib_device_dealloc().


>
> IMHO you should free it as part of the ib device dealloc.
>
> Jason