[PATCH v3 01/11] platform/x86/intel: refactor endpoint usage
David E. Box
david.e.box at linux.intel.com
Mon Jun 9 17:04:33 UTC 2025
On Fri, 2025-06-06 at 19:20 +0000, Ruhl, Michael J wrote:
> > -----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?
Yes. After looking through this series, I don't see a need for this patch to
extend telem_enpoint for general use. Let's just place a copy of the pdev in
entry. Then you can drop the first two patches.
David
>
> 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