diff mbox series

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

Message ID 20231012023840.3845703-4-david.e.box@linux.intel.com
State New
Headers
Series intel_pmc: Add telemetry API to read counters |

Commit Message

David E. Box Oct. 12, 2023, 2:38 a.m. UTC 
  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@linux.intel.com>
---
V3 - New patch.

 drivers/platform/x86/intel/vsec.c | 17 ++++++++++-------
 1 file changed, 10 insertions(+), 7 deletions(-)

Comments

kernel test robot Oct. 12, 2023, 5:25 a.m. UTC | #1 
Hi David,

kernel test robot noticed the following build warnings:

[auto build test WARNING on acce85a7dd28eac3858d44230f4c65985d0f271c]

url:    https://github.com/intel-lab-lkp/linux/commits/David-E-Box/platform-x86-intel-vsec-Move-structures-to-header/20231012-104217
base:   acce85a7dd28eac3858d44230f4c65985d0f271c
patch link:    https://lore.kernel.org/r/20231012023840.3845703-4-david.e.box%40linux.intel.com
patch subject: [PATCH V3 03/16] platform/x86/intel/vsec: Use cleanup.h
reproduce: (https://download.01.org/0day-ci/archive/20231012/202310121314.3Xpqom2w-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202310121314.3Xpqom2w-lkp@intel.com/

# many are suggestions rather than must-fix

ERROR:SPACING: need consistent spacing around '*' (ctx:WxV)
#31: FILE: drivers/platform/x86/intel/vsec.c:159:
+	struct intel_vsec_device __free(kfree) *intel_vsec_dev = NULL;
 	                                       ^
kernel test robot Oct. 12, 2023, 5:48 a.m. UTC | #2 
Hi David,

kernel test robot noticed the following build warnings:

[auto build test WARNING on acce85a7dd28eac3858d44230f4c65985d0f271c]

url:    https://github.com/intel-lab-lkp/linux/commits/David-E-Box/platform-x86-intel-vsec-Move-structures-to-header/20231012-104217
base:   acce85a7dd28eac3858d44230f4c65985d0f271c
patch link:    https://lore.kernel.org/r/20231012023840.3845703-4-david.e.box%40linux.intel.com
patch subject: [PATCH V3 03/16] platform/x86/intel/vsec: Use cleanup.h
reproduce: (https://download.01.org/0day-ci/archive/20231012/202310121333.TE7R7kSs-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202310121333.TE7R7kSs-lkp@intel.com/

# many are suggestions rather than must-fix

ERROR:SPACING: need consistent spacing around '*' (ctx:WxV)
#31: FILE: drivers/platform/x86/intel/vsec.c:159:
+	struct intel_vsec_device __free(kfree) *intel_vsec_dev = NULL;
 	                                       ^
Ilpo Järvinen Oct. 12, 2023, 2:46 p.m. UTC | #3 
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@linux.intel.com>
> ---
> 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?
David E. Box Oct. 12, 2023, 5:13 p.m. UTC | #4 
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@linux.intel.com>
> > ---
> > 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

>
David E. Box Oct. 12, 2023, 5:23 p.m. UTC | #5 
On Thu, 2023-10-12 at 13:25 +0800, kernel test robot wrote:
> Hi David,
> 
> kernel test robot noticed the following build warnings:
> 
> [auto build test WARNING on acce85a7dd28eac3858d44230f4c65985d0f271c]
> 
> url:   
> https://github.com/intel-lab-lkp/linux/commits/David-E-Box/platform-x86-intel-vsec-Move-structures-to-header/20231012-104217
> base:   acce85a7dd28eac3858d44230f4c65985d0f271c
> patch link:   
> https://lore.kernel.org/r/20231012023840.3845703-4-david.e.box%40linux.intel.com
> patch subject: [PATCH V3 03/16] platform/x86/intel/vsec: Use cleanup.h
> reproduce:
> (https://download.01.org/0day-ci/archive/20231012/202310121314.3Xpqom2w-lkp@in
> tel.com/reproduce)
> 
> If you fix the issue in a separate patch/commit (i.e. not just a new version
> of
> the same patch/commit), kindly add following tags
> > Reported-by: kernel test robot <lkp@intel.com>
> > Closes:
> > https://lore.kernel.org/oe-kbuild-all/202310121314.3Xpqom2w-lkp@intel.com/
> 
> # many are suggestions rather than must-fix
> 
> ERROR:SPACING: need consistent spacing around '*' (ctx:WxV)
> #31: FILE: drivers/platform/x86/intel/vsec.c:159:
> +       struct intel_vsec_device __free(kfree) *intel_vsec_dev = NULL;

These looks like false positives.

David

>                                                ^
>
Ilpo Järvinen Oct. 13, 2023, 10:39 a.m. UTC | #6 
Hi,

I added checkpatch people, please check what looks a false positive below.

On Thu, 12 Oct 2023, David E. Box wrote:

> On Thu, 2023-10-12 at 13:25 +0800, kernel test robot wrote:
> > Hi David,
> > 
> > kernel test robot noticed the following build warnings:
> > 
> > [auto build test WARNING on acce85a7dd28eac3858d44230f4c65985d0f271c]
> > 
> > url:   
> > https://github.com/intel-lab-lkp/linux/commits/David-E-Box/platform-x86-intel-vsec-Move-structures-to-header/20231012-104217
> > base:   acce85a7dd28eac3858d44230f4c65985d0f271c
> > patch link:   
> > https://lore.kernel.org/r/20231012023840.3845703-4-david.e.box%40linux.intel.com
> > patch subject: [PATCH V3 03/16] platform/x86/intel/vsec: Use cleanup.h
> > reproduce:
> > (https://download.01.org/0day-ci/archive/20231012/202310121314.3Xpqom2w-lkp@in
> > tel.com/reproduce)
> > 
> > If you fix the issue in a separate patch/commit (i.e. not just a new version
> > of
> > the same patch/commit), kindly add following tags
> > > Reported-by: kernel test robot <lkp@intel.com>
> > > Closes:
> > > https://lore.kernel.org/oe-kbuild-all/202310121314.3Xpqom2w-lkp@intel.com/
> > 
> > # many are suggestions rather than must-fix
> > 
> > ERROR:SPACING: need consistent spacing around '*' (ctx:WxV)
> > #31: FILE: drivers/platform/x86/intel/vsec.c:159:
> > +       struct intel_vsec_device __free(kfree) *intel_vsec_dev = NULL;
> 
> These looks like false positives.

I agree. If I interpret the error message right checkpatch seems to think
that's a multiplication which is not the case here.
Ilpo Järvinen Oct. 13, 2023, 10:54 a.m. UTC | #7 
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@linux.intel.com>
> > > ---
> > > 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.

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.

If you still want to keep using cleanup.h, it would perhaps be less 
dangerous if you change the code such that no_free_ptr() for
intel_vsec_dev and the resource (in 4/16, that's a similar case, isn't 
it?) 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.
Joe Perches Oct. 13, 2023, 6:14 p.m. UTC | #8 
On Fri, 2023-10-13 at 13:39 +0300, Ilpo Järvinen wrote:
> Hi,
> 
> I added checkpatch people, please check what looks a false positive below.

Yeah, thanks I guess.
The __free uses are very new.
I'll play around with adding it to $Attributes
and see what happens.


> On Thu, 12 Oct 2023, David E. Box wrote:
> > On Thu, 2023-10-12 at 13:25 +0800, kernel test robot wrote:
> > > kernel test robot noticed the following build warnings:
> > > # many are suggestions rather than must-fix
[]
> > > ERROR:SPACING: need consistent spacing around '*' (ctx:WxV)
> > > #31: FILE: drivers/platform/x86/intel/vsec.c:159:
> > > +       struct intel_vsec_device __free(kfree) *intel_vsec_dev = NULL;
> > 
> > These looks like false positives.
> 
> I agree. If I interpret the error message right checkpatch seems to think
> that's a multiplication which is not the case here.
David E. Box Oct. 13, 2023, 10:16 p.m. UTC | #9 
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@linux.intel.com>
> > > > ---
> > > > 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.
> 
> 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.
> 
> If you still want to keep using cleanup.h, it would perhaps be less 
> dangerous if you change the code such that no_free_ptr() for
> intel_vsec_dev and the resource (in 4/16, that's a similar case, isn't 
> it?)

yes

>  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));

David
Ilpo Järvinen Oct. 16, 2023, 12:02 p.m. UTC | #10 
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@linux.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.
Joe Perches Oct. 24, 2023, 5:15 a.m. UTC | #11 
On Fri, 2023-10-13 at 11:14 -0700, Joe Perches wrote:
> On Fri, 2023-10-13 at 13:39 +0300, Ilpo Järvinen wrote:
> > Hi,
> > 
> > I added checkpatch people, please check what looks a false positive below.
> 
> Yeah, thanks I guess.
> The __free uses are very new.
> I'll play around with adding it to $Attributes
> and see what happens.
> 
> 
> > On Thu, 12 Oct 2023, David E. Box wrote:
> > > On Thu, 2023-10-12 at 13:25 +0800, kernel test robot wrote:
> > > > kernel test robot noticed the following build warnings:
> > > > # many are suggestions rather than must-fix
> []
> > > > ERROR:SPACING: need consistent spacing around '*' (ctx:WxV)
> > > > #31: FILE: drivers/platform/x86/intel/vsec.c:159:
> > > > +       struct intel_vsec_device __free(kfree) *intel_vsec_dev = NULL;
> > > .
> > > These looks like false positives.
> > 
> > I agree. If I interpret the error message right checkpatch seems to think
> > that's a multiplication which is not the case here.

So __free is an odd $Attribute as it takes an argument.
I've added it along with the other odd $Attribute __alloc_size
to another variable called $ArgAttribute and $Attribute.

Oddly, I don't understand why perl does not perform
the elimination using

	$foo =~ s/\b$ArgAttribute\b//g;

but does do the elimination using

	$foo =~ s/\b$ArgAttribute//g;

maybe something to do with the closing parenthesis in the match.

So now there is a separate substitution for this in a test.

Comments?

---
 scripts/checkpatch.pl | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 25fdb7fda1128..501383fa31c55 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -488,9 +488,14 @@ our $InitAttributeConst = qr{$InitAttributePrefix(?:initconst\b)};
 our $InitAttributeInit = qr{$InitAttributePrefix(?:init\b)};
 our $InitAttribute = qr{$InitAttributeData|$InitAttributeConst|$InitAttributeInit};
 
+our $ArgAttribute = qr{(?x:
+			__alloc_size\s*\(\s*\d+\s*(?:,\s*\d+\s*)?\)|
+			__free\s*\(\s*\w+\s*\)
+)};
+
 # Notes to $Attribute:
 # We need \b after 'init' otherwise 'initconst' will cause a false positive in a check
-our $Attribute	= qr{
+our $Attribute	= qr{(?x:
 			const|
 			volatile|
 			__percpu|
@@ -516,8 +521,8 @@ our $Attribute	= qr{
 			____cacheline_aligned_in_smp|
 			____cacheline_internodealigned_in_smp|
 			__weak|
-			__alloc_size\s*\(\s*\d+\s*(?:,\s*\d+\s*)?\)
-		  }x;
+			$ArgAttribute
+		  )};
 our $Modifier;
 our $Inline	= qr{inline|__always_inline|noinline|__inline|__inline__};
 our $Member	= qr{->$Ident|\.$Ident|\[[^]]*\]};
@@ -4091,6 +4096,9 @@ sub process {
 			# remove $Attribute/$Sparse uses to simplify comparisons
 			$sl =~ s/\b(?:$Attribute|$Sparse)\b//g;
 			$pl =~ s/\b(?:$Attribute|$Sparse)\b//g;
+			$sl =~ s/\b(?:$ArgAttribute)//g;
+			$pl =~ s/\b(?:$ArgAttribute)//g;
+
 			if (($pl =~ /^\+\s+$Declare\s*$Ident\s*[=,;:\[]/ ||
 			# function pointer declarations
 			     $pl =~ /^\+\s+$Declare\s*\(\s*\*\s*$Ident\s*\)\s*[=,;:\[\(]/ ||
diff mbox series

Patch

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;
 }
 
 static bool intel_vsec_walk_header(struct pci_dev *pdev,