Re: [PATCH v4 14/16] platform/x86/intel/pmc/ssram: Add ACPI discovery scaffolding

[David Box]

On Tue, May 19, 2026 at 03:46:46PM +0300, Ilpo Järvinen wrote:
> On Thu, 14 May 2026, David E. Box wrote:
> 
> > Prepare the SSRAM telemetry driver for ACPI-based discovery by adding the
> > common initialization path and selection framework needed for both PCI and
> > ACPI resource discovery.
> > 
> > At this stage, existing supported devices continue to use the PCI path.
> > This change lays the groundwork for follow-on patches that wire platform
> > IDs to the ACPI policy path.
> > 
> > Signed-off-by: Xi Pardee <xi.pardee@xxxxxxxxxxxxxxx>
> > Signed-off-by: David E. Box <david.e.box@xxxxxxxxxxxxxxx>
> > ---
> > V4 - Replaced local raw ACPI discovery pointer type u32 (*)[4] with
> >      acpi_disc_t in SSRAM ACPI initialization path.
> > 
> > V3 - No changes
> > 
> > V2 changes:
> >   - Fixed cleanup patterns using __free() attributes
> >   - Addressed Ilpo's recommendations for safer cleanup.h patterns
> > 
> >  .../platform/x86/intel/pmc/ssram_telemetry.c  | 70 +++++++++++++++++++
> >  1 file changed, 70 insertions(+)
> > 
> > diff --git a/drivers/platform/x86/intel/pmc/ssram_telemetry.c b/drivers/platform/x86/intel/pmc/ssram_telemetry.c
> > index 597bfb7ad822..1a9f1e67cbe1 100644
> > --- a/drivers/platform/x86/intel/pmc/ssram_telemetry.c
> > +++ b/drivers/platform/x86/intel/pmc/ssram_telemetry.c
> > @@ -5,6 +5,7 @@
> >   * Copyright (c) 2023, Intel Corporation.
> >   */
> >  
> > +#include <linux/acpi.h>
> >  #include <linux/bits.h>
> >  #include <linux/cleanup.h>
> >  #include <linux/device.h>
> > @@ -29,14 +30,17 @@ DEFINE_FREE(pmc_ssram_telemetry_iounmap, void __iomem *, if (_T) iounmap(_T))
> >  
> >  enum resource_method {
> >  	RES_METHOD_PCI,
> > +	RES_METHOD_ACPI,
> >  };
> >  
> >  struct ssram_type {
> >  	enum resource_method method;
> > +	enum pmc_index p_index;
> >  };
> >  
> >  static const struct ssram_type pci_main = {
> >  	.method = RES_METHOD_PCI,
> > +	.p_index = PMC_IDX_MAIN,
> >  };
> >  
> >  static struct pmc_ssram_telemetry pmc_ssram_telems[MAX_NUM_PMC];
> > @@ -149,6 +153,67 @@ static int pmc_ssram_telemetry_pci_init(struct pci_dev *pcidev)
> >  	return ret;
> >  }
> >  
> > +static int pmc_ssram_telemetry_get_pmc_acpi(struct pci_dev *pcidev,  unsigned int pmc_idx)
> > +{
> > +	u64 ssram_base;
> > +
> > +	ssram_base = pci_resource_start(pcidev, 0);
> > +	if (!ssram_base)
> > +		return -ENODEV;
> > +
> > +	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);
> > +
> > +	return 0;
> > +}
> > +
> > +static int pmc_ssram_telemetry_acpi_init(struct pci_dev *pcidev,
> > +					 enum pmc_index index)
> > +{
> > +	struct acpi_buffer buf = { ACPI_ALLOCATE_BUFFER, NULL };
> > +	acpi_handle handle = ACPI_HANDLE(&pcidev->dev);
> 
> Move the assignment next to !handle check.

Done.

> 
> > +	struct intel_vsec_header header;
> > +	struct intel_vsec_header *headers[2] = { &header, NULL };
> > +	struct intel_vsec_platform_info info = { };
> > +	void *dsd_buf __free(pmc_acpi_free) = buf.pointer;
> 
> Why is this here and not below the acpi function call?

Good catch. I had several of these and missed this one.

> 
> > +	union acpi_object *dsd;
> > +	acpi_status status;
> > +	int ret;
> > +
> > +	if (!handle)
> > +		return -ENODEV;
> > +
> > +	status = acpi_evaluate_object(handle, "_DSD", NULL, &buf);
> > +	if (ACPI_FAILURE(status))
> > +		return -ENODEV;
> > +
> > +	dsd = pmc_find_telem_guid(buf.pointer);
> 
> pmc_find_telem_guid(dsd_buf) ?

Yes. Switched in V5.

Thanks for the review.

David

> 
> > +	if (!dsd)
> > +		return -ENODEV;
> > +
> > +	acpi_disc_t disc __free(kfree) = pmc_parse_telem_dsd(dsd, &header);
> > +	if (IS_ERR(disc))
> > +		return PTR_ERR(disc);
> > +
> > +	info.headers = headers;
> > +	info.caps = VSEC_CAP_TELEMETRY;
> > +	info.acpi_disc = disc;
> > +	info.src = INTEL_VSEC_DISC_ACPI;
> > +
> > +	/* This is an ACPI companion device. PCI BAR will be used for base addr. */
> > +	info.base_addr = 0;
> > +
> > +	ret = intel_vsec_register(&pcidev->dev, &info);
> > +	if (ret)
> > +		return ret;
> > +
> > +	return pmc_ssram_telemetry_get_pmc_acpi(pcidev, index);
> > +}
> > +
> >  /**
> >   * pmc_ssram_telemetry_get_pmc_info() - Get a PMC devid and base_addr information
> >   * @pmc_idx:               Index of the PMC
> > @@ -189,6 +254,7 @@ static int pmc_ssram_telemetry_probe(struct pci_dev *pcidev, const struct pci_de
> >  {
> >  	const struct ssram_type *ssram_type;
> >  	enum resource_method method;
> > +	enum pmc_index index;
> >  	int ret;
> >  
> >  	ssram_type = (const struct ssram_type *)id->driver_data;
> > @@ -198,6 +264,7 @@ static int pmc_ssram_telemetry_probe(struct pci_dev *pcidev, const struct pci_de
> >  	}
> >  
> >  	method = ssram_type->method;
> > +	index = ssram_type->p_index;
> >  
> >  	ret = pcim_enable_device(pcidev);
> >  	if (ret) {
> > @@ -207,6 +274,8 @@ static int pmc_ssram_telemetry_probe(struct pci_dev *pcidev, const struct pci_de
> >  
> >  	if (method == RES_METHOD_PCI)
> >  		ret = pmc_ssram_telemetry_pci_init(pcidev);
> > +	else if (method == RES_METHOD_ACPI)
> > +		ret = pmc_ssram_telemetry_acpi_init(pcidev, index);
> >  	else
> >  		ret = -EINVAL;
> >  
> > @@ -239,6 +308,7 @@ static struct pci_driver pmc_ssram_telemetry_driver = {
> >  };
> >  module_pci_driver(pmc_ssram_telemetry_driver);
> >  
> > +MODULE_IMPORT_NS("INTEL_PMC_CORE");
> >  MODULE_IMPORT_NS("INTEL_VSEC");
> >  MODULE_AUTHOR("Xi Pardee <xi.pardee@xxxxxxxxx>");
> >  MODULE_DESCRIPTION("Intel PMC SSRAM Telemetry driver");
> > 
> 
> -- 
>  i.
>