[PATCH v3 01/11] platform/x86/intel: refactor endpoint usage
Ruhl, Michael J
michael.j.ruhl at intel.com
Fri Jun 6 19:20:00 UTC 2025
>-----Original Message-----
>From: David E. Box <david.e.box at linux.intel.com>
>Sent: Friday, June 6, 2025 1:55 PM
>To: Ruhl, Michael J <michael.j.ruhl at intel.com>; platform-driver-
>x86 at vger.kernel.org; intel-xe at lists.freedesktop.org; hdegoede at redhat.com;
>ilpo.jarvinen at linux.intel.com; De Marchi, Lucas <lucas.demarchi at intel.com>;
>Vivi, Rodrigo <rodrigo.vivi at intel.com>; thomas.hellstrom at linux.intel.com;
>airlied at gmail.com; simona at ffwll.ch
>Cc: stable at vger.kernel.org
>Subject: Re: [PATCH v3 01/11] platform/x86/intel: refactor endpoint usage
>
>Hi Mike,
>
>On Thu, 2025-06-05 at 14:44 -0400, Michael J. Ruhl wrote:
>> The use of an endpoint has introduced a dependency in all class/pmt
>> drivers to have an endpoint allocated.
>>
>> The telemetry driver has this allocation, the crashlog does not.
>>
>> The current usage is very telemetry focused, but should be common code.
>
>The endpoint exists specifically to support the exported APIs in the telemetry
>driver. It's reference-counted via kref to ensure safe cleanup once all API
>consumers are done. Unless the kernel needs to invoke a crashlog API through
>this mechanism, I’m not sure this change is necessary. I’ll go through the rest
>of the patches to understand how the endpoint is being used, but my initial
>reaction is that is not be needed.
>
>
>>
>> With this in mind:
>> rename the struct telemetry_endpoint to struct class_endpoint,
>> refactor the common endpoint code to be in the class.c module
>>
>> Fixes: 416eeb2e1fc7 ("platform/x86/intel/pmt: telemetry: Export API to read
>> telemetry")
>> Cc: <stable at vger.kernel.org>
>> Signed-off-by: Michael J. Ruhl <michael.j.ruhl at intel.com>
>> ---
>> drivers/platform/x86/intel/pmc/core.c | 3 +-
>> drivers/platform/x86/intel/pmc/core.h | 4 +-
>> drivers/platform/x86/intel/pmc/core_ssram.c | 2 +-
>> drivers/platform/x86/intel/pmt/class.c | 45 ++++++++++++++++++
>> drivers/platform/x86/intel/pmt/class.h | 21 +++++++--
>> drivers/platform/x86/intel/pmt/telemetry.c | 51 ++++-----------------
>> drivers/platform/x86/intel/pmt/telemetry.h | 23 ++++------
>> 7 files changed, 84 insertions(+), 65 deletions(-)
>>
>> diff --git a/drivers/platform/x86/intel/pmc/core.c
>> b/drivers/platform/x86/intel/pmc/core.c
>> index 7a1d11f2914f..805f56665d1d 100644
>> --- a/drivers/platform/x86/intel/pmc/core.c
>> +++ b/drivers/platform/x86/intel/pmc/core.c
>> @@ -29,6 +29,7 @@
>> #include <asm/tsc.h>
>>
>> #include "core.h"
>> +#include "../pmt/class.h"
>> #include "../pmt/telemetry.h"
>>
>> /* Maximum number of modes supported by platfoms that has low power
>mode
>> capability */
>> @@ -1198,7 +1199,7 @@ int get_primary_reg_base(struct pmc *pmc)
>>
>> void pmc_core_punit_pmt_init(struct pmc_dev *pmcdev, u32 guid)
>> {
>> - struct telem_endpoint *ep;
>> + struct class_endpoint *ep;
>
>I'd name it pmt_endpoint instead of class_endpoint.
Wil do.
>> struct pci_dev *pcidev;
>>
>> pcidev = pci_get_domain_bus_and_slot(0, 0, PCI_DEVFN(10, 0));
>> diff --git a/drivers/platform/x86/intel/pmc/core.h
>> b/drivers/platform/x86/intel/pmc/core.h
>> index 945a1c440cca..1c12ea7c3ce3 100644
>> --- a/drivers/platform/x86/intel/pmc/core.h
>> +++ b/drivers/platform/x86/intel/pmc/core.h
>> @@ -16,7 +16,7 @@
>> #include <linux/bits.h>
>> #include <linux/platform_device.h>
>>
>> -struct telem_endpoint;
>> +struct class_endpoint;
>>
>> #define SLP_S0_RES_COUNTER_MASK GENMASK(31,
>0)
>>
>> @@ -432,7 +432,7 @@ struct pmc_dev {
>>
>> bool has_die_c6;
>> u32 die_c6_offset;
>> - struct telem_endpoint *punit_ep;
>> + struct class_endpoint *punit_ep;
>> struct pmc_info *regmap_list;
>> };
>>
>> diff --git a/drivers/platform/x86/intel/pmc/core_ssram.c
>> b/drivers/platform/x86/intel/pmc/core_ssram.c
>> index 739569803017..3e670fc380a5 100644
>> --- a/drivers/platform/x86/intel/pmc/core_ssram.c
>> +++ b/drivers/platform/x86/intel/pmc/core_ssram.c
>> @@ -42,7 +42,7 @@ static u32 pmc_core_find_guid(struct pmc_info *list,
>const
>> struct pmc_reg_map *m
>>
>> static int pmc_core_get_lpm_req(struct pmc_dev *pmcdev, struct pmc *pmc)
>> {
>> - struct telem_endpoint *ep;
>> + struct class_endpoint *ep;
>> const u8 *lpm_indices;
>> int num_maps, mode_offset = 0;
>> int ret, mode;
>> diff --git a/drivers/platform/x86/intel/pmt/class.c
>> b/drivers/platform/x86/intel/pmt/class.c
>> index 7233b654bbad..bba552131bc2 100644
>> --- a/drivers/platform/x86/intel/pmt/class.c
>> +++ b/drivers/platform/x86/intel/pmt/class.c
>> @@ -76,6 +76,47 @@ int pmt_telem_read_mmio(struct pci_dev *pdev,
>struct
>> pmt_callbacks *cb, u32 guid
>> }
>> EXPORT_SYMBOL_NS_GPL(pmt_telem_read_mmio, "INTEL_PMT");
>>
>> +/* Called when all users unregister and the device is removed */
>> +static void pmt_class_ep_release(struct kref *kref)
>> +{
>> + struct class_endpoint *ep;
>> +
>> + ep = container_of(kref, struct class_endpoint, kref);
>> + kfree(ep);
>> +}
>> +
>> +void intel_pmt_release_endpoint(struct class_endpoint *ep)
>> +{
>> + kref_put(&ep->kref, pmt_class_ep_release);
>> +}
>> +EXPORT_SYMBOL_NS_GPL(intel_pmt_release_endpoint, "INTEL_PMT");
>> +
>> +int intel_pmt_add_endpoint(struct intel_vsec_device *ivdev,
>> + struct intel_pmt_entry *entry)
>> +{
>> + struct class_endpoint *ep;
>> +
>> + ep = kzalloc(sizeof(*ep), GFP_KERNEL);
>> + if (!ep)
>> + return -ENOMEM;
>> +
>> + ep->pcidev = ivdev->pcidev;
>> + ep->header.access_type = entry->header.access_type;
>> + ep->header.guid = entry->header.guid;
>> + ep->header.base_offset = entry->header.base_offset;
>> + ep->header.size = entry->header.size;
>> + ep->base = entry->base;
>> + ep->present = true;
>> + ep->cb = ivdev->priv_data;
>> +
>> + /* Endpoint lifetimes are managed by kref, not devres */
>> + kref_init(&ep->kref);
>> +
>> + entry->ep = ep;
>> +
>> + return 0;
>> +}
>> +EXPORT_SYMBOL_NS_GPL(intel_pmt_add_endpoint, "INTEL_PMT");
>> /*
>> * sysfs
>> */
>> @@ -97,6 +138,10 @@ intel_pmt_read(struct file *filp, struct kobject *kobj,
>> if (count > entry->size - off)
>> count = entry->size - off;
>>
>> + /* verify endpoint is available */
>> + if (!entry->ep)
>> + return -ENODEV;
>> +
>
>Hmm ...
>
>> count = pmt_telem_read_mmio(entry->ep->pcidev, entry->cb, entry-
>> >header.guid, buf,
>> entry->base, off, count);
>
>... intel_pmt_read() is only intended to handle sysfs reads, not to access
>driver endpoints. But entry->ep is a handle registered by a driver via
>pmt_telem_find_and_register_endpoint() which won’t be called unless another
>driver explicitly does so. If no driver registers the endpoint, entry->ep will
>be NULL, and this read path will dereference it, leading to a NULL pointer bug.
>
>This call to entry->ep->pcidev shouldn't be here. It mistakenly mixes the sysfs
>path with the driver API path. Actual use of entry->ep belongs only in the
>exported read calls in telemetry.c.
An additional issue here is that the callback interface requires the pcidev.
Is the pcidev available form a different location? (I am not seeing it...)
Maybe the pcidev * should be moved to the intel_pmt_entry struct?
Thanks,
Mike
>David
>
>>
>> diff --git a/drivers/platform/x86/intel/pmt/class.h
>> b/drivers/platform/x86/intel/pmt/class.h
>> index b2006d57779d..d2d8f9e31c9d 100644
>> --- a/drivers/platform/x86/intel/pmt/class.h
>> +++ b/drivers/platform/x86/intel/pmt/class.h
>> @@ -9,8 +9,6 @@
>> #include <linux/err.h>
>> #include <linux/io.h>
>>
>> -#include "telemetry.h"
>> -
>> /* PMT access types */
>> #define ACCESS_BARID 2
>> #define ACCESS_LOCAL 3
>> @@ -19,11 +17,19 @@
>> #define GET_BIR(v) ((v) & GENMASK(2, 0))
>> #define GET_ADDRESS(v) ((v) & GENMASK(31, 3))
>>
>> +struct kref;
>> struct pci_dev;
>>
>> -struct telem_endpoint {
>> +struct class_header {
>> + u8 access_type;
>> + u16 size;
>> + u32 guid;
>> + u32 base_offset;
>> +};
>> +
>> +struct class_endpoint {
>> struct pci_dev *pcidev;
>> - struct telem_header header;
>> + struct class_header header;
>> struct pmt_callbacks *cb;
>> void __iomem *base;
>> bool present;
>> @@ -38,7 +44,7 @@ struct intel_pmt_header {
>> };
>>
>> struct intel_pmt_entry {
>> - struct telem_endpoint *ep;
>> + struct class_endpoint *ep;
>> struct intel_pmt_header header;
>> struct bin_attribute pmt_bin_attr;
>> struct kobject *kobj;
>> @@ -69,4 +75,9 @@ int intel_pmt_dev_create(struct intel_pmt_entry
>*entry,
>> struct intel_vsec_device *dev, int idx);
>> void intel_pmt_dev_destroy(struct intel_pmt_entry *entry,
>> struct intel_pmt_namespace *ns);
>> +
>> +int intel_pmt_add_endpoint(struct intel_vsec_device *ivdev,
>> + struct intel_pmt_entry *entry);
>> +void intel_pmt_release_endpoint(struct class_endpoint *ep);
>> +
>> #endif
>> diff --git a/drivers/platform/x86/intel/pmt/telemetry.c
>> b/drivers/platform/x86/intel/pmt/telemetry.c
>> index ac3a9bdf5601..27d09867e6a3 100644
>> --- a/drivers/platform/x86/intel/pmt/telemetry.c
>> +++ b/drivers/platform/x86/intel/pmt/telemetry.c
>> @@ -18,6 +18,7 @@
>> #include <linux/overflow.h>
>>
>> #include "class.h"
>> +#include "telemetry.h"
>>
>> #define TELEM_SIZE_OFFSET 0x0
>> #define TELEM_GUID_OFFSET 0x4
>> @@ -93,48 +94,14 @@ static int pmt_telem_header_decode(struct
>intel_pmt_entry
>> *entry,
>> return 0;
>> }
>>
>> -static int pmt_telem_add_endpoint(struct intel_vsec_device *ivdev,
>> - struct intel_pmt_entry *entry)
>> -{
>> - struct telem_endpoint *ep;
>> -
>> - /* Endpoint lifetimes are managed by kref, not devres */
>> - entry->ep = kzalloc(sizeof(*(entry->ep)), GFP_KERNEL);
>> - if (!entry->ep)
>> - return -ENOMEM;
>> -
>> - ep = entry->ep;
>> - ep->pcidev = ivdev->pcidev;
>> - ep->header.access_type = entry->header.access_type;
>> - ep->header.guid = entry->header.guid;
>> - ep->header.base_offset = entry->header.base_offset;
>> - ep->header.size = entry->header.size;
>> - ep->base = entry->base;
>> - ep->present = true;
>> - ep->cb = ivdev->priv_data;
>> -
>> - kref_init(&ep->kref);
>> -
>> - return 0;
>> -}
>> -
>> static DEFINE_XARRAY_ALLOC(telem_array);
>> static struct intel_pmt_namespace pmt_telem_ns = {
>> .name = "telem",
>> .xa = &telem_array,
>> .pmt_header_decode = pmt_telem_header_decode,
>> - .pmt_add_endpoint = pmt_telem_add_endpoint,
>> + .pmt_add_endpoint = intel_pmt_add_endpoint,
>> };
>>
>> -/* Called when all users unregister and the device is removed */
>> -static void pmt_telem_ep_release(struct kref *kref)
>> -{
>> - struct telem_endpoint *ep;
>> -
>> - ep = container_of(kref, struct telem_endpoint, kref);
>> - kfree(ep);
>> -}
>> -
>> unsigned long pmt_telem_get_next_endpoint(unsigned long start)
>> {
>> struct intel_pmt_entry *entry;
>> @@ -155,7 +122,7 @@ unsigned long
>pmt_telem_get_next_endpoint(unsigned long
>> start)
>> }
>> EXPORT_SYMBOL_NS_GPL(pmt_telem_get_next_endpoint,
>"INTEL_PMT_TELEMETRY");
>>
>> -struct telem_endpoint *pmt_telem_register_endpoint(int devid)
>> +struct class_endpoint *pmt_telem_register_endpoint(int devid)
>> {
>> struct intel_pmt_entry *entry;
>> unsigned long index = devid;
>> @@ -174,9 +141,9 @@ struct telem_endpoint
>*pmt_telem_register_endpoint(int
>> devid)
>> }
>> EXPORT_SYMBOL_NS_GPL(pmt_telem_register_endpoint,
>"INTEL_PMT_TELEMETRY");
>>
>> -void pmt_telem_unregister_endpoint(struct telem_endpoint *ep)
>> +void pmt_telem_unregister_endpoint(struct class_endpoint *ep)
>> {
>> - kref_put(&ep->kref, pmt_telem_ep_release);
>> + intel_pmt_release_endpoint(ep);
>> }
>> EXPORT_SYMBOL_NS_GPL(pmt_telem_unregister_endpoint,
>"INTEL_PMT_TELEMETRY");
>>
>> @@ -206,7 +173,7 @@ int pmt_telem_get_endpoint_info(int devid, struct
>> telem_endpoint_info *info)
>> }
>> EXPORT_SYMBOL_NS_GPL(pmt_telem_get_endpoint_info,
>"INTEL_PMT_TELEMETRY");
>>
>> -int pmt_telem_read(struct telem_endpoint *ep, u32 id, u64 *data, u32
>count)
>> +int pmt_telem_read(struct class_endpoint *ep, u32 id, u64 *data, u32
>count)
>> {
>> u32 offset, size;
>>
>> @@ -226,7 +193,7 @@ int pmt_telem_read(struct telem_endpoint *ep, u32
>id, u64
>> *data, u32 count)
>> }
>> EXPORT_SYMBOL_NS_GPL(pmt_telem_read, "INTEL_PMT_TELEMETRY");
>>
>> -int pmt_telem_read32(struct telem_endpoint *ep, u32 id, u32 *data, u32
>count)
>> +int pmt_telem_read32(struct class_endpoint *ep, u32 id, u32 *data, u32
>count)
>> {
>> u32 offset, size;
>>
>> @@ -245,7 +212,7 @@ int pmt_telem_read32(struct telem_endpoint *ep,
>u32 id,
>> u32 *data, u32 count)
>> }
>> EXPORT_SYMBOL_NS_GPL(pmt_telem_read32, "INTEL_PMT_TELEMETRY");
>>
>> -struct telem_endpoint *
>> +struct class_endpoint *
>> pmt_telem_find_and_register_endpoint(struct pci_dev *pcidev, u32 guid,
>u16
>> pos)
>> {
>> int devid = 0;
>> @@ -279,7 +246,7 @@ static void pmt_telem_remove(struct
>auxiliary_device
>> *auxdev)
>> for (i = 0; i < priv->num_entries; i++) {
>> struct intel_pmt_entry *entry = &priv->entry[i];
>>
>> - kref_put(&entry->ep->kref, pmt_telem_ep_release);
>> + pmt_telem_unregister_endpoint(entry->ep);
>> intel_pmt_dev_destroy(entry, &pmt_telem_ns);
>> }
>> mutex_unlock(&ep_lock);
>> diff --git a/drivers/platform/x86/intel/pmt/telemetry.h
>> b/drivers/platform/x86/intel/pmt/telemetry.h
>> index d45af5512b4e..e987dd32a58a 100644
>> --- a/drivers/platform/x86/intel/pmt/telemetry.h
>> +++ b/drivers/platform/x86/intel/pmt/telemetry.h
>> @@ -2,6 +2,8 @@
>> #ifndef _TELEMETRY_H
>> #define _TELEMETRY_H
>>
>> +#include "class.h"
>> +
>> /* Telemetry types */
>> #define PMT_TELEM_TELEMETRY 0
>> #define PMT_TELEM_CRASHLOG 1
>> @@ -9,16 +11,9 @@
>> struct telem_endpoint;
>> struct pci_dev;
>>
>> -struct telem_header {
>> - u8 access_type;
>> - u16 size;
>> - u32 guid;
>> - u32 base_offset;
>> -};
>> -
>> struct telem_endpoint_info {
>> struct pci_dev *pdev;
>> - struct telem_header header;
>> + struct class_header header;
>> };
>>
>> /**
>> @@ -47,7 +42,7 @@ unsigned long
>pmt_telem_get_next_endpoint(unsigned long
>> start);
>> * * endpoint - On success returns pointer to the telemetry endpoint
>> * * -ENXIO - telemetry endpoint not found
>> */
>> -struct telem_endpoint *pmt_telem_register_endpoint(int devid);
>> +struct class_endpoint *pmt_telem_register_endpoint(int devid);
>>
>> /**
>> * pmt_telem_unregister_endpoint() - Unregister a telemetry endpoint
>> @@ -55,7 +50,7 @@ struct telem_endpoint
>*pmt_telem_register_endpoint(int
>> devid);
>> *
>> * Decrements the kref usage counter for the endpoint.
>> */
>> -void pmt_telem_unregister_endpoint(struct telem_endpoint *ep);
>> +void pmt_telem_unregister_endpoint(struct class_endpoint *ep);
>>
>> /**
>> * pmt_telem_get_endpoint_info() - Get info for an endpoint from its devid
>> @@ -80,8 +75,8 @@ int pmt_telem_get_endpoint_info(int devid, struct
>> telem_endpoint_info *info);
>> * * endpoint - On success returns pointer to the telemetry endpoint
>> * * -ENXIO - telemetry endpoint not found
>> */
>> -struct telem_endpoint *pmt_telem_find_and_register_endpoint(struct
>pci_dev
>> *pcidev,
>> - u32 guid, u16 pos);
>> +struct class_endpoint *pmt_telem_find_and_register_endpoint(struct
>pci_dev
>> *pcidev,
>> + u32 guid, u16
>> pos);
>>
>> /**
>> * pmt_telem_read() - Read qwords from counter sram using sample id
>> @@ -101,7 +96,7 @@ struct telem_endpoint
>> *pmt_telem_find_and_register_endpoint(struct pci_dev *pcid
>> * * -EPIPE - The device was removed during the read. Data written
>> * but should be considered invalid.
>> */
>> -int pmt_telem_read(struct telem_endpoint *ep, u32 id, u64 *data, u32
>count);
>> +int pmt_telem_read(struct class_endpoint *ep, u32 id, u64 *data, u32
>count);
>>
>> /**
>> * pmt_telem_read32() - Read qwords from counter sram using sample id
>> @@ -121,6 +116,6 @@ int pmt_telem_read(struct telem_endpoint *ep, u32
>id, u64
>> *data, u32 count);
>> * * -EPIPE - The device was removed during the read. Data written
>> * but should be considered invalid.
>> */
>> -int pmt_telem_read32(struct telem_endpoint *ep, u32 id, u32 *data, u32
>> count);
>> +int pmt_telem_read32(struct class_endpoint *ep, u32 id, u32 *data, u32
>> count);
>>
>> #endif
More information about the Intel-xe
mailing list