Re: [PATCH V2 14/17] platform/x86/intel/pmc/ssram_telemetry: Fix cleanup pattern for __free() variables

[David Box]

On Tue, Apr 07, 2026 at 04:33:06PM +0300, Ilpo Järvinen wrote:
> On Tue, 24 Mar 2026, David E. Box wrote:
> 
> > Fix improper cleanup.h usage where __free() variables were initialized to
> > NULL and then assigned later. Move ssram variable declarations into the
> > if/else branches where they're actually assigned to follow the safer
> > pattern recommended in cleanup.h.  This change requires also moving the
> > pmc_ssram_get_devid_pwrmbase() and add_pmt calls into both if/else branches
> > to keep operations within the scope of the local ssram variables.
> > 
> > Signed-off-by: David E. Box <david.e.box@xxxxxxxxxxxxxxx>
> > ---
> > 
> > V2 changes:
> > - New patch addressing Ilpo's review of cleanup.h patterns
> > 
> >  .../platform/x86/intel/pmc/ssram_telemetry.c  | 25 +++++++++++--------
> >  1 file changed, 15 insertions(+), 10 deletions(-)
> > 
> > diff --git a/drivers/platform/x86/intel/pmc/ssram_telemetry.c b/drivers/platform/x86/intel/pmc/ssram_telemetry.c
> > index b329e0c0080b..b1ba17f18ea5 100644
> > --- a/drivers/platform/x86/intel/pmc/ssram_telemetry.c
> > +++ b/drivers/platform/x86/intel/pmc/ssram_telemetry.c
> > @@ -102,12 +102,11 @@ pmc_ssram_telemetry_add_pmt(struct pci_dev *pcidev, u64 ssram_base, void __iomem
> >  static int
> >  pmc_ssram_telemetry_get_pmc_pci(struct pci_dev *pcidev, unsigned int pmc_idx, u32 offset)
> >  {
> > -	void __iomem __free(pmc_ssram_telemetry_iounmap) *tmp_ssram = NULL;
> > -	void __iomem __free(pmc_ssram_telemetry_iounmap) *ssram = NULL;
> >  	u64 ssram_base;
> >  
> >  	ssram_base = pci_resource_start(pcidev, 0);
> > -	tmp_ssram = ioremap(ssram_base, SSRAM_HDR_SIZE);
> > +	void __iomem __free(pmc_ssram_telemetry_iounmap) *tmp_ssram =
> > +		ioremap(ssram_base, SSRAM_HDR_SIZE);
> >  	if (!tmp_ssram)
> >  		return -ENOMEM;
> >  
> > @@ -121,18 +120,24 @@ pmc_ssram_telemetry_get_pmc_pci(struct pci_dev *pcidev, unsigned int pmc_idx, u3
> >  		if (!ssram_base)
> >  			return 0;
> >  
> > -		ssram = ioremap(ssram_base, SSRAM_HDR_SIZE);
> > +		void __iomem __free(pmc_ssram_telemetry_iounmap) *ssram =
> > +			ioremap(ssram_base, SSRAM_HDR_SIZE);
> >  		if (!ssram)
> >  			return -ENOMEM;
> >  
> > +		pmc_ssram_get_devid_pwrmbase(ssram, pmc_idx);
> > +
> > +		/* Find and register and PMC telemetry entries */
> > +		return pmc_ssram_telemetry_add_pmt(pcidev, ssram_base, ssram);
> >  	} else {
> > -		ssram = no_free_ptr(tmp_ssram);
> > +		void __iomem __free(pmc_ssram_telemetry_iounmap) *ssram =
> > +			no_free_ptr(tmp_ssram);
> > +
> > +		pmc_ssram_get_devid_pwrmbase(ssram, pmc_idx);
> > +
> > +		/* Find and register and PMC telemetry entries */
> > +		return pmc_ssram_telemetry_add_pmt(pcidev, ssram_base, ssram);
> >  	}
> > -
> > -	pmc_ssram_get_devid_pwrmbase(ssram, pmc_idx);
> > -
> > -	/* Find and register and PMC telemetry entries */
> > -	return pmc_ssram_telemetry_add_pmt(pcidev, ssram_base, ssram);
> 
> Hi,
> 
> I'm sorry you probably made this because of my request, but this doesn't 
> make things simpler so lets stick with having that = NULL outside of the 
> inner blocks.
> 
> You could have overruled me at your own discretion when you realized you 
> have to start duplicating code (and just mention that in the coverletter).

Thanks. I'll change it back and remember that.

David
> 
> -- 
>  i.
>