Re: [PATCH V3 03/16] platform/x86/intel/vsec: Use cleanup.h

[David E. Box]

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@xxxxxxxxxxxxxxx>
> > ---
> > V3 - New patch.
> > 
> >  drivers/platform/x86/intel/vsec.c | 17 ++++++++++-------
> >  1 file changed, 10 insertions(+), 7 deletions(-)
> > 
> > diff --git a/drivers/platform/x86/intel/vsec.c
> > b/drivers/platform/x86/intel/vsec.c
> > index 15866b7d3117..f03ab89ab7c0 100644
> > --- a/drivers/platform/x86/intel/vsec.c
> > +++ b/drivers/platform/x86/intel/vsec.c
> > @@ -15,6 +15,7 @@
> >  
> >  #include <linux/auxiliary_bus.h>
> >  #include <linux/bits.h>
> > +#include <linux/cleanup.h>
> >  #include <linux/delay.h>
> >  #include <linux/kernel.h>
> >  #include <linux/idr.h>
> > @@ -155,10 +156,10 @@ EXPORT_SYMBOL_NS_GPL(intel_vsec_add_aux, INTEL_VSEC);
> >  static int intel_vsec_add_dev(struct pci_dev *pdev, struct
> > intel_vsec_header *header,
> >                               struct intel_vsec_platform_info *info)
> >  {
> > -       struct intel_vsec_device *intel_vsec_dev;
> > +       struct intel_vsec_device __free(kfree) *intel_vsec_dev = NULL;
> >         struct resource *res, *tmp;
> >         unsigned long quirks = info->quirks;
> > -       int i;
> > +       int ret, i;
> >  
> >         if (!intel_vsec_supported(header->id, info->caps))
> >                 return -EINVAL;
> > @@ -178,10 +179,8 @@ static int intel_vsec_add_dev(struct pci_dev *pdev,
> > struct intel_vsec_header *he
> >                 return -ENOMEM;
> >  
> >         res = kcalloc(header->num_entries, sizeof(*res), GFP_KERNEL);
> > -       if (!res) {
> > -               kfree(intel_vsec_dev);
> > +       if (!res)
> >                 return -ENOMEM;
> > -       }
> >  
> >         if (quirks & VSEC_QUIRK_TABLE_SHIFT)
> >                 header->offset >>= TABLE_OFFSET_SHIFT;
> > @@ -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.

David

>