[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <2bf861c9-456a-cf42-9777-1641f6ab87b5@linux.intel.com>
Date: Mon, 16 Oct 2023 15:02:24 +0300 (EEST)
From: Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
To: "David E. Box" <david.e.box@...ux.intel.com>
cc: LKML <linux-kernel@...r.kernel.org>,
platform-driver-x86@...r.kernel.org, rajvi.jingar@...ux.intel.com
Subject: Re: [PATCH V3 03/16] platform/x86/intel/vsec: Use cleanup.h
On Fri, 13 Oct 2023, David E. Box wrote:
> On Fri, 2023-10-13 at 13:54 +0300, Ilpo Järvinen wrote:
> > On Thu, 12 Oct 2023, David E. Box wrote:
> >
> > > On Thu, 2023-10-12 at 17:46 +0300, Ilpo Järvinen wrote:
> > > > On Wed, 11 Oct 2023, David E. Box wrote:
> > > >
> > > > > Use cleanup.h helpers to handle cleanup of resources in
> > > > > intel_vsec_add_dev() after failures.
> > > > >
> > > > > Signed-off-by: David E. Box <david.e.box@...ux.intel.com>
> > > > > ---
> > > > > @@ -208,8 +207,12 @@ static int intel_vsec_add_dev(struct pci_dev *pdev,
> > > > > struct intel_vsec_header *he
> > > > > else
> > > > > intel_vsec_dev->ida = &intel_vsec_ida;
> > > > >
> > > > > - return intel_vsec_add_aux(pdev, NULL, intel_vsec_dev,
> > > > > - intel_vsec_name(header->id));
> > > > > + ret = intel_vsec_add_aux(pdev, NULL, intel_vsec_dev,
> > > > > + intel_vsec_name(header->id));
> > > > > +
> > > > > + no_free_ptr(intel_vsec_dev);
> > > > > +
> > > > > + return ret;
> > > >
> > > > So if intel_vsec_add_aux() returned an error, intel_vsec_dev won't be
> > > > freed because of the call call to no_free_ptr() before return. I that's
> > > > not what you intended?
> > >
> > > It will have been freed if intel_vsec_add_aux() fails. It's a little messy.
> > > That
> > > function creates the auxdev and assigns the release function which will free
> > > intel_vsec_dev when the device is removed. But there are failure points that
> > > will also invoke the release function. Because of this, for all the failure
> > > points in that function we free intel_vsec_dev so that it's state doesn't
> > > need
> > > to be questioned here. This happens explicitly if the failure is before
> > > auxiliary_device_init(), or through the release function invoked by
> > > auxiliary_device_uninit() if after.
> >
> > Oh, that's really convoluted and no wonder I missed it completely. It
> > might even be that using cleanup.h here isn't really worth it. I know
> > I pushed you into that direction but I didn't realize all the complexity
> > at that point.
...
> > are before the intel_vsec_add_aux() call (and I'd also add a comment
> > to explain that freeing them is now responsability of
> > intel_vsec_add_aux()). That way, we don't leave a trap into there where
> > somebody happily adds another no_free_ptr() to the same group and it
> > causes a mem leak.
>
> Sure. After the comment I'd just do this then still the value is still needed,
>
> ret = intel_vsec_add_aux(pdev, NULL, no_free_ptr(intel_vsec_dev),
> intel_vsec_name(header->id));
True, I realized later that the variable gets NULLed because of how
no_free_ptr() works so no_free_ptr() has to be within the call itself, but
that's actually much better than my initial suggestion!
So I think the best we can get out of this is along the lines of (with the
subsequent change with res too):
/* Pass the ownership of intel_vsec_dev and resource within it to intel_vsec_add_aux() */
no_free_ptr(res);
return intel_vsec_add_aux(pdev, info->parent, no_free_ptr(intel_vsec_dev),
intel_vsec_name(header->id));
That seems the least trappiest and actually nicely documents who is
responsible for what. To contrast the earlier, this feels very elegant,
the perceived complexity related to intel_vsec_add_aux() no longer feels
tricky at all so we end up solving also that problem better than the
original.
--
i.
Powered by blists - more mailing lists