* [PATCH v3 01/11] platform/x86/intel: refactor endpoint usage [not found] <20250605184444.515556-1-michael.j.ruhl@intel.com> @ 2025-06-05 18:44 ` Michael J. Ruhl 2025-06-06 17:54 ` David E. Box 2025-06-05 18:44 ` [PATCH v3 02/11] platform/x86/intel/pmt: crashlog binary file endpoint Michael J. Ruhl 1 sibling, 1 reply; 8+ messages in thread From: Michael J. Ruhl @ 2025-06-05 18:44 UTC (permalink / raw) To: platform-driver-x86, intel-xe, hdegoede, ilpo.jarvinen, lucas.demarchi, rodrigo.vivi, thomas.hellstrom, airlied, simona, david.e.box Cc: Michael J. Ruhl, stable 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. 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@vger.kernel.org> Signed-off-by: Michael J. Ruhl <michael.j.ruhl@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; 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; + count = pmt_telem_read_mmio(entry->ep->pcidev, entry->cb, entry->header.guid, buf, entry->base, off, count); 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 -- 2.49.0 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH v3 01/11] platform/x86/intel: refactor endpoint usage 2025-06-05 18:44 ` [PATCH v3 01/11] platform/x86/intel: refactor endpoint usage Michael J. Ruhl @ 2025-06-06 17:54 ` David E. Box 2025-06-06 19:20 ` Ruhl, Michael J 0 siblings, 1 reply; 8+ messages in thread From: David E. Box @ 2025-06-06 17:54 UTC (permalink / raw) To: Michael J. Ruhl, platform-driver-x86, intel-xe, hdegoede, ilpo.jarvinen, lucas.demarchi, rodrigo.vivi, thomas.hellstrom, airlied, simona Cc: stable 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@vger.kernel.org> > Signed-off-by: Michael J. Ruhl <michael.j.ruhl@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. > 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. 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* RE: [PATCH v3 01/11] platform/x86/intel: refactor endpoint usage 2025-06-06 17:54 ` David E. Box @ 2025-06-06 19:20 ` Ruhl, Michael J 2025-06-09 17:04 ` David E. Box 0 siblings, 1 reply; 8+ messages in thread From: Ruhl, Michael J @ 2025-06-06 19:20 UTC (permalink / raw) To: david.e.box@linux.intel.com, platform-driver-x86@vger.kernel.org, intel-xe@lists.freedesktop.org, hdegoede@redhat.com, ilpo.jarvinen@linux.intel.com, De Marchi, Lucas, Vivi, Rodrigo, thomas.hellstrom@linux.intel.com, airlied@gmail.com, simona@ffwll.ch Cc: stable@vger.kernel.org >-----Original Message----- >From: David E. Box <david.e.box@linux.intel.com> >Sent: Friday, June 6, 2025 1:55 PM >To: Ruhl, Michael J <michael.j.ruhl@intel.com>; platform-driver- >x86@vger.kernel.org; intel-xe@lists.freedesktop.org; hdegoede@redhat.com; >ilpo.jarvinen@linux.intel.com; De Marchi, Lucas <lucas.demarchi@intel.com>; >Vivi, Rodrigo <rodrigo.vivi@intel.com>; thomas.hellstrom@linux.intel.com; >airlied@gmail.com; simona@ffwll.ch >Cc: stable@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@vger.kernel.org> >> Signed-off-by: Michael J. Ruhl <michael.j.ruhl@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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v3 01/11] platform/x86/intel: refactor endpoint usage 2025-06-06 19:20 ` Ruhl, Michael J @ 2025-06-09 17:04 ` David E. Box 0 siblings, 0 replies; 8+ messages in thread From: David E. Box @ 2025-06-09 17:04 UTC (permalink / raw) To: Ruhl, Michael J, platform-driver-x86@vger.kernel.org, intel-xe@lists.freedesktop.org, hdegoede@redhat.com, ilpo.jarvinen@linux.intel.com, De Marchi, Lucas, Vivi, Rodrigo, thomas.hellstrom@linux.intel.com, airlied@gmail.com, simona@ffwll.ch Cc: stable@vger.kernel.org On Fri, 2025-06-06 at 19:20 +0000, Ruhl, Michael J wrote: > > -----Original Message----- > > From: David E. Box <david.e.box@linux.intel.com> > > Sent: Friday, June 6, 2025 1:55 PM > > To: Ruhl, Michael J <michael.j.ruhl@intel.com>; platform-driver- > > x86@vger.kernel.org; intel-xe@lists.freedesktop.org; hdegoede@redhat.com; > > ilpo.jarvinen@linux.intel.com; De Marchi, Lucas <lucas.demarchi@intel.com>; > > Vivi, Rodrigo <rodrigo.vivi@intel.com>; thomas.hellstrom@linux.intel.com; > > airlied@gmail.com; simona@ffwll.ch > > Cc: stable@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@vger.kernel.org> > > > Signed-off-by: Michael J. Ruhl <michael.j.ruhl@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 > ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH v3 02/11] platform/x86/intel/pmt: crashlog binary file endpoint [not found] <20250605184444.515556-1-michael.j.ruhl@intel.com> 2025-06-05 18:44 ` [PATCH v3 01/11] platform/x86/intel: refactor endpoint usage Michael J. Ruhl @ 2025-06-05 18:44 ` Michael J. Ruhl 2025-06-06 19:54 ` David E. Box 1 sibling, 1 reply; 8+ messages in thread From: Michael J. Ruhl @ 2025-06-05 18:44 UTC (permalink / raw) To: platform-driver-x86, intel-xe, hdegoede, ilpo.jarvinen, lucas.demarchi, rodrigo.vivi, thomas.hellstrom, airlied, simona, david.e.box Cc: Michael J. Ruhl, stable Usage of the intel_pmt_read() for binary sysfs, requires an allocated endpoint struct. The crashlog driver does not allocate the endpoint. Without the ep, the crashlog usage causes the following NULL pointer exception: BUG: kernel NULL pointer dereference, address: 0000000000000000 Oops: Oops: 0000 [#1] SMP NOPTI RIP: 0010:intel_pmt_read+0x3b/0x70 [pmt_class] Code: Call Trace: <TASK> ? sysfs_kf_bin_read+0xc0/0xe0 kernfs_fop_read_iter+0xac/0x1a0 vfs_read+0x26d/0x350 ksys_read+0x6b/0xe0 __x64_sys_read+0x1d/0x30 x64_sys_call+0x1bc8/0x1d70 do_syscall_64+0x6d/0x110 Add the endpoint information to the crashlog driver to avoid the NULL pointer exception. Fixes: 416eeb2e1fc7 ("platform/x86/intel/pmt: telemetry: Export API to read telemetry") Cc: <stable@vger.kernel.org> Signed-off-by: Michael J. Ruhl <michael.j.ruhl@intel.com> --- drivers/platform/x86/intel/pmt/crashlog.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/drivers/platform/x86/intel/pmt/crashlog.c b/drivers/platform/x86/intel/pmt/crashlog.c index 6a9eb3c4b313..74ce199e59f0 100644 --- a/drivers/platform/x86/intel/pmt/crashlog.c +++ b/drivers/platform/x86/intel/pmt/crashlog.c @@ -252,6 +252,7 @@ static struct intel_pmt_namespace pmt_crashlog_ns = { .xa = &crashlog_array, .attr_grp = &pmt_crashlog_group, .pmt_header_decode = pmt_crashlog_header_decode, + .pmt_add_endpoint = intel_pmt_add_endpoint, }; /* @@ -262,8 +263,12 @@ static void pmt_crashlog_remove(struct auxiliary_device *auxdev) struct pmt_crashlog_priv *priv = auxiliary_get_drvdata(auxdev); int i; - for (i = 0; i < priv->num_entries; i++) - intel_pmt_dev_destroy(&priv->entry[i].entry, &pmt_crashlog_ns); + for (i = 0; i < priv->num_entries; i++) { + struct intel_pmt_entry *entry = &priv->entry[i].entry; + + intel_pmt_release_endpoint(entry->ep); + intel_pmt_dev_destroy(entry, &pmt_crashlog_ns); + } } static int pmt_crashlog_probe(struct auxiliary_device *auxdev, -- 2.49.0 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH v3 02/11] platform/x86/intel/pmt: crashlog binary file endpoint 2025-06-05 18:44 ` [PATCH v3 02/11] platform/x86/intel/pmt: crashlog binary file endpoint Michael J. Ruhl @ 2025-06-06 19:54 ` David E. Box 0 siblings, 0 replies; 8+ messages in thread From: David E. Box @ 2025-06-06 19:54 UTC (permalink / raw) To: Michael J. Ruhl, platform-driver-x86, intel-xe, hdegoede, ilpo.jarvinen, lucas.demarchi, rodrigo.vivi, thomas.hellstrom, airlied, simona Cc: stable On Thu, 2025-06-05 at 14:44 -0400, Michael J. Ruhl wrote: > Usage of the intel_pmt_read() for binary sysfs, requires an allocated > endpoint struct. The crashlog driver does not allocate the endpoint. > > Without the ep, the crashlog usage causes the following NULL pointer > exception: > > BUG: kernel NULL pointer dereference, address: 0000000000000000 Okay, there it is. I'll still review the rest to see if the endpoint is even needed, but if not then you could drop this patch too. David > Oops: Oops: 0000 [#1] SMP NOPTI > RIP: 0010:intel_pmt_read+0x3b/0x70 [pmt_class] > Code: > Call Trace: > <TASK> > ? sysfs_kf_bin_read+0xc0/0xe0 > kernfs_fop_read_iter+0xac/0x1a0 > vfs_read+0x26d/0x350 > ksys_read+0x6b/0xe0 > __x64_sys_read+0x1d/0x30 > x64_sys_call+0x1bc8/0x1d70 > do_syscall_64+0x6d/0x110 > > Add the endpoint information to the crashlog driver to avoid the NULL > pointer exception. > > Fixes: 416eeb2e1fc7 ("platform/x86/intel/pmt: telemetry: Export API to read > telemetry") > Cc: <stable@vger.kernel.org> > Signed-off-by: Michael J. Ruhl <michael.j.ruhl@intel.com> > --- > drivers/platform/x86/intel/pmt/crashlog.c | 9 +++++++-- > 1 file changed, 7 insertions(+), 2 deletions(-) > > diff --git a/drivers/platform/x86/intel/pmt/crashlog.c > b/drivers/platform/x86/intel/pmt/crashlog.c > index 6a9eb3c4b313..74ce199e59f0 100644 > --- a/drivers/platform/x86/intel/pmt/crashlog.c > +++ b/drivers/platform/x86/intel/pmt/crashlog.c > @@ -252,6 +252,7 @@ static struct intel_pmt_namespace pmt_crashlog_ns = { > .xa = &crashlog_array, > .attr_grp = &pmt_crashlog_group, > .pmt_header_decode = pmt_crashlog_header_decode, > + .pmt_add_endpoint = intel_pmt_add_endpoint, > }; > > /* > @@ -262,8 +263,12 @@ static void pmt_crashlog_remove(struct auxiliary_device > *auxdev) > struct pmt_crashlog_priv *priv = auxiliary_get_drvdata(auxdev); > int i; > > - for (i = 0; i < priv->num_entries; i++) > - intel_pmt_dev_destroy(&priv->entry[i].entry, > &pmt_crashlog_ns); > + for (i = 0; i < priv->num_entries; i++) { > + struct intel_pmt_entry *entry = &priv->entry[i].entry; > + > + intel_pmt_release_endpoint(entry->ep); > + intel_pmt_dev_destroy(entry, &pmt_crashlog_ns); > + } > } > > static int pmt_crashlog_probe(struct auxiliary_device *auxdev, ^ permalink raw reply [flat|nested] 8+ messages in thread
[parent not found: <20250605180906.515367-1-michael.j.ruhl@intel.com>]
* [PATCH v3 01/11] platform/x86/intel: refactor endpoint usage [not found] <20250605180906.515367-1-michael.j.ruhl@intel.com> @ 2025-06-05 18:08 ` Michael J. Ruhl 0 siblings, 0 replies; 8+ messages in thread From: Michael J. Ruhl @ 2025-06-05 18:08 UTC (permalink / raw) To: michael.j.ruhl; +Cc: stable 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. 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@vger.kernel.org> Signed-off-by: Michael J. Ruhl <michael.j.ruhl@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; 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; + count = pmt_telem_read_mmio(entry->ep->pcidev, entry->cb, entry->header.guid, buf, entry->base, off, count); 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 -- 2.49.0 ^ permalink raw reply related [flat|nested] 8+ messages in thread
[parent not found: <20250605173248.510441-1-michael.j.ruhl@intel.com>]
* [PATCH v3 01/11] platform/x86/intel: refactor endpoint usage [not found] <20250605173248.510441-1-michael.j.ruhl@intel.com> @ 2025-06-05 17:32 ` Michael J. Ruhl 0 siblings, 0 replies; 8+ messages in thread From: Michael J. Ruhl @ 2025-06-05 17:32 UTC (permalink / raw) To: michael.j.ruhl; +Cc: stable 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. 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@vger.kernel.org> Signed-off-by: Michael J. Ruhl <michael.j.ruhl@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; 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; + count = pmt_telem_read_mmio(entry->ep->pcidev, entry->cb, entry->header.guid, buf, entry->base, off, count); 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 -- 2.49.0 ^ permalink raw reply related [flat|nested] 8+ messages in thread
end of thread, other threads:[~2025-06-09 17:04 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <20250605184444.515556-1-michael.j.ruhl@intel.com>
2025-06-05 18:44 ` [PATCH v3 01/11] platform/x86/intel: refactor endpoint usage Michael J. Ruhl
2025-06-06 17:54 ` David E. Box
2025-06-06 19:20 ` Ruhl, Michael J
2025-06-09 17:04 ` David E. Box
2025-06-05 18:44 ` [PATCH v3 02/11] platform/x86/intel/pmt: crashlog binary file endpoint Michael J. Ruhl
2025-06-06 19:54 ` David E. Box
[not found] <20250605180906.515367-1-michael.j.ruhl@intel.com>
2025-06-05 18:08 ` [PATCH v3 01/11] platform/x86/intel: refactor endpoint usage Michael J. Ruhl
[not found] <20250605173248.510441-1-michael.j.ruhl@intel.com>
2025-06-05 17:32 ` Michael J. Ruhl
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox