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

[Ilpo Järvinen]

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

-- 
 i.