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

David Box
2026-04-17T22:28:42Z

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@linux.intel.com>
> > ---
> >
> > 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.
>