On Wed, Jan 29, 2020 at 1:45 AM Jason Gunthorpe <jgg@xxxxxxxxxxxx> wrote:
>
> On Mon, Jan 27, 2020 at 01:09:46PM +0530, Devesh Sharma wrote:
> > On Sat, Jan 25, 2020 at 11:33 PM Jason Gunthorpe <jgg@xxxxxxxxxxxx> 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().
>
> It makes more sense to me to put all the memory deallocation together
> in one place, then there is no concern about ordering.
>
> We now have the dealloc_driver callback for this purpose.
>
> It is not 'last deallocation' that matters, but what all the other
> stuff is doing between destroy_chip_ctx() and ib_device_dealloc()
As far as this series is concerned, driver is saving crashes however
in a nasty way. I would like to move forward with what I have in this
patch and submit a new patch series which would implement your
suggestion.
>
> Jason
