| Message ID | 20231018231624.1044633-4-david.e.box@linux.intel.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | intel_pmc: Add telemetry API to read counters | expand |
Hi David, kernel test robot noticed the following build warnings: [auto build test WARNING on 3f720b21ec5af466e50e99dc517af267b67d248c] url: https://github.com/intel-lab-lkp/linux/commits/David-E-Box/platform-x86-intel-vsec-Move-structures-to-header/20231019-071914 base: 3f720b21ec5af466e50e99dc517af267b67d248c patch link: https://lore.kernel.org/r/20231018231624.1044633-4-david.e.box%40linux.intel.com patch subject: [PATCH V4 03/17] platform/x86/intel/vsec: Use cleanup.h reproduce: (https://download.01.org/0day-ci/archive/20231019/202310190857.Vc2mlWmN-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/202310190857.Vc2mlWmN-lkp@intel.com/ # many are suggestions rather than must-fix ERROR:SPACING: need consistent spacing around '*' (ctx:WxV) #32: FILE: drivers/platform/x86/intel/vsec.c:151: + struct intel_vsec_device __free(kfree) *intel_vsec_dev = NULL; ^ ERROR:SPACING: need consistent spacing around '*' (ctx:WxV) #33: FILE: drivers/platform/x86/intel/vsec.c:152: + struct resource __free(kfree) *res = NULL; ^
On Wed, 18 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> > --- > V4 - Do no_free_ptr() before and in call to intel_vsec_add_aux(). > - Add resource cleanup in this patch. > > V3 - New patch. > > drivers/platform/x86/intel/vsec.c | 23 +++++++++++++++-------- > 1 file changed, 15 insertions(+), 8 deletions(-) > > diff --git a/drivers/platform/x86/intel/vsec.c b/drivers/platform/x86/intel/vsec.c > index b14eba545770..28191313d515 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> > @@ -147,10 +148,11 @@ 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 resource *res, *tmp; > + struct intel_vsec_device __free(kfree) *intel_vsec_dev = NULL; > + struct resource __free(kfree) *res = NULL; > + struct resource *tmp; > unsigned long quirks = info->quirks; > - int i; > + int ret, i; > > if (!intel_vsec_supported(header->id, info->caps)) > return -EINVAL; > @@ -170,10 +172,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; > @@ -200,8 +200,15 @@ 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)); > + /* > + * Pass the ownership of intel_vsec_dev and resource within it to > + * intel_vsec_add_aux() > + */ > + no_free_ptr(res); > + ret = intel_vsec_add_aux(pdev, NULL, no_free_ptr(intel_vsec_dev), > + intel_vsec_name(header->id)); > + > + return ret; This looks good other than you don't need to add the ret variable at all. After changing that, please add: Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
diff --git a/drivers/platform/x86/intel/vsec.c b/drivers/platform/x86/intel/vsec.c index b14eba545770..28191313d515 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> @@ -147,10 +148,11 @@ 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 resource *res, *tmp; + struct intel_vsec_device __free(kfree) *intel_vsec_dev = NULL; + struct resource __free(kfree) *res = NULL; + struct resource *tmp; unsigned long quirks = info->quirks; - int i; + int ret, i; if (!intel_vsec_supported(header->id, info->caps)) return -EINVAL; @@ -170,10 +172,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; @@ -200,8 +200,15 @@ 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)); + /* + * Pass the ownership of intel_vsec_dev and resource within it to + * intel_vsec_add_aux() + */ + no_free_ptr(res); + ret = intel_vsec_add_aux(pdev, NULL, no_free_ptr(intel_vsec_dev), + intel_vsec_name(header->id)); + + return ret; } static bool intel_vsec_walk_header(struct pci_dev *pdev,
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> --- V4 - Do no_free_ptr() before and in call to intel_vsec_add_aux(). - Add resource cleanup in this patch. V3 - New patch. drivers/platform/x86/intel/vsec.c | 23 +++++++++++++++-------- 1 file changed, 15 insertions(+), 8 deletions(-)