[PATCH v3 00/11] Crashlog Type1 Version2 support

[PATCH v3 00/11] Crashlog Type1 Version2 support

[Michael J. Ruhl]

The Intel BMG GPU device supports the crashlog feature, which was
exposed in an Xe driver patch (drm/xe/vsec: Support BMG devices),
however the version of crashlog used by the BMG GPU does not have
a supporing PMT driver.

Update the PMT crashlog driver to support the BMG crashlog feature.

v2:
 - fix a misconfig for the crashlog DVSEC info in the xe driver
 - address review comments
v3:
 - re-order bug fix patches for stable
 - added re-order trigger logic patch
 - added helper patch to address repeated code patterns
 - address review comments

Michael J. Ruhl (11):
  platform/x86/intel: refactor endpoint usage
  platform/x86/intel/pmt: crashlog binary file endpoint
  drm/xe: Correct BMG VSEC header sizing
  platform/x86/intel/pmt: white space cleanup
  platform/x86/intel/pmt: use guard(mutex)
  platform/x86/intel/pmt: re-order trigger logic
  platform/x86/intel/pmt: correct types
  platform/x86/intel/pmt: decouple sysfs and namespace
  platform/x86/intel/pmt: add register access helpers
  platform/x86/intel/pmt: use a version struct
  platform/x86/intel/pmt: support BMG crashlog

 drivers/gpu/drm/xe/xe_vsec.c                |  20 +-
 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      |  57 ++-
 drivers/platform/x86/intel/pmt/class.h      |  23 +-
 drivers/platform/x86/intel/pmt/crashlog.c   | 477 ++++++++++++++++----
 drivers/platform/x86/intel/pmt/telemetry.c  |  51 +--
 drivers/platform/x86/intel/pmt/telemetry.h  |  23 +-
 9 files changed, 483 insertions(+), 177 deletions(-)

-- 
2.49.0

[PATCH v3 01/11] platform/x86/intel: refactor endpoint usage

[Michael J. Ruhl]

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

[PATCH v3 02/11] platform/x86/intel/pmt: crashlog binary file endpoint

[Michael J. Ruhl]

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

[PATCH v3 03/11] drm/xe: Correct BMG VSEC header sizing

[Michael J. Ruhl]

The intel_vsec_header information for the crashlog feature is
incorrect.

Update the VSEC header with correct sizing and count.

Since the crashlog entries are "merged" (num_entries = 2), the
separate capabilities entries must be merged as well.

Signed-off-by: Michael J. Ruhl <michael.j.ruhl@intel.com>
---
 drivers/gpu/drm/xe/xe_vsec.c | 20 +++++---------------
 1 file changed, 5 insertions(+), 15 deletions(-)

diff --git a/drivers/gpu/drm/xe/xe_vsec.c b/drivers/gpu/drm/xe/xe_vsec.c
index 3e573b0b7ebd..67238fc57a4d 100644
--- a/drivers/gpu/drm/xe/xe_vsec.c
+++ b/drivers/gpu/drm/xe/xe_vsec.c
@@ -32,28 +32,18 @@ static struct intel_vsec_header bmg_telemetry = {
 	.offset = BMG_DISCOVERY_OFFSET,
 };
 
-static struct intel_vsec_header bmg_punit_crashlog = {
-	.length = 0x10,
+static struct intel_vsec_header bmg_crashlog = {
+	.length = 0x18,
 	.id = VSEC_ID_CRASHLOG,
-	.num_entries = 1,
-	.entry_size = 4,
+	.num_entries = 2,
+	.entry_size = 6,
 	.tbir = 0,
 	.offset = BMG_DISCOVERY_OFFSET + 0x60,
 };
 
-static struct intel_vsec_header bmg_oobmsm_crashlog = {
-	.length = 0x10,
-	.id = VSEC_ID_CRASHLOG,
-	.num_entries = 1,
-	.entry_size = 4,
-	.tbir = 0,
-	.offset = BMG_DISCOVERY_OFFSET + 0x78,
-};
-
 static struct intel_vsec_header *bmg_capabilities[] = {
 	&bmg_telemetry,
-	&bmg_punit_crashlog,
-	&bmg_oobmsm_crashlog,
+	&bmg_crashlog,
 	NULL
 };
 
-- 
2.49.0

[PATCH v3 04/11] platform/x86/intel/pmt: white space cleanup

[Michael J. Ruhl]

Noticed two white space issues; cleaned them.

Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
Signed-off-by: Michael J. Ruhl <michael.j.ruhl@intel.com>
---
 drivers/platform/x86/intel/pmt/crashlog.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/platform/x86/intel/pmt/crashlog.c b/drivers/platform/x86/intel/pmt/crashlog.c
index 74ce199e59f0..e997fc48b9ce 100644
--- a/drivers/platform/x86/intel/pmt/crashlog.c
+++ b/drivers/platform/x86/intel/pmt/crashlog.c
@@ -143,7 +143,7 @@ enable_show(struct device *dev, struct device_attribute *attr, char *buf)
 
 static ssize_t
 enable_store(struct device *dev, struct device_attribute *attr,
-	    const char *buf, size_t count)
+	     const char *buf, size_t count)
 {
 	struct crashlog_entry *entry;
 	bool enabled;
@@ -177,7 +177,7 @@ trigger_show(struct device *dev, struct device_attribute *attr, char *buf)
 
 static ssize_t
 trigger_store(struct device *dev, struct device_attribute *attr,
-	    const char *buf, size_t count)
+	      const char *buf, size_t count)
 {
 	struct crashlog_entry *entry;
 	bool trigger;
-- 
2.49.0

[PATCH v3 05/11] platform/x86/intel/pmt: use guard(mutex)

[Michael J. Ruhl]

Update the mutex paths to use the new guard() mechanism.

With the removal of goto, do some minor cleanup of the current logic
path.

Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
Signed-off-by: Michael J. Ruhl <michael.j.ruhl@intel.com>
---
 drivers/platform/x86/intel/pmt/crashlog.c | 32 +++++++++++------------
 1 file changed, 15 insertions(+), 17 deletions(-)

diff --git a/drivers/platform/x86/intel/pmt/crashlog.c b/drivers/platform/x86/intel/pmt/crashlog.c
index e997fc48b9ce..94858bfb52f8 100644
--- a/drivers/platform/x86/intel/pmt/crashlog.c
+++ b/drivers/platform/x86/intel/pmt/crashlog.c
@@ -155,9 +155,9 @@ enable_store(struct device *dev, struct device_attribute *attr,
 	if (result)
 		return result;
 
-	mutex_lock(&entry->control_mutex);
+	guard(mutex)(&entry->control_mutex);
+
 	pmt_crashlog_set_disable(&entry->entry, !enabled);
-	mutex_unlock(&entry->control_mutex);
 
 	return count;
 }
@@ -189,26 +189,24 @@ trigger_store(struct device *dev, struct device_attribute *attr,
 	if (result)
 		return result;
 
-	mutex_lock(&entry->control_mutex);
+	guard(mutex)(&entry->control_mutex);
 
 	if (!trigger) {
 		pmt_crashlog_set_clear(&entry->entry);
-	} else if (pmt_crashlog_complete(&entry->entry)) {
-		/* we cannot trigger a new crash if one is still pending */
-		result = -EEXIST;
-		goto err;
-	} else if (pmt_crashlog_disabled(&entry->entry)) {
-		/* if device is currently disabled, return busy */
-		result = -EBUSY;
-		goto err;
-	} else {
-		pmt_crashlog_set_execute(&entry->entry);
+		return count;
 	}
 
-	result = count;
-err:
-	mutex_unlock(&entry->control_mutex);
-	return result;
+	/* we cannot trigger a new crash if one is still pending */
+	if (pmt_crashlog_complete(&entry->entry))
+		return -EEXIST;
+
+	/* if device is currently disabled, return busy */
+	if (pmt_crashlog_disabled(&entry->entry))
+		return -EBUSY;
+
+	pmt_crashlog_set_execute(&entry->entry);
+
+	return count;
 }
 static DEVICE_ATTR_RW(trigger);
 
-- 
2.49.0

[PATCH v3 06/11] platform/x86/intel/pmt: re-order trigger logic

[Michael J. Ruhl]

Setting the clear bit or checking the complete bit before checking to
see if crashlog is disabled seems incorrect.

Check disable before accessing any other bits.

Signed-off-by: Michael J. Ruhl <michael.j.ruhl@intel.com>
---
 drivers/platform/x86/intel/pmt/crashlog.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/platform/x86/intel/pmt/crashlog.c b/drivers/platform/x86/intel/pmt/crashlog.c
index 94858bfb52f8..aa3f57fbe018 100644
--- a/drivers/platform/x86/intel/pmt/crashlog.c
+++ b/drivers/platform/x86/intel/pmt/crashlog.c
@@ -191,6 +191,10 @@ trigger_store(struct device *dev, struct device_attribute *attr,
 
 	guard(mutex)(&entry->control_mutex);
 
+	/* if device is currently disabled, return busy */
+	if (pmt_crashlog_disabled(&entry->entry))
+		return -EBUSY;
+
 	if (!trigger) {
 		pmt_crashlog_set_clear(&entry->entry);
 		return count;
@@ -200,10 +204,6 @@ trigger_store(struct device *dev, struct device_attribute *attr,
 	if (pmt_crashlog_complete(&entry->entry))
 		return -EEXIST;
 
-	/* if device is currently disabled, return busy */
-	if (pmt_crashlog_disabled(&entry->entry))
-		return -EBUSY;
-
 	pmt_crashlog_set_execute(&entry->entry);
 
 	return count;
-- 
2.49.0

[PATCH v3 07/11] platform/x86/intel/pmt: correct types

[Michael J. Ruhl]

A couple of auto variables do not match the return types of some of
the functions.

Update the mismatched types to match.

Signed-off-by: Michael J. Ruhl <michael.j.ruhl@intel.com>
---
 drivers/platform/x86/intel/pmt/crashlog.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/platform/x86/intel/pmt/crashlog.c b/drivers/platform/x86/intel/pmt/crashlog.c
index aa3f57fbe018..a51923d0be72 100644
--- a/drivers/platform/x86/intel/pmt/crashlog.c
+++ b/drivers/platform/x86/intel/pmt/crashlog.c
@@ -136,7 +136,7 @@ static ssize_t
 enable_show(struct device *dev, struct device_attribute *attr, char *buf)
 {
 	struct intel_pmt_entry *entry = dev_get_drvdata(dev);
-	int enabled = !pmt_crashlog_disabled(entry);
+	bool enabled = !pmt_crashlog_disabled(entry);
 
 	return sprintf(buf, "%d\n", enabled);
 }
@@ -167,7 +167,7 @@ static ssize_t
 trigger_show(struct device *dev, struct device_attribute *attr, char *buf)
 {
 	struct intel_pmt_entry *entry;
-	int trigger;
+	bool trigger;
 
 	entry = dev_get_drvdata(dev);
 	trigger = pmt_crashlog_complete(entry);
-- 
2.49.0

[PATCH v3 08/11] platform/x86/intel/pmt: decouple sysfs and namespace

[Michael J. Ruhl]

The PMT namespace includes the crashlog sysfs attribute information.
Other crashlog version/types may need different sysfs attributes.
Coupling the attributes with the namespace blocks this usage.

Decouple sysfs attributes from the name space and add them to the
specific entry.

Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
Signed-off-by: Michael J. Ruhl <michael.j.ruhl@intel.com>
---
 drivers/platform/x86/intel/pmt/class.c    | 12 ++++++------
 drivers/platform/x86/intel/pmt/class.h    |  2 +-
 drivers/platform/x86/intel/pmt/crashlog.c |  3 ++-
 3 files changed, 9 insertions(+), 8 deletions(-)

diff --git a/drivers/platform/x86/intel/pmt/class.c b/drivers/platform/x86/intel/pmt/class.c
index bba552131bc2..880baf02a985 100644
--- a/drivers/platform/x86/intel/pmt/class.c
+++ b/drivers/platform/x86/intel/pmt/class.c
@@ -329,8 +329,8 @@ static int intel_pmt_dev_register(struct intel_pmt_entry *entry,
 
 	entry->kobj = &dev->kobj;
 
-	if (ns->attr_grp) {
-		ret = sysfs_create_group(entry->kobj, ns->attr_grp);
+	if (entry->attr_grp) {
+		ret = sysfs_create_group(entry->kobj, entry->attr_grp);
 		if (ret)
 			goto fail_sysfs_create_group;
 	}
@@ -371,8 +371,8 @@ static int intel_pmt_dev_register(struct intel_pmt_entry *entry,
 fail_add_endpoint:
 	sysfs_remove_bin_file(entry->kobj, &entry->pmt_bin_attr);
 fail_ioremap:
-	if (ns->attr_grp)
-		sysfs_remove_group(entry->kobj, ns->attr_grp);
+	if (entry->attr_grp)
+		sysfs_remove_group(entry->kobj, entry->attr_grp);
 fail_sysfs_create_group:
 	device_unregister(dev);
 fail_dev_create:
@@ -414,8 +414,8 @@ void intel_pmt_dev_destroy(struct intel_pmt_entry *entry,
 	if (entry->size)
 		sysfs_remove_bin_file(entry->kobj, &entry->pmt_bin_attr);
 
-	if (ns->attr_grp)
-		sysfs_remove_group(entry->kobj, ns->attr_grp);
+	if (entry->attr_grp)
+		sysfs_remove_group(entry->kobj, entry->attr_grp);
 
 	device_unregister(dev);
 	xa_erase(ns->xa, entry->devid);
diff --git a/drivers/platform/x86/intel/pmt/class.h b/drivers/platform/x86/intel/pmt/class.h
index d2d8f9e31c9d..a44571c09253 100644
--- a/drivers/platform/x86/intel/pmt/class.h
+++ b/drivers/platform/x86/intel/pmt/class.h
@@ -47,6 +47,7 @@ struct intel_pmt_entry {
 	struct class_endpoint	*ep;
 	struct intel_pmt_header	header;
 	struct bin_attribute	pmt_bin_attr;
+	const struct attribute_group *attr_grp;
 	struct kobject		*kobj;
 	void __iomem		*disc_table;
 	void __iomem		*base;
@@ -60,7 +61,6 @@ struct intel_pmt_entry {
 struct intel_pmt_namespace {
 	const char *name;
 	struct xarray *xa;
-	const struct attribute_group *attr_grp;
 	int (*pmt_header_decode)(struct intel_pmt_entry *entry,
 				 struct device *dev);
 	int (*pmt_add_endpoint)(struct intel_vsec_device *ivdev,
diff --git a/drivers/platform/x86/intel/pmt/crashlog.c b/drivers/platform/x86/intel/pmt/crashlog.c
index a51923d0be72..463b0ba4a842 100644
--- a/drivers/platform/x86/intel/pmt/crashlog.c
+++ b/drivers/platform/x86/intel/pmt/crashlog.c
@@ -241,6 +241,8 @@ static int pmt_crashlog_header_decode(struct intel_pmt_entry *entry,
 	/* Size is measured in DWORDS, but accessor returns bytes */
 	header->size = GET_SIZE(readl(disc_table + SIZE_OFFSET));
 
+	entry->attr_grp = &pmt_crashlog_group;
+
 	return 0;
 }
 
@@ -248,7 +250,6 @@ static DEFINE_XARRAY_ALLOC(crashlog_array);
 static struct intel_pmt_namespace pmt_crashlog_ns = {
 	.name = "crashlog",
 	.xa = &crashlog_array,
-	.attr_grp = &pmt_crashlog_group,
 	.pmt_header_decode = pmt_crashlog_header_decode,
 	.pmt_add_endpoint = intel_pmt_add_endpoint,
 };
-- 
2.49.0

[PATCH v3 09/11] platform/x86/intel/pmt: add register access helpers

[Michael J. Ruhl]

The control register is used in a read/modify/write pattern.
The status register is used in a read/check bit pattern.

Add helpers to eliminate common code.

Signed-off-by: Michael J. Ruhl <michael.j.ruhl@intel.com>
---
 drivers/platform/x86/intel/pmt/crashlog.c | 58 +++++++++++------------
 1 file changed, 29 insertions(+), 29 deletions(-)

diff --git a/drivers/platform/x86/intel/pmt/crashlog.c b/drivers/platform/x86/intel/pmt/crashlog.c
index 463b0ba4a842..318d7a21f00e 100644
--- a/drivers/platform/x86/intel/pmt/crashlog.c
+++ b/drivers/platform/x86/intel/pmt/crashlog.c
@@ -62,20 +62,40 @@ struct pmt_crashlog_priv {
 /*
  * I/O
  */
-static bool pmt_crashlog_complete(struct intel_pmt_entry *entry)
+#define SET     true
+#define CLEAR   false
+
+static void read_modify_write(struct intel_pmt_entry *entry, u32 bit, bool set)
 {
-	u32 control = readl(entry->disc_table + CONTROL_OFFSET);
+	u32 reg = readl(entry->disc_table + CONTROL_OFFSET);
+
+	reg &= ~CRASHLOG_FLAG_TRIGGER_MASK;
+
+	if (set)
+		reg |= bit;
+	else
+		reg &= bit;
+
+	writel(reg, entry->disc_table + CONTROL_OFFSET);
+}
+
+static bool read_check(struct intel_pmt_entry *entry, u32 bit)
+{
+	u32 reg = readl(entry->disc_table + CONTROL_OFFSET);
+
+	return !!(reg & bit);
+}
 
+static bool pmt_crashlog_complete(struct intel_pmt_entry *entry)
+{
 	/* return current value of the crashlog complete flag */
-	return !!(control & CRASHLOG_FLAG_TRIGGER_COMPLETE);
+	return read_check(entry, CRASHLOG_FLAG_TRIGGER_COMPLETE);
 }
 
 static bool pmt_crashlog_disabled(struct intel_pmt_entry *entry)
 {
-	u32 control = readl(entry->disc_table + CONTROL_OFFSET);
-
 	/* return current value of the crashlog disabled flag */
-	return !!(control & CRASHLOG_FLAG_DISABLE);
+	return read_check(entry, CRASHLOG_FLAG_DISABLE);
 }
 
 static bool pmt_crashlog_supported(struct intel_pmt_entry *entry)
@@ -96,37 +116,17 @@ static bool pmt_crashlog_supported(struct intel_pmt_entry *entry)
 static void pmt_crashlog_set_disable(struct intel_pmt_entry *entry,
 				     bool disable)
 {
-	u32 control = readl(entry->disc_table + CONTROL_OFFSET);
-
-	/* clear trigger bits so we are only modifying disable flag */
-	control &= ~CRASHLOG_FLAG_TRIGGER_MASK;
-
-	if (disable)
-		control |= CRASHLOG_FLAG_DISABLE;
-	else
-		control &= ~CRASHLOG_FLAG_DISABLE;
-
-	writel(control, entry->disc_table + CONTROL_OFFSET);
+	read_modify_write(entry, CRASHLOG_FLAG_DISABLE, disable);
 }
 
 static void pmt_crashlog_set_clear(struct intel_pmt_entry *entry)
 {
-	u32 control = readl(entry->disc_table + CONTROL_OFFSET);
-
-	control &= ~CRASHLOG_FLAG_TRIGGER_MASK;
-	control |= CRASHLOG_FLAG_TRIGGER_CLEAR;
-
-	writel(control, entry->disc_table + CONTROL_OFFSET);
+	read_modify_write(entry, CRASHLOG_FLAG_TRIGGER_CLEAR, SET);
 }
 
 static void pmt_crashlog_set_execute(struct intel_pmt_entry *entry)
 {
-	u32 control = readl(entry->disc_table + CONTROL_OFFSET);
-
-	control &= ~CRASHLOG_FLAG_TRIGGER_MASK;
-	control |= CRASHLOG_FLAG_TRIGGER_EXECUTE;
-
-	writel(control, entry->disc_table + CONTROL_OFFSET);
+	read_modify_write(entry, CRASHLOG_FLAG_TRIGGER_EXECUTE, SET);
 }
 
 /*
-- 
2.49.0

[PATCH v3 10/11] platform/x86/intel/pmt: use a version struct

[Michael J. Ruhl]

In preparation for supporting multiple crashlog versions, use a struct
to keep bit offset info for the status and control bits.

Signed-off-by: Michael J. Ruhl <michael.j.ruhl@intel.com>
---
 drivers/platform/x86/intel/pmt/crashlog.c | 176 ++++++++++++++--------
 1 file changed, 112 insertions(+), 64 deletions(-)

diff --git a/drivers/platform/x86/intel/pmt/crashlog.c b/drivers/platform/x86/intel/pmt/crashlog.c
index 318d7a21f00e..fe6563721886 100644
--- a/drivers/platform/x86/intel/pmt/crashlog.c
+++ b/drivers/platform/x86/intel/pmt/crashlog.c
@@ -22,21 +22,6 @@
 /* Crashlog discovery header types */
 #define CRASH_TYPE_OOBMSM	1
 
-/* Control Flags */
-#define CRASHLOG_FLAG_DISABLE		BIT(28)
-
-/*
- * Bits 29 and 30 control the state of bit 31.
- *
- * Bit 29 will clear bit 31, if set, allowing a new crashlog to be captured.
- * Bit 30 will immediately trigger a crashlog to be generated, setting bit 31.
- * Bit 31 is the read-only status with a 1 indicating log is complete.
- */
-#define CRASHLOG_FLAG_TRIGGER_CLEAR	BIT(29)
-#define CRASHLOG_FLAG_TRIGGER_EXECUTE	BIT(30)
-#define CRASHLOG_FLAG_TRIGGER_COMPLETE	BIT(31)
-#define CRASHLOG_FLAG_TRIGGER_MASK	GENMASK(31, 28)
-
 /* Crashlog Discovery Header */
 #define CONTROL_OFFSET		0x0
 #define GUID_OFFSET		0x4
@@ -48,10 +33,63 @@
 /* size is in bytes */
 #define GET_SIZE(v)		((v) * sizeof(u32))
 
+/*
+ * Type 1 Version 0
+ * status and control registers are combined.
+ *
+ * Bits 29 and 30 control the state of bit 31.
+ * Bit 29 will clear bit 31, if set, allowing a new crashlog to be captured.
+ * Bit 30 will immediately trigger a crashlog to be generated, setting bit 31.
+ * Bit 31 is the read-only status with a 1 indicating log is complete.
+ */
+#define TYPE1_VER0_STATUS_OFFSET	0x00
+#define TYPE1_VER0_CONTROL_OFFSET	0x00
+
+#define TYPE1_VER0_DISABLE		BIT(28)
+#define TYPE1_VER0_CLEAR		BIT(29)
+#define TYPE1_VER0_EXECUTE		BIT(30)
+#define TYPE1_VER0_COMPLETE		BIT(31)
+#define TYPE1_VER0_TRIGGER_MASK		GENMASK(31, 28)
+
+/* After offset, order alphabetically, not bit ordered */
+struct crashlog_status {
+	u32 offset;
+	u32 clear;
+	u32 complete;
+	u32 disable;
+};
+
+struct crashlog_control {
+	u32 offset;
+	u32 trigger_mask;
+	u32 clear;
+	u32 disable;
+	u32 manual;
+};
+
+struct crashlog_info {
+	struct crashlog_status status;
+	struct crashlog_control control;
+};
+
+const struct crashlog_info crashlog_type1_ver0 = {
+	.status.offset = TYPE1_VER0_STATUS_OFFSET,
+	.status.clear = TYPE1_VER0_CLEAR,
+	.status.complete = TYPE1_VER0_COMPLETE,
+	.status.disable = TYPE1_VER0_DISABLE,
+
+	.control.offset = TYPE1_VER0_CONTROL_OFFSET,
+	.control.trigger_mask = TYPE1_VER0_TRIGGER_MASK,
+	.control.clear = TYPE1_VER0_CLEAR,
+	.control.disable = TYPE1_VER0_DISABLE,
+	.control.manual = TYPE1_VER0_EXECUTE,
+};
+
 struct crashlog_entry {
 	/* entry must be first member of struct */
 	struct intel_pmt_entry		entry;
 	struct mutex			control_mutex;
+	const struct crashlog_info	*info;
 };
 
 struct pmt_crashlog_priv {
@@ -59,74 +97,82 @@ struct pmt_crashlog_priv {
 	struct crashlog_entry	entry[];
 };
 
+/*
+ * This is the generic access to a PMT struct. So the use of
+ * struct crashlog_entry
+ * doesn't "make sense" here.
+ */
+static bool pmt_crashlog_supported(struct intel_pmt_entry *entry)
+{
+	u32 discovery_header = readl(entry->disc_table + CONTROL_OFFSET);
+	u32 crash_type, version;
+
+	crash_type = GET_TYPE(discovery_header);
+	version = GET_VERSION(discovery_header);
+
+	/*
+	 * Currently we only recognize OOBMSM version 0 devices.
+	 * We can ignore all other crashlog devices in the system.
+	 */
+	return crash_type == CRASH_TYPE_OOBMSM && version == 0;
+}
+
 /*
  * I/O
  */
+
 #define SET     true
 #define CLEAR   false
 
-static void read_modify_write(struct intel_pmt_entry *entry, u32 bit, bool set)
+static void read_modify_write(struct crashlog_entry *crashlog, u32 bit, bool set)
 {
-	u32 reg = readl(entry->disc_table + CONTROL_OFFSET);
+	const struct crashlog_control *control = &crashlog->info->control;
+	struct intel_pmt_entry *entry = &crashlog->entry;
+	u32 reg = readl(entry->disc_table + control->offset);
 
-	reg &= ~CRASHLOG_FLAG_TRIGGER_MASK;
+	reg &= ~control->trigger_mask;
 
 	if (set)
 		reg |= bit;
 	else
 		reg &= bit;
 
-	writel(reg, entry->disc_table + CONTROL_OFFSET);
+	writel(reg, entry->disc_table + control->offset);
 }
 
-static bool read_check(struct intel_pmt_entry *entry, u32 bit)
+static bool read_check(struct crashlog_entry *crashlog, u32 bit)
 {
-	u32 reg = readl(entry->disc_table + CONTROL_OFFSET);
+	const struct crashlog_status *status = &crashlog->info->status;
+	u32 reg = readl(crashlog->entry.disc_table + status->offset);
 
 	return !!(reg & bit);
 }
 
-static bool pmt_crashlog_complete(struct intel_pmt_entry *entry)
+static bool pmt_crashlog_complete(struct crashlog_entry *crashlog)
 {
 	/* return current value of the crashlog complete flag */
-	return read_check(entry, CRASHLOG_FLAG_TRIGGER_COMPLETE);
+	return read_check(crashlog, crashlog->info->status.complete);
 }
 
-static bool pmt_crashlog_disabled(struct intel_pmt_entry *entry)
+static bool pmt_crashlog_disabled(struct crashlog_entry *crashlog)
 {
 	/* return current value of the crashlog disabled flag */
-	return read_check(entry, CRASHLOG_FLAG_DISABLE);
-}
-
-static bool pmt_crashlog_supported(struct intel_pmt_entry *entry)
-{
-	u32 discovery_header = readl(entry->disc_table + CONTROL_OFFSET);
-	u32 crash_type, version;
-
-	crash_type = GET_TYPE(discovery_header);
-	version = GET_VERSION(discovery_header);
-
-	/*
-	 * Currently we only recognize OOBMSM version 0 devices.
-	 * We can ignore all other crashlog devices in the system.
-	 */
-	return crash_type == CRASH_TYPE_OOBMSM && version == 0;
+	return read_check(crashlog, crashlog->info->status.disable);
 }
 
-static void pmt_crashlog_set_disable(struct intel_pmt_entry *entry,
-				     bool disable)
+static void pmt_crashlog_set_disable(struct crashlog_entry *crashlog, bool disable)
 {
-	read_modify_write(entry, CRASHLOG_FLAG_DISABLE, disable);
+	read_modify_write(crashlog, crashlog->info->control.disable, disable);
 }
 
-static void pmt_crashlog_set_clear(struct intel_pmt_entry *entry)
+static void pmt_crashlog_set_clear(struct crashlog_entry *crashlog)
 {
-	read_modify_write(entry, CRASHLOG_FLAG_TRIGGER_CLEAR, SET);
+	read_modify_write(crashlog, crashlog->info->control.clear, SET);
 }
 
-static void pmt_crashlog_set_execute(struct intel_pmt_entry *entry)
+static void pmt_crashlog_set_execute(struct crashlog_entry *crashlog)
 {
-	read_modify_write(entry, CRASHLOG_FLAG_TRIGGER_EXECUTE, SET);
+	read_modify_write(crashlog, crashlog->info->control.manual, SET);
 }
 
 /*
@@ -135,8 +181,8 @@ static void pmt_crashlog_set_execute(struct intel_pmt_entry *entry)
 static ssize_t
 enable_show(struct device *dev, struct device_attribute *attr, char *buf)
 {
-	struct intel_pmt_entry *entry = dev_get_drvdata(dev);
-	bool enabled = !pmt_crashlog_disabled(entry);
+	struct crashlog_entry *crashlog = dev_get_drvdata(dev);
+	bool enabled = !pmt_crashlog_disabled(crashlog);
 
 	return sprintf(buf, "%d\n", enabled);
 }
@@ -145,19 +191,19 @@ static ssize_t
 enable_store(struct device *dev, struct device_attribute *attr,
 	     const char *buf, size_t count)
 {
-	struct crashlog_entry *entry;
+	struct crashlog_entry *crashlog;
 	bool enabled;
 	int result;
 
-	entry = dev_get_drvdata(dev);
+	crashlog = dev_get_drvdata(dev);
 
 	result = kstrtobool(buf, &enabled);
 	if (result)
 		return result;
 
-	guard(mutex)(&entry->control_mutex);
+	guard(mutex)(&crashlog->control_mutex);
 
-	pmt_crashlog_set_disable(&entry->entry, !enabled);
+	pmt_crashlog_set_disable(crashlog, !enabled);
 
 	return count;
 }
@@ -166,11 +212,11 @@ static DEVICE_ATTR_RW(enable);
 static ssize_t
 trigger_show(struct device *dev, struct device_attribute *attr, char *buf)
 {
-	struct intel_pmt_entry *entry;
+	struct crashlog_entry *crashlog;
 	bool trigger;
 
-	entry = dev_get_drvdata(dev);
-	trigger = pmt_crashlog_complete(entry);
+	crashlog = dev_get_drvdata(dev);
+	trigger = pmt_crashlog_complete(crashlog);
 
 	return sprintf(buf, "%d\n", trigger);
 }
@@ -179,32 +225,33 @@ static ssize_t
 trigger_store(struct device *dev, struct device_attribute *attr,
 	      const char *buf, size_t count)
 {
-	struct crashlog_entry *entry;
+	struct crashlog_entry *crashlog;
 	bool trigger;
 	int result;
 
-	entry = dev_get_drvdata(dev);
+	crashlog = dev_get_drvdata(dev);
 
 	result = kstrtobool(buf, &trigger);
 	if (result)
 		return result;
 
-	guard(mutex)(&entry->control_mutex);
+	guard(mutex)(&crashlog->control_mutex);
 
 	/* if device is currently disabled, return busy */
-	if (pmt_crashlog_disabled(&entry->entry))
+	if (pmt_crashlog_disabled(crashlog))
 		return -EBUSY;
 
 	if (!trigger) {
-		pmt_crashlog_set_clear(&entry->entry);
+		pmt_crashlog_set_clear(crashlog);
 		return count;
 	}
 
 	/* we cannot trigger a new crash if one is still pending */
-	if (pmt_crashlog_complete(&entry->entry))
+	if (pmt_crashlog_complete(crashlog))
 		return -EEXIST;
 
-	pmt_crashlog_set_execute(&entry->entry);
+	pmt_crashlog_set_execute(crashlog);
+
 
 	return count;
 }
@@ -230,9 +277,10 @@ static int pmt_crashlog_header_decode(struct intel_pmt_entry *entry,
 	if (!pmt_crashlog_supported(entry))
 		return 1;
 
-	/* initialize control mutex */
+	/* initialize the crashlog struct */
 	crashlog = container_of(entry, struct crashlog_entry, entry);
 	mutex_init(&crashlog->control_mutex);
+	crashlog->info = &crashlog_type1_ver0;
 
 	header->access_type = GET_ACCESS(readl(disc_table));
 	header->guid = readl(disc_table + GUID_OFFSET);
-- 
2.49.0

[PATCH v3 11/11] platform/x86/intel/pmt: support BMG crashlog

[Michael J. Ruhl]

The Battlemage GPU has the type 1 version 2 crashlog feature.

Update the crashlog driver to support this crashlog version.

Signed-off-by: Michael J. Ruhl <michael.j.ruhl@intel.com>
---
 drivers/platform/x86/intel/pmt/crashlog.c | 283 ++++++++++++++++++++--
 1 file changed, 264 insertions(+), 19 deletions(-)

diff --git a/drivers/platform/x86/intel/pmt/crashlog.c b/drivers/platform/x86/intel/pmt/crashlog.c
index fe6563721886..0fb60036a9bb 100644
--- a/drivers/platform/x86/intel/pmt/crashlog.c
+++ b/drivers/platform/x86/intel/pmt/crashlog.c
@@ -51,20 +51,53 @@
 #define TYPE1_VER0_COMPLETE		BIT(31)
 #define TYPE1_VER0_TRIGGER_MASK		GENMASK(31, 28)
 
+/*
+ * Type 1 Version 2
+ * status and control are two different registers
+ */
+#define TYPE1_VER2_STATUS_OFFSET	0x00
+#define TYPE1_VER2_CONTROL_OFFSET	0x14
+
+/* status register */
+#define TYPE1_VER2_CLEAR_SUPPORT	BIT(20)
+#define TYPE1_VER2_REARMED		BIT(25)
+#define TYPE1_VER2_ERROR		BIT(26)
+#define TYPE1_VER2_CONSUMED		BIT(27)
+#define TYPE1_VER2_DISABLED		BIT(28)
+#define TYPE1_VER2_CLEARED		BIT(29)
+#define TYPE1_VER2_IN_PROGRESS		BIT(30)
+#define TYPE1_VER2_COMPLETE		BIT(31)
+
+/* control register */
+#define TYPE1_VER2_CONSUME		BIT(25)
+#define TYPE1_VER2_REARM		BIT(28)
+#define TYPE1_VER2_EXECUTE		BIT(29)
+#define TYPE1_VER2_CLEAR		BIT(30)
+#define TYPE1_VER2_DISABLE		BIT(31)
+#define TYPE1_VER2_TRIGGER_MASK		(TYPE1_VER2_CONSUME | TYPE1_VER2_EXECUTE | \
+					 TYPE1_VER2_CLEAR | TYPE1_VER2_DISABLE)
+
 /* After offset, order alphabetically, not bit ordered */
 struct crashlog_status {
 	u32 offset;
-	u32 clear;
+	u32 clear_supported;
+	u32 cleared;
 	u32 complete;
-	u32 disable;
+	u32 consumed;
+	u32 disabled;
+	u32 error;
+	u32 in_progress;
+	u32 rearmed;
 };
 
 struct crashlog_control {
 	u32 offset;
 	u32 trigger_mask;
 	u32 clear;
+	u32 consume;
 	u32 disable;
 	u32 manual;
+	u32 rearm;
 };
 
 struct crashlog_info {
@@ -74,9 +107,9 @@ struct crashlog_info {
 
 const struct crashlog_info crashlog_type1_ver0 = {
 	.status.offset = TYPE1_VER0_STATUS_OFFSET,
-	.status.clear = TYPE1_VER0_CLEAR,
+	.status.cleared = TYPE1_VER0_CLEAR,
 	.status.complete = TYPE1_VER0_COMPLETE,
-	.status.disable = TYPE1_VER0_DISABLE,
+	.status.disabled = TYPE1_VER0_DISABLE,
 
 	.control.offset = TYPE1_VER0_CONTROL_OFFSET,
 	.control.trigger_mask = TYPE1_VER0_TRIGGER_MASK,
@@ -85,6 +118,26 @@ const struct crashlog_info crashlog_type1_ver0 = {
 	.control.manual = TYPE1_VER0_EXECUTE,
 };
 
+const struct crashlog_info crashlog_type1_ver2 = {
+	.status.offset = TYPE1_VER2_STATUS_OFFSET,
+	.status.clear_supported = TYPE1_VER2_CLEAR_SUPPORT,
+	.status.cleared = TYPE1_VER2_CLEARED,
+	.status.complete = TYPE1_VER2_COMPLETE,
+	.status.consumed = TYPE1_VER2_CONSUMED,
+	.status.disabled = TYPE1_VER2_DISABLED,
+	.status.error = TYPE1_VER2_ERROR,
+	.status.in_progress = TYPE1_VER2_IN_PROGRESS,
+	.status.rearmed = TYPE1_VER2_REARMED,
+
+	.control.offset = TYPE1_VER2_CONTROL_OFFSET,
+	.control.trigger_mask = TYPE1_VER2_TRIGGER_MASK,
+	.control.clear = TYPE1_VER2_CLEAR,
+	.control.consume = TYPE1_VER2_CONSUME,
+	.control.disable = TYPE1_VER2_DISABLE,
+	.control.manual = TYPE1_VER2_EXECUTE,
+	.control.rearm = TYPE1_VER2_REARM,
+};
+
 struct crashlog_entry {
 	/* entry must be first member of struct */
 	struct intel_pmt_entry		entry;
@@ -99,30 +152,35 @@ struct pmt_crashlog_priv {
 
 /*
  * This is the generic access to a PMT struct. So the use of
- * struct crashlog_entry
- * doesn't "make sense" here.
+ *   struct crashlog_entry
+ * doesn't "make sense" here, i.e. use:
+ *   struct intel_pmt_entry
  */
-static bool pmt_crashlog_supported(struct intel_pmt_entry *entry)
+static bool pmt_crashlog_supported(struct intel_pmt_entry *entry, u32 *crash_type, u32 *version)
 {
 	u32 discovery_header = readl(entry->disc_table + CONTROL_OFFSET);
-	u32 crash_type, version;
 
-	crash_type = GET_TYPE(discovery_header);
-	version = GET_VERSION(discovery_header);
+	*crash_type = GET_TYPE(discovery_header);
+	*version = GET_VERSION(discovery_header);
 
 	/*
-	 * Currently we only recognize OOBMSM version 0 devices.
-	 * We can ignore all other crashlog devices in the system.
+	 * Currently we only recognize OOBMSM (type 1) and version 0 or 2
+	 * devices.
+	 *
+	 * Ignore all other crashlog devices in the system.
 	 */
-	return crash_type == CRASH_TYPE_OOBMSM && version == 0;
+	if (*crash_type == CRASH_TYPE_OOBMSM && (*version == 0 || *version == 2))
+		return true;
+
+	return false;
 }
 
 /*
  * I/O
  */
 
-#define SET     true
-#define CLEAR   false
+#define SET	true
+#define CLEAR	false
 
 static void read_modify_write(struct crashlog_entry *crashlog, u32 bit, bool set)
 {
@@ -157,7 +215,7 @@ static bool pmt_crashlog_complete(struct crashlog_entry *crashlog)
 static bool pmt_crashlog_disabled(struct crashlog_entry *crashlog)
 {
 	/* return current value of the crashlog disabled flag */
-	return read_check(crashlog, crashlog->info->status.disable);
+	return read_check(crashlog, crashlog->info->status.disabled);
 }
 
 static void pmt_crashlog_set_disable(struct crashlog_entry *crashlog, bool disable)
@@ -175,9 +233,119 @@ static void pmt_crashlog_set_execute(struct crashlog_entry *crashlog)
 	read_modify_write(crashlog, crashlog->info->control.manual, SET);
 }
 
+/* version 2 support */
+static bool pmt_crashlog_cleared(struct crashlog_entry *crashlog)
+{
+	/* return current value of the crashlog cleared flag */
+	return read_check(crashlog, crashlog->info->status.cleared);
+}
+
+static bool pmt_crashlog_consumed(struct crashlog_entry *crashlog)
+{
+	/* return current value of the crashlog consumedflag */
+	return read_check(crashlog, crashlog->info->status.consumed);
+}
+
+static void pmt_crashlog_set_consumed(struct crashlog_entry *crashlog)
+{
+	read_modify_write(crashlog, crashlog->info->control.consume, SET);
+}
+
+static bool pmt_crashlog_error(struct crashlog_entry *crashlog)
+{
+	/* return current value of the crashlog error flag */
+	return read_check(crashlog, crashlog->info->status.error);
+}
+
+static bool pmt_crashlog_rearm(struct crashlog_entry *crashlog)
+{
+	/* return current value of the crashlog reamed flag */
+	return read_check(crashlog, crashlog->info->status.rearmed);
+}
+
+static void pmt_crashlog_set_rearm(struct crashlog_entry *crashlog)
+{
+	read_modify_write(crashlog, crashlog->info->control.rearm, SET);
+}
+
 /*
  * sysfs
  */
+static ssize_t
+clear_show(struct device *dev, struct device_attribute *attr, char *buf)
+{
+	struct crashlog_entry *crashlog = dev_get_drvdata(dev);
+	bool cleared = pmt_crashlog_cleared(crashlog);
+
+	return sysfs_emit(buf, "%d\n", cleared);
+}
+
+static ssize_t
+clear_store(struct device *dev, struct device_attribute *attr,
+	    const char *buf, size_t count)
+{
+	struct crashlog_entry *crashlog;
+	bool clear;
+	int result;
+
+	crashlog = dev_get_drvdata(dev);
+
+	result = kstrtobool(buf, &clear);
+	if (result)
+		return result;
+
+	/* set bit only */
+	if (!clear)
+		return -EINVAL;
+
+	guard(mutex)(&crashlog->control_mutex);
+
+	pmt_crashlog_set_clear(crashlog);
+
+	return count;
+}
+static DEVICE_ATTR_RW(clear);
+
+static ssize_t
+consumed_show(struct device *dev, struct device_attribute *attr, char *buf)
+{
+	struct crashlog_entry *crashlog = dev_get_drvdata(dev);
+	bool consumed = pmt_crashlog_consumed(crashlog);
+
+	return sysfs_emit(buf, "%d\n", consumed);
+}
+
+static ssize_t consumed_store(struct device *dev, struct device_attribute *attr,
+			      const char *buf, size_t count)
+{
+	struct crashlog_entry *crashlog;
+	bool consumed;
+	int result;
+
+	crashlog = dev_get_drvdata(dev);
+
+	result = kstrtobool(buf, &consumed);
+	if (result)
+		return result;
+
+	/* set bit only */
+	if (!consumed)
+		return -EINVAL;
+
+	guard(mutex)(&crashlog->control_mutex);
+
+	if (pmt_crashlog_disabled(crashlog))
+		return -EBUSY;
+
+	if (!pmt_crashlog_complete(crashlog))
+		return -EEXIST;
+
+	pmt_crashlog_set_consumed(crashlog);
+
+	return count;
+}
+static DEVICE_ATTR_RW(consumed);
+
 static ssize_t
 enable_show(struct device *dev, struct device_attribute *attr, char *buf)
 {
@@ -209,6 +377,50 @@ enable_store(struct device *dev, struct device_attribute *attr,
 }
 static DEVICE_ATTR_RW(enable);
 
+static ssize_t
+error_show(struct device *dev, struct device_attribute *attr, char *buf)
+{
+	struct crashlog_entry *crashlog = dev_get_drvdata(dev);
+	bool error = pmt_crashlog_error(crashlog);
+
+	return sysfs_emit(buf, "%d\n", error);
+}
+static DEVICE_ATTR_RO(error);
+
+static ssize_t
+rearm_show(struct device *dev, struct device_attribute *attr, char *buf)
+{
+	struct crashlog_entry *crashlog = dev_get_drvdata(dev);
+	int rearmed = pmt_crashlog_rearm(crashlog);
+
+	return sysfs_emit(buf, "%d\n", rearmed);
+}
+
+static ssize_t rearm_store(struct device *dev, struct device_attribute *attr,
+			   const char *buf, size_t count)
+{
+	struct crashlog_entry *crashlog;
+	bool rearm;
+	int result;
+
+	crashlog = dev_get_drvdata(dev);
+
+	result = kstrtobool(buf, &rearm);
+	if (result)
+		return result;
+
+	/* set only */
+	if (!rearm)
+		return -EINVAL;
+
+	guard(mutex)(&crashlog->control_mutex);
+
+	pmt_crashlog_set_rearm(crashlog);
+
+	return count;
+}
+static DEVICE_ATTR_RW(rearm);
+
 static ssize_t
 trigger_show(struct device *dev, struct device_attribute *attr, char *buf)
 {
@@ -263,24 +475,57 @@ static struct attribute *pmt_crashlog_attrs[] = {
 	NULL
 };
 
+static struct attribute *pmt_crashlog_ver2_attrs[] = {
+	&dev_attr_clear.attr,
+	&dev_attr_consumed.attr,
+	&dev_attr_enable.attr,
+	&dev_attr_error.attr,
+	&dev_attr_rearm.attr,
+	&dev_attr_trigger.attr,
+	NULL
+};
+
 static const struct attribute_group pmt_crashlog_group = {
 	.attrs	= pmt_crashlog_attrs,
 };
 
+static const struct attribute_group pmt_crashlog_ver2_group = {
+	.attrs = pmt_crashlog_ver2_attrs,
+};
+
+static const struct crashlog_info *select_crashlog_info(u32 type, u32 version)
+{
+	if (version == 0)
+		return &crashlog_type1_ver0;
+
+	return &crashlog_type1_ver2;
+}
+
+static const struct attribute_group *select_sysfs_grp(u32 type, u32 version)
+{
+	if (version == 0)
+		return &pmt_crashlog_group;
+
+	return &pmt_crashlog_ver2_group;
+}
+
 static int pmt_crashlog_header_decode(struct intel_pmt_entry *entry,
 				      struct device *dev)
 {
 	void __iomem *disc_table = entry->disc_table;
 	struct intel_pmt_header *header = &entry->header;
 	struct crashlog_entry *crashlog;
+	u32 version;
+	u32 type;
 
-	if (!pmt_crashlog_supported(entry))
+	if (!pmt_crashlog_supported(entry, &type, &version))
 		return 1;
 
 	/* initialize the crashlog struct */
 	crashlog = container_of(entry, struct crashlog_entry, entry);
 	mutex_init(&crashlog->control_mutex);
-	crashlog->info = &crashlog_type1_ver0;
+
+	crashlog->info = select_crashlog_info(type, version);
 
 	header->access_type = GET_ACCESS(readl(disc_table));
 	header->guid = readl(disc_table + GUID_OFFSET);
@@ -289,7 +534,7 @@ static int pmt_crashlog_header_decode(struct intel_pmt_entry *entry,
 	/* Size is measured in DWORDS, but accessor returns bytes */
 	header->size = GET_SIZE(readl(disc_table + SIZE_OFFSET));
 
-	entry->attr_grp = &pmt_crashlog_group;
+	entry->attr_grp = select_sysfs_grp(type, version);
 
 	return 0;
 }
-- 
2.49.0

Re: [PATCH v3 01/11] platform/x86/intel: refactor endpoint usage

[David E. Box]

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

RE: [PATCH v3 01/11] platform/x86/intel: refactor endpoint usage

[Ruhl, Michael J]

>-----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

Re: [PATCH v3 02/11] platform/x86/intel/pmt: crashlog binary file endpoint

[David E. Box]

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,

Re: [PATCH v3 03/11] drm/xe: Correct BMG VSEC header sizing

[David E. Box]

On Thu, 2025-06-05 at 14:44 -0400, Michael J. Ruhl wrote:
> The intel_vsec_header information for the crashlog feature is
> incorrect.
> 
> Update the VSEC header with correct sizing and count.
> 
> Since the crashlog entries are "merged" (num_entries = 2), the
> separate capabilities entries must be merged as well.
> 
> Signed-off-by: Michael J. Ruhl <michael.j.ruhl@intel.com>

Fixes tag?

David

> ---
>  drivers/gpu/drm/xe/xe_vsec.c | 20 +++++---------------
>  1 file changed, 5 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/gpu/drm/xe/xe_vsec.c b/drivers/gpu/drm/xe/xe_vsec.c
> index 3e573b0b7ebd..67238fc57a4d 100644
> --- a/drivers/gpu/drm/xe/xe_vsec.c
> +++ b/drivers/gpu/drm/xe/xe_vsec.c
> @@ -32,28 +32,18 @@ static struct intel_vsec_header bmg_telemetry = {
>  	.offset = BMG_DISCOVERY_OFFSET,
>  };
>  
> -static struct intel_vsec_header bmg_punit_crashlog = {
> -	.length = 0x10,
> +static struct intel_vsec_header bmg_crashlog = {
> +	.length = 0x18,
>  	.id = VSEC_ID_CRASHLOG,
> -	.num_entries = 1,
> -	.entry_size = 4,
> +	.num_entries = 2,
> +	.entry_size = 6,
>  	.tbir = 0,
>  	.offset = BMG_DISCOVERY_OFFSET + 0x60,
>  };
>  
> -static struct intel_vsec_header bmg_oobmsm_crashlog = {
> -	.length = 0x10,
> -	.id = VSEC_ID_CRASHLOG,
> -	.num_entries = 1,
> -	.entry_size = 4,
> -	.tbir = 0,
> -	.offset = BMG_DISCOVERY_OFFSET + 0x78,
> -};
> -
>  static struct intel_vsec_header *bmg_capabilities[] = {
>  	&bmg_telemetry,
> -	&bmg_punit_crashlog,
> -	&bmg_oobmsm_crashlog,
> +	&bmg_crashlog,
>  	NULL
>  };
>  

Re: [PATCH v3 04/11] platform/x86/intel/pmt: white space cleanup

[David E. Box]

On Thu, 2025-06-05 at 14:44 -0400, Michael J. Ruhl wrote:
> Noticed two white space issues; cleaned them.
> 
> Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> Signed-off-by: Michael J. Ruhl <michael.j.ruhl@intel.com>
> ---
>  drivers/platform/x86/intel/pmt/crashlog.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/platform/x86/intel/pmt/crashlog.c
> b/drivers/platform/x86/intel/pmt/crashlog.c
> index 74ce199e59f0..e997fc48b9ce 100644
> --- a/drivers/platform/x86/intel/pmt/crashlog.c
> +++ b/drivers/platform/x86/intel/pmt/crashlog.c
> @@ -143,7 +143,7 @@ enable_show(struct device *dev, struct device_attribute
> *attr, char *buf)
>  
>  static ssize_t
>  enable_store(struct device *dev, struct device_attribute *attr,
> -	    const char *buf, size_t count)
> +	     const char *buf, size_t count)
>  {
>  	struct crashlog_entry *entry;
>  	bool enabled;
> @@ -177,7 +177,7 @@ trigger_show(struct device *dev, struct device_attribute
> *attr, char *buf)
>  
>  static ssize_t
>  trigger_store(struct device *dev, struct device_attribute *attr,
> -	    const char *buf, size_t count)
> +	      const char *buf, size_t count)
>  {
>  	struct crashlog_entry *entry;
>  	bool trigger;

Reviewed-by: David E. Box <david.e.box@linux.intel.com>

Re: [PATCH v3 05/11] platform/x86/intel/pmt: use guard(mutex)

[David E. Box]

On Thu, 2025-06-05 at 14:44 -0400, Michael J. Ruhl wrote:
> Update the mutex paths to use the new guard() mechanism.
> 
> With the removal of goto, do some minor cleanup of the current logic
> path.
> 
> Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> Signed-off-by: Michael J. Ruhl <michael.j.ruhl@intel.com>
> ---
>  drivers/platform/x86/intel/pmt/crashlog.c | 32 +++++++++++------------
>  1 file changed, 15 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/platform/x86/intel/pmt/crashlog.c
> b/drivers/platform/x86/intel/pmt/crashlog.c
> index e997fc48b9ce..94858bfb52f8 100644
> --- a/drivers/platform/x86/intel/pmt/crashlog.c
> +++ b/drivers/platform/x86/intel/pmt/crashlog.c
> @@ -155,9 +155,9 @@ enable_store(struct device *dev, struct device_attribute
> *attr,
>  	if (result)
>  		return result;
>  
> -	mutex_lock(&entry->control_mutex);
> +	guard(mutex)(&entry->control_mutex);
> +
>  	pmt_crashlog_set_disable(&entry->entry, !enabled);
> -	mutex_unlock(&entry->control_mutex);
>  
>  	return count;
>  }
> @@ -189,26 +189,24 @@ trigger_store(struct device *dev, struct
> device_attribute *attr,
>  	if (result)
>  		return result;
>  
> -	mutex_lock(&entry->control_mutex);
> +	guard(mutex)(&entry->control_mutex);
>  
>  	if (!trigger) {
>  		pmt_crashlog_set_clear(&entry->entry);
> -	} else if (pmt_crashlog_complete(&entry->entry)) {
> -		/* we cannot trigger a new crash if one is still pending */
> -		result = -EEXIST;
> -		goto err;
> -	} else if (pmt_crashlog_disabled(&entry->entry)) {
> -		/* if device is currently disabled, return busy */
> -		result = -EBUSY;
> -		goto err;
> -	} else {
> -		pmt_crashlog_set_execute(&entry->entry);
> +		return count;
>  	}
>  
> -	result = count;
> -err:
> -	mutex_unlock(&entry->control_mutex);
> -	return result;
> +	/* we cannot trigger a new crash if one is still pending */
> +	if (pmt_crashlog_complete(&entry->entry))
> +		return -EEXIST;
> +
> +	/* if device is currently disabled, return busy */
> +	if (pmt_crashlog_disabled(&entry->entry))
> +		return -EBUSY;
> +
> +	pmt_crashlog_set_execute(&entry->entry);
> +
> +	return count;
>  }
>  static DEVICE_ATTR_RW(trigger);
>  

Don't see include for cleanup.h. If add then

Reviewed-by: David E. Box <david.e.box@linux.intel.com>

Re: [PATCH v3 06/11] platform/x86/intel/pmt: re-order trigger logic

[David E. Box]

On Thu, 2025-06-05 at 14:44 -0400, Michael J. Ruhl wrote:
> Setting the clear bit or checking the complete bit before checking to
> see if crashlog is disabled seems incorrect.
> 
> Check disable before accessing any other bits.
> 
> Signed-off-by: Michael J. Ruhl <michael.j.ruhl@intel.com>
> ---
>  drivers/platform/x86/intel/pmt/crashlog.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/platform/x86/intel/pmt/crashlog.c
> b/drivers/platform/x86/intel/pmt/crashlog.c
> index 94858bfb52f8..aa3f57fbe018 100644
> --- a/drivers/platform/x86/intel/pmt/crashlog.c
> +++ b/drivers/platform/x86/intel/pmt/crashlog.c
> @@ -191,6 +191,10 @@ trigger_store(struct device *dev, struct device_attribute
> *attr,
>  
>  	guard(mutex)(&entry->control_mutex);
>  
> +	/* if device is currently disabled, return busy */
> +	if (pmt_crashlog_disabled(&entry->entry))
> +		return -EBUSY;
> +
>  	if (!trigger) {
>  		pmt_crashlog_set_clear(&entry->entry);
>  		return count;
> @@ -200,10 +204,6 @@ trigger_store(struct device *dev, struct device_attribute
> *attr,
>  	if (pmt_crashlog_complete(&entry->entry))
>  		return -EEXIST;
>  
> -	/* if device is currently disabled, return busy */
> -	if (pmt_crashlog_disabled(&entry->entry))
> -		return -EBUSY;
> -
>  	pmt_crashlog_set_execute(&entry->entry);
>  
>  	return count;

LGTM

Reviewed-by: David E. Box <david.e.box@linux.intel.com>

Re: [PATCH v3 07/11] platform/x86/intel/pmt: correct types

[David E. Box]

On Thu, 2025-06-05 at 14:44 -0400, Michael J. Ruhl wrote:
> A couple of auto variables do not match the return types of some of
> the functions.
> 
> Update the mismatched types to match.
> 
> Signed-off-by: Michael J. Ruhl <michael.j.ruhl@intel.com>
> ---
>  drivers/platform/x86/intel/pmt/crashlog.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/platform/x86/intel/pmt/crashlog.c
> b/drivers/platform/x86/intel/pmt/crashlog.c
> index aa3f57fbe018..a51923d0be72 100644
> --- a/drivers/platform/x86/intel/pmt/crashlog.c
> +++ b/drivers/platform/x86/intel/pmt/crashlog.c
> @@ -136,7 +136,7 @@ static ssize_t
>  enable_show(struct device *dev, struct device_attribute *attr, char *buf)
>  {
>  	struct intel_pmt_entry *entry = dev_get_drvdata(dev);
> -	int enabled = !pmt_crashlog_disabled(entry);
> +	bool enabled = !pmt_crashlog_disabled(entry);
>  
>  	return sprintf(buf, "%d\n", enabled);
>  }
> @@ -167,7 +167,7 @@ static ssize_t
>  trigger_show(struct device *dev, struct device_attribute *attr, char *buf)
>  {
>  	struct intel_pmt_entry *entry;
> -	int trigger;
> +	bool trigger;
>  
>  	entry = dev_get_drvdata(dev);
>  	trigger = pmt_crashlog_complete(entry);

Reviewed-by: David E. Box <david.e.box@linux.intel.com>

Re: [PATCH v3 10/11] platform/x86/intel/pmt: use a version struct

[David E. Box]

On Thu, 2025-06-05 at 14:44 -0400, Michael J. Ruhl wrote:
> In preparation for supporting multiple crashlog versions, use a struct
> to keep bit offset info for the status and control bits.
> 
> Signed-off-by: Michael J. Ruhl <michael.j.ruhl@intel.com>
> ---
>  drivers/platform/x86/intel/pmt/crashlog.c | 176 ++++++++++++++--------
>  1 file changed, 112 insertions(+), 64 deletions(-)
> 
> diff --git a/drivers/platform/x86/intel/pmt/crashlog.c
> b/drivers/platform/x86/intel/pmt/crashlog.c
> index 318d7a21f00e..fe6563721886 100644
> --- a/drivers/platform/x86/intel/pmt/crashlog.c
> +++ b/drivers/platform/x86/intel/pmt/crashlog.c
> @@ -22,21 +22,6 @@
>  /* Crashlog discovery header types */
>  #define CRASH_TYPE_OOBMSM	1
>  
> -/* Control Flags */
> -#define CRASHLOG_FLAG_DISABLE		BIT(28)
> -
> -/*
> - * Bits 29 and 30 control the state of bit 31.
> - *
> - * Bit 29 will clear bit 31, if set, allowing a new crashlog to be captured.
> - * Bit 30 will immediately trigger a crashlog to be generated, setting bit
> 31.
> - * Bit 31 is the read-only status with a 1 indicating log is complete.
> - */
> -#define CRASHLOG_FLAG_TRIGGER_CLEAR	BIT(29)
> -#define CRASHLOG_FLAG_TRIGGER_EXECUTE	BIT(30)
> -#define CRASHLOG_FLAG_TRIGGER_COMPLETE	BIT(31)
> -#define CRASHLOG_FLAG_TRIGGER_MASK	GENMASK(31, 28)
> -
>  /* Crashlog Discovery Header */
>  #define CONTROL_OFFSET		0x0
>  #define GUID_OFFSET		0x4
> @@ -48,10 +33,63 @@
>  /* size is in bytes */
>  #define GET_SIZE(v)		((v) * sizeof(u32))
>  
> +/*
> + * Type 1 Version 0
> + * status and control registers are combined.
> + *
> + * Bits 29 and 30 control the state of bit 31.
> + * Bit 29 will clear bit 31, if set, allowing a new crashlog to be captured.
> + * Bit 30 will immediately trigger a crashlog to be generated, setting bit
> 31.
> + * Bit 31 is the read-only status with a 1 indicating log is complete.
> + */
> +#define TYPE1_VER0_STATUS_OFFSET	0x00
> +#define TYPE1_VER0_CONTROL_OFFSET	0x00
> +
> +#define TYPE1_VER0_DISABLE		BIT(28)
> +#define TYPE1_VER0_CLEAR		BIT(29)
> +#define TYPE1_VER0_EXECUTE		BIT(30)
> +#define TYPE1_VER0_COMPLETE		BIT(31)
> +#define TYPE1_VER0_TRIGGER_MASK		GENMASK(31, 28)
> +
> +/* After offset, order alphabetically, not bit ordered */
> +struct crashlog_status {
> +	u32 offset;
> +	u32 clear;
> +	u32 complete;
> +	u32 disable;
> +};
> +
> +struct crashlog_control {
> +	u32 offset;
> +	u32 trigger_mask;
> +	u32 clear;
> +	u32 disable;
> +	u32 manual;
> +};
> +
> +struct crashlog_info {
> +	struct crashlog_status status;
> +	struct crashlog_control control;
> +};
> +
> +const struct crashlog_info crashlog_type1_ver0 = {
> +	.status.offset = TYPE1_VER0_STATUS_OFFSET,
> +	.status.clear = TYPE1_VER0_CLEAR,
> +	.status.complete = TYPE1_VER0_COMPLETE,
> +	.status.disable = TYPE1_VER0_DISABLE,
> +
> +	.control.offset = TYPE1_VER0_CONTROL_OFFSET,
> +	.control.trigger_mask = TYPE1_VER0_TRIGGER_MASK,
> +	.control.clear = TYPE1_VER0_CLEAR,
> +	.control.disable = TYPE1_VER0_DISABLE,
> +	.control.manual = TYPE1_VER0_EXECUTE,
> +};
> +

static?

David

>  struct crashlog_entry {
>  	/* entry must be first member of struct */
>  	struct intel_pmt_entry		entry;
>  	struct mutex			control_mutex;
> +	const struct crashlog_info	*info;
>  };
>  
>  struct pmt_crashlog_priv {
> @@ -59,74 +97,82 @@ struct pmt_crashlog_priv {
>  	struct crashlog_entry	entry[];
>  };
>  
> +/*
> + * This is the generic access to a PMT struct. So the use of
> + * struct crashlog_entry
> + * doesn't "make sense" here.
> + */
> +static bool pmt_crashlog_supported(struct intel_pmt_entry *entry)
> +{
> +	u32 discovery_header = readl(entry->disc_table + CONTROL_OFFSET);
> +	u32 crash_type, version;
> +
> +	crash_type = GET_TYPE(discovery_header);
> +	version = GET_VERSION(discovery_header);
> +
> +	/*
> +	 * Currently we only recognize OOBMSM version 0 devices.
> +	 * We can ignore all other crashlog devices in the system.
> +	 */
> +	return crash_type == CRASH_TYPE_OOBMSM && version == 0;
> +}
> +
>  /*
>   * I/O
>   */
> +
>  #define SET     true
>  #define CLEAR   false
>  
> -static void read_modify_write(struct intel_pmt_entry *entry, u32 bit, bool
> set)
> +static void read_modify_write(struct crashlog_entry *crashlog, u32 bit, bool
> set)
>  {
> -	u32 reg = readl(entry->disc_table + CONTROL_OFFSET);
> +	const struct crashlog_control *control = &crashlog->info->control;
> +	struct intel_pmt_entry *entry = &crashlog->entry;
> +	u32 reg = readl(entry->disc_table + control->offset);
>  
> -	reg &= ~CRASHLOG_FLAG_TRIGGER_MASK;
> +	reg &= ~control->trigger_mask;
>  
>  	if (set)
>  		reg |= bit;
>  	else
>  		reg &= bit;
>  
> -	writel(reg, entry->disc_table + CONTROL_OFFSET);
> +	writel(reg, entry->disc_table + control->offset);
>  }
>  
> -static bool read_check(struct intel_pmt_entry *entry, u32 bit)
> +static bool read_check(struct crashlog_entry *crashlog, u32 bit)
>  {
> -	u32 reg = readl(entry->disc_table + CONTROL_OFFSET);
> +	const struct crashlog_status *status = &crashlog->info->status;
> +	u32 reg = readl(crashlog->entry.disc_table + status->offset);
>  
>  	return !!(reg & bit);
>  }
>  
> -static bool pmt_crashlog_complete(struct intel_pmt_entry *entry)
> +static bool pmt_crashlog_complete(struct crashlog_entry *crashlog)
>  {
>  	/* return current value of the crashlog complete flag */
> -	return read_check(entry, CRASHLOG_FLAG_TRIGGER_COMPLETE);
> +	return read_check(crashlog, crashlog->info->status.complete);
>  }
>  
> -static bool pmt_crashlog_disabled(struct intel_pmt_entry *entry)
> +static bool pmt_crashlog_disabled(struct crashlog_entry *crashlog)
>  {
>  	/* return current value of the crashlog disabled flag */
> -	return read_check(entry, CRASHLOG_FLAG_DISABLE);
> -}
> -
> -static bool pmt_crashlog_supported(struct intel_pmt_entry *entry)
> -{
> -	u32 discovery_header = readl(entry->disc_table + CONTROL_OFFSET);
> -	u32 crash_type, version;
> -
> -	crash_type = GET_TYPE(discovery_header);
> -	version = GET_VERSION(discovery_header);
> -
> -	/*
> -	 * Currently we only recognize OOBMSM version 0 devices.
> -	 * We can ignore all other crashlog devices in the system.
> -	 */
> -	return crash_type == CRASH_TYPE_OOBMSM && version == 0;
> +	return read_check(crashlog, crashlog->info->status.disable);
>  }
>  
> -static void pmt_crashlog_set_disable(struct intel_pmt_entry *entry,
> -				     bool disable)
> +static void pmt_crashlog_set_disable(struct crashlog_entry *crashlog, bool
> disable)
>  {
> -	read_modify_write(entry, CRASHLOG_FLAG_DISABLE, disable);
> +	read_modify_write(crashlog, crashlog->info->control.disable,
> disable);
>  }
>  
> -static void pmt_crashlog_set_clear(struct intel_pmt_entry *entry)
> +static void pmt_crashlog_set_clear(struct crashlog_entry *crashlog)
>  {
> -	read_modify_write(entry, CRASHLOG_FLAG_TRIGGER_CLEAR, SET);
> +	read_modify_write(crashlog, crashlog->info->control.clear, SET);
>  }
>  
> -static void pmt_crashlog_set_execute(struct intel_pmt_entry *entry)
> +static void pmt_crashlog_set_execute(struct crashlog_entry *crashlog)
>  {
> -	read_modify_write(entry, CRASHLOG_FLAG_TRIGGER_EXECUTE, SET);
> +	read_modify_write(crashlog, crashlog->info->control.manual, SET);
>  }
>  
>  /*
> @@ -135,8 +181,8 @@ static void pmt_crashlog_set_execute(struct
> intel_pmt_entry *entry)
>  static ssize_t
>  enable_show(struct device *dev, struct device_attribute *attr, char *buf)
>  {
> -	struct intel_pmt_entry *entry = dev_get_drvdata(dev);
> -	bool enabled = !pmt_crashlog_disabled(entry);
> +	struct crashlog_entry *crashlog = dev_get_drvdata(dev);
> +	bool enabled = !pmt_crashlog_disabled(crashlog);
>  
>  	return sprintf(buf, "%d\n", enabled);
>  }
> @@ -145,19 +191,19 @@ static ssize_t
>  enable_store(struct device *dev, struct device_attribute *attr,
>  	     const char *buf, size_t count)
>  {
> -	struct crashlog_entry *entry;
> +	struct crashlog_entry *crashlog;
>  	bool enabled;
>  	int result;
>  
> -	entry = dev_get_drvdata(dev);
> +	crashlog = dev_get_drvdata(dev);
>  
>  	result = kstrtobool(buf, &enabled);
>  	if (result)
>  		return result;
>  
> -	guard(mutex)(&entry->control_mutex);
> +	guard(mutex)(&crashlog->control_mutex);
>  
> -	pmt_crashlog_set_disable(&entry->entry, !enabled);
> +	pmt_crashlog_set_disable(crashlog, !enabled);
>  
>  	return count;
>  }
> @@ -166,11 +212,11 @@ static DEVICE_ATTR_RW(enable);
>  static ssize_t
>  trigger_show(struct device *dev, struct device_attribute *attr, char *buf)
>  {
> -	struct intel_pmt_entry *entry;
> +	struct crashlog_entry *crashlog;
>  	bool trigger;
>  
> -	entry = dev_get_drvdata(dev);
> -	trigger = pmt_crashlog_complete(entry);
> +	crashlog = dev_get_drvdata(dev);
> +	trigger = pmt_crashlog_complete(crashlog);
>  
>  	return sprintf(buf, "%d\n", trigger);
>  }
> @@ -179,32 +225,33 @@ static ssize_t
>  trigger_store(struct device *dev, struct device_attribute *attr,
>  	      const char *buf, size_t count)
>  {
> -	struct crashlog_entry *entry;
> +	struct crashlog_entry *crashlog;
>  	bool trigger;
>  	int result;
>  
> -	entry = dev_get_drvdata(dev);
> +	crashlog = dev_get_drvdata(dev);
>  
>  	result = kstrtobool(buf, &trigger);
>  	if (result)
>  		return result;
>  
> -	guard(mutex)(&entry->control_mutex);
> +	guard(mutex)(&crashlog->control_mutex);
>  
>  	/* if device is currently disabled, return busy */
> -	if (pmt_crashlog_disabled(&entry->entry))
> +	if (pmt_crashlog_disabled(crashlog))
>  		return -EBUSY;
>  
>  	if (!trigger) {
> -		pmt_crashlog_set_clear(&entry->entry);
> +		pmt_crashlog_set_clear(crashlog);
>  		return count;
>  	}
>  
>  	/* we cannot trigger a new crash if one is still pending */
> -	if (pmt_crashlog_complete(&entry->entry))
> +	if (pmt_crashlog_complete(crashlog))
>  		return -EEXIST;
>  
> -	pmt_crashlog_set_execute(&entry->entry);
> +	pmt_crashlog_set_execute(crashlog);
> +
>  
>  	return count;
>  }
> @@ -230,9 +277,10 @@ static int pmt_crashlog_header_decode(struct
> intel_pmt_entry *entry,
>  	if (!pmt_crashlog_supported(entry))
>  		return 1;
>  
> -	/* initialize control mutex */
> +	/* initialize the crashlog struct */
>  	crashlog = container_of(entry, struct crashlog_entry, entry);
>  	mutex_init(&crashlog->control_mutex);
> +	crashlog->info = &crashlog_type1_ver0;
>  
>  	header->access_type = GET_ACCESS(readl(disc_table));
>  	header->guid = readl(disc_table + GUID_OFFSET);

Re: [PATCH v3 11/11] platform/x86/intel/pmt: support BMG crashlog

[David E. Box]

On Thu, 2025-06-05 at 14:44 -0400, Michael J. Ruhl wrote:
> The Battlemage GPU has the type 1 version 2 crashlog feature.
> 
> Update the crashlog driver to support this crashlog version.
> 
> Signed-off-by: Michael J. Ruhl <michael.j.ruhl@intel.com>
> ---
>  drivers/platform/x86/intel/pmt/crashlog.c | 283 ++++++++++++++++++++--
>  1 file changed, 264 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/platform/x86/intel/pmt/crashlog.c
> b/drivers/platform/x86/intel/pmt/crashlog.c
> index fe6563721886..0fb60036a9bb 100644
> --- a/drivers/platform/x86/intel/pmt/crashlog.c
> +++ b/drivers/platform/x86/intel/pmt/crashlog.c
> @@ -51,20 +51,53 @@
>  #define TYPE1_VER0_COMPLETE		BIT(31)
>  #define TYPE1_VER0_TRIGGER_MASK		GENMASK(31, 28)
>  
> +/*
> + * Type 1 Version 2
> + * status and control are two different registers
> + */
> +#define TYPE1_VER2_STATUS_OFFSET	0x00
> +#define TYPE1_VER2_CONTROL_OFFSET	0x14
> +
> +/* status register */
> +#define TYPE1_VER2_CLEAR_SUPPORT	BIT(20)
> +#define TYPE1_VER2_REARMED		BIT(25)
> +#define TYPE1_VER2_ERROR		BIT(26)
> +#define TYPE1_VER2_CONSUMED		BIT(27)
> +#define TYPE1_VER2_DISABLED		BIT(28)
> +#define TYPE1_VER2_CLEARED		BIT(29)
> +#define TYPE1_VER2_IN_PROGRESS		BIT(30)
> +#define TYPE1_VER2_COMPLETE		BIT(31)
> +
> +/* control register */
> +#define TYPE1_VER2_CONSUME		BIT(25)
> +#define TYPE1_VER2_REARM		BIT(28)
> +#define TYPE1_VER2_EXECUTE		BIT(29)
> +#define TYPE1_VER2_CLEAR		BIT(30)
> +#define TYPE1_VER2_DISABLE		BIT(31)
> +#define TYPE1_VER2_TRIGGER_MASK		(TYPE1_VER2_CONSUME |
> TYPE1_VER2_EXECUTE | \
> +					 TYPE1_VER2_CLEAR |
> TYPE1_VER2_DISABLE)
> +
>  /* After offset, order alphabetically, not bit ordered */
>  struct crashlog_status {
>  	u32 offset;
> -	u32 clear;
> +	u32 clear_supported;
> +	u32 cleared;
>  	u32 complete;
> -	u32 disable;
> +	u32 consumed;
> +	u32 disabled;
> +	u32 error;
> +	u32 in_progress;
> +	u32 rearmed;
>  };

Since this struct was just introduced, better to use the intended names in the
original patch to avoid unnecessary churn.

>  
>  struct crashlog_control {
>  	u32 offset;
>  	u32 trigger_mask;
>  	u32 clear;
> +	u32 consume;
>  	u32 disable;
>  	u32 manual;
> +	u32 rearm;
>  };
>  
>  struct crashlog_info {
> @@ -74,9 +107,9 @@ struct crashlog_info {
>  
>  const struct crashlog_info crashlog_type1_ver0 = {
>  	.status.offset = TYPE1_VER0_STATUS_OFFSET,
> -	.status.clear = TYPE1_VER0_CLEAR,
> +	.status.cleared = TYPE1_VER0_CLEAR,
>  	.status.complete = TYPE1_VER0_COMPLETE,
> -	.status.disable = TYPE1_VER0_DISABLE,
> +	.status.disabled = TYPE1_VER0_DISABLE,
>  
>  	.control.offset = TYPE1_VER0_CONTROL_OFFSET,
>  	.control.trigger_mask = TYPE1_VER0_TRIGGER_MASK,
> @@ -85,6 +118,26 @@ const struct crashlog_info crashlog_type1_ver0 = {
>  	.control.manual = TYPE1_VER0_EXECUTE,
>  };
>  
> +const struct crashlog_info crashlog_type1_ver2 = {
> +	.status.offset = TYPE1_VER2_STATUS_OFFSET,
> +	.status.clear_supported = TYPE1_VER2_CLEAR_SUPPORT,
> +	.status.cleared = TYPE1_VER2_CLEARED,
> +	.status.complete = TYPE1_VER2_COMPLETE,
> +	.status.consumed = TYPE1_VER2_CONSUMED,
> +	.status.disabled = TYPE1_VER2_DISABLED,
> +	.status.error = TYPE1_VER2_ERROR,
> +	.status.in_progress = TYPE1_VER2_IN_PROGRESS,
> +	.status.rearmed = TYPE1_VER2_REARMED,
> +
> +	.control.offset = TYPE1_VER2_CONTROL_OFFSET,
> +	.control.trigger_mask = TYPE1_VER2_TRIGGER_MASK,
> +	.control.clear = TYPE1_VER2_CLEAR,
> +	.control.consume = TYPE1_VER2_CONSUME,
> +	.control.disable = TYPE1_VER2_DISABLE,
> +	.control.manual = TYPE1_VER2_EXECUTE,
> +	.control.rearm = TYPE1_VER2_REARM,
> +};
> +
>  struct crashlog_entry {
>  	/* entry must be first member of struct */
>  	struct intel_pmt_entry		entry;
> @@ -99,30 +152,35 @@ struct pmt_crashlog_priv {
>  
>  /*
>   * This is the generic access to a PMT struct. So the use of
> - * struct crashlog_entry
> - * doesn't "make sense" here.
> + *   struct crashlog_entry
> + * doesn't "make sense" here, i.e. use:
> + *   struct intel_pmt_entry
>   */

I don't think this comment is needed. Still, this could have been done in the
patch that added it.

> -static bool pmt_crashlog_supported(struct intel_pmt_entry *entry)
> +static bool pmt_crashlog_supported(struct intel_pmt_entry *entry, u32
> *crash_type, u32 *version)
>  {
>  	u32 discovery_header = readl(entry->disc_table + CONTROL_OFFSET);
> -	u32 crash_type, version;
>  
> -	crash_type = GET_TYPE(discovery_header);
> -	version = GET_VERSION(discovery_header);
> +	*crash_type = GET_TYPE(discovery_header);
> +	*version = GET_VERSION(discovery_header);
>  
>  	/*
> -	 * Currently we only recognize OOBMSM version 0 devices.
> -	 * We can ignore all other crashlog devices in the system.
> +	 * Currently we only recognize OOBMSM (type 1) and version 0 or 2
> +	 * devices.
> +	 *
> +	 * Ignore all other crashlog devices in the system.
>  	 */
> -	return crash_type == CRASH_TYPE_OOBMSM && version == 0;
> +	if (*crash_type == CRASH_TYPE_OOBMSM && (*version == 0 || *version ==
> 2))
> +		return true;
> +
> +	return false;
>  }
>  
>  /*
>   * I/O
>   */
>  
> -#define SET     true
> -#define CLEAR   false
> +#define SET	true
> +#define CLEAR	false

Ditto

>  
>  static void read_modify_write(struct crashlog_entry *crashlog, u32 bit, bool
> set)
>  {
> @@ -157,7 +215,7 @@ static bool pmt_crashlog_complete(struct crashlog_entry
> *crashlog)
>  static bool pmt_crashlog_disabled(struct crashlog_entry *crashlog)
>  {
>  	/* return current value of the crashlog disabled flag */
> -	return read_check(crashlog, crashlog->info->status.disable);
> +	return read_check(crashlog, crashlog->info->status.disabled);
>  }
>  
>  static void pmt_crashlog_set_disable(struct crashlog_entry *crashlog, bool
> disable)
> @@ -175,9 +233,119 @@ static void pmt_crashlog_set_execute(struct
> crashlog_entry *crashlog)
>  	read_modify_write(crashlog, crashlog->info->control.manual, SET);
>  }
>  
> +/* version 2 support */
> +static bool pmt_crashlog_cleared(struct crashlog_entry *crashlog)
> +{
> +	/* return current value of the crashlog cleared flag */
> +	return read_check(crashlog, crashlog->info->status.cleared);
> +}
> +
> +static bool pmt_crashlog_consumed(struct crashlog_entry *crashlog)
> +{
> +	/* return current value of the crashlog consumedflag */
> +	return read_check(crashlog, crashlog->info->status.consumed);
> +}
> +
> +static void pmt_crashlog_set_consumed(struct crashlog_entry *crashlog)
> +{
> +	read_modify_write(crashlog, crashlog->info->control.consume, SET);
> +}
> +
> +static bool pmt_crashlog_error(struct crashlog_entry *crashlog)
> +{
> +	/* return current value of the crashlog error flag */
> +	return read_check(crashlog, crashlog->info->status.error);
> +}
> +
> +static bool pmt_crashlog_rearm(struct crashlog_entry *crashlog)
> +{
> +	/* return current value of the crashlog reamed flag */
> +	return read_check(crashlog, crashlog->info->status.rearmed);
> +}
> +
> +static void pmt_crashlog_set_rearm(struct crashlog_entry *crashlog)
> +{
> +	read_modify_write(crashlog, crashlog->info->control.rearm, SET);
> +}
> +
>  /*
>   * sysfs
>   */
> +static ssize_t
> +clear_show(struct device *dev, struct device_attribute *attr, char *buf)
> +{
> +	struct crashlog_entry *crashlog = dev_get_drvdata(dev);
> +	bool cleared = pmt_crashlog_cleared(crashlog);
> +
> +	return sysfs_emit(buf, "%d\n", cleared);
> +}
> +
> +static ssize_t
> +clear_store(struct device *dev, struct device_attribute *attr,
> +	    const char *buf, size_t count)
> +{
> +	struct crashlog_entry *crashlog;
> +	bool clear;
> +	int result;
> +
> +	crashlog = dev_get_drvdata(dev);
> +
> +	result = kstrtobool(buf, &clear);
> +	if (result)
> +		return result;
> +
> +	/* set bit only */
> +	if (!clear)
> +		return -EINVAL;
> +
> +	guard(mutex)(&crashlog->control_mutex);
> +
> +	pmt_crashlog_set_clear(crashlog);
> +
> +	return count;
> +}
> +static DEVICE_ATTR_RW(clear);
> +
> +static ssize_t
> +consumed_show(struct device *dev, struct device_attribute *attr, char *buf)
> +{
> +	struct crashlog_entry *crashlog = dev_get_drvdata(dev);
> +	bool consumed = pmt_crashlog_consumed(crashlog);
> +
> +	return sysfs_emit(buf, "%d\n", consumed);
> +}
> +
> +static ssize_t consumed_store(struct device *dev, struct device_attribute
> *attr,
> +			      const char *buf, size_t count)
> +{
> +	struct crashlog_entry *crashlog;
> +	bool consumed;
> +	int result;
> +
> +	crashlog = dev_get_drvdata(dev);
> +
> +	result = kstrtobool(buf, &consumed);
> +	if (result)
> +		return result;
> +
> +	/* set bit only */
> +	if (!consumed)
> +		return -EINVAL;
> +
> +	guard(mutex)(&crashlog->control_mutex);
> +
> +	if (pmt_crashlog_disabled(crashlog))
> +		return -EBUSY;
> +
> +	if (!pmt_crashlog_complete(crashlog))
> +		return -EEXIST;
> +
> +	pmt_crashlog_set_consumed(crashlog);
> +
> +	return count;
> +}
> +static DEVICE_ATTR_RW(consumed);
> +
>  static ssize_t
>  enable_show(struct device *dev, struct device_attribute *attr, char *buf)
>  {
> @@ -209,6 +377,50 @@ enable_store(struct device *dev, struct device_attribute
> *attr,
>  }
>  static DEVICE_ATTR_RW(enable);
>  
> +static ssize_t
> +error_show(struct device *dev, struct device_attribute *attr, char *buf)
> +{
> +	struct crashlog_entry *crashlog = dev_get_drvdata(dev);
> +	bool error = pmt_crashlog_error(crashlog);
> +
> +	return sysfs_emit(buf, "%d\n", error);
> +}
> +static DEVICE_ATTR_RO(error);
> +
> +static ssize_t
> +rearm_show(struct device *dev, struct device_attribute *attr, char *buf)
> +{
> +	struct crashlog_entry *crashlog = dev_get_drvdata(dev);
> +	int rearmed = pmt_crashlog_rearm(crashlog);
> +
> +	return sysfs_emit(buf, "%d\n", rearmed);
> +}
> +
> +static ssize_t rearm_store(struct device *dev, struct device_attribute *attr,
> +			   const char *buf, size_t count)
> +{
> +	struct crashlog_entry *crashlog;
> +	bool rearm;
> +	int result;
> +
> +	crashlog = dev_get_drvdata(dev);
> +
> +	result = kstrtobool(buf, &rearm);
> +	if (result)
> +		return result;
> +
> +	/* set only */
> +	if (!rearm)
> +		return -EINVAL;
> +
> +	guard(mutex)(&crashlog->control_mutex);
> +
> +	pmt_crashlog_set_rearm(crashlog);
> +
> +	return count;
> +}
> +static DEVICE_ATTR_RW(rearm);
> +
>  static ssize_t
>  trigger_show(struct device *dev, struct device_attribute *attr, char *buf)
>  {
> @@ -263,24 +475,57 @@ static struct attribute *pmt_crashlog_attrs[] = {
>  	NULL
>  };
> 

For what follows ...

>  
> +static struct attribute *pmt_crashlog_ver2_attrs[] = {
> +	&dev_attr_clear.attr,
> +	&dev_attr_consumed.attr,
> +	&dev_attr_enable.attr,
> +	&dev_attr_error.attr,
> +	&dev_attr_rearm.attr,
> +	&dev_attr_trigger.attr,
> +	NULL
> +};
> +
>  static const struct attribute_group pmt_crashlog_group = {
>  	.attrs	= pmt_crashlog_attrs,
>  };
>  
> +static const struct attribute_group pmt_crashlog_ver2_group = {
> +	.attrs = pmt_crashlog_ver2_attrs,
> +};

... I would have the group and attributes both reflect type and version in the
name. Update pmt_crashlog_group as well.

David

> +
> +static const struct crashlog_info *select_crashlog_info(u32 type, u32
> version)
> +{
> +	if (version == 0)
> +		return &crashlog_type1_ver0;
> +
> +	return &crashlog_type1_ver2;
> +}
> +
> +static const struct attribute_group *select_sysfs_grp(u32 type, u32 version)
> +{
> +	if (version == 0)
> +		return &pmt_crashlog_group;
> +
> +	return &pmt_crashlog_ver2_group;
> +}
> +
>  static int pmt_crashlog_header_decode(struct intel_pmt_entry *entry,
>  				      struct device *dev)
>  {
>  	void __iomem *disc_table = entry->disc_table;
>  	struct intel_pmt_header *header = &entry->header;
>  	struct crashlog_entry *crashlog;
> +	u32 version;
> +	u32 type;
>  
> -	if (!pmt_crashlog_supported(entry))
> +	if (!pmt_crashlog_supported(entry, &type, &version))
>  		return 1;
>  
>  	/* initialize the crashlog struct */
>  	crashlog = container_of(entry, struct crashlog_entry, entry);
>  	mutex_init(&crashlog->control_mutex);
> -	crashlog->info = &crashlog_type1_ver0;
> +
> +	crashlog->info = select_crashlog_info(type, version);
>  
>  	header->access_type = GET_ACCESS(readl(disc_table));
>  	header->guid = readl(disc_table + GUID_OFFSET);
> @@ -289,7 +534,7 @@ static int pmt_crashlog_header_decode(struct
> intel_pmt_entry *entry,
>  	/* Size is measured in DWORDS, but accessor returns bytes */
>  	header->size = GET_SIZE(readl(disc_table + SIZE_OFFSET));
>  
> -	entry->attr_grp = &pmt_crashlog_group;
> +	entry->attr_grp = select_sysfs_grp(type, version);
>  
>  	return 0;
>  }

RE: [PATCH v3 03/11] drm/xe: Correct BMG VSEC header sizing

[Ruhl, Michael J]

>-----Original Message-----
>From: David E. Box <david.e.box@linux.intel.com>
>Sent: Friday, June 6, 2025 5:13 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
>Subject: Re: [PATCH v3 03/11] drm/xe: Correct BMG VSEC header sizing
>
>On Thu, 2025-06-05 at 14:44 -0400, Michael J. Ruhl wrote:
>> The intel_vsec_header information for the crashlog feature is
>> incorrect.
>>
>> Update the VSEC header with correct sizing and count.
>>
>> Since the crashlog entries are "merged" (num_entries = 2), the
>> separate capabilities entries must be merged as well.
>>
>> Signed-off-by: Michael J. Ruhl <michael.j.ruhl@intel.com>
>
>Fixes tag?

Since the crashlog feature isn't available until this series, a fixes would not address any
issues.

So I didn't think it was necessary.  If you would like one added, I will update.

M

>David
>
>> ---
>>  drivers/gpu/drm/xe/xe_vsec.c | 20 +++++---------------
>>  1 file changed, 5 insertions(+), 15 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/xe/xe_vsec.c b/drivers/gpu/drm/xe/xe_vsec.c
>> index 3e573b0b7ebd..67238fc57a4d 100644
>> --- a/drivers/gpu/drm/xe/xe_vsec.c
>> +++ b/drivers/gpu/drm/xe/xe_vsec.c
>> @@ -32,28 +32,18 @@ static struct intel_vsec_header bmg_telemetry = {
>>  	.offset = BMG_DISCOVERY_OFFSET,
>>  };
>>
>> -static struct intel_vsec_header bmg_punit_crashlog = {
>> -	.length = 0x10,
>> +static struct intel_vsec_header bmg_crashlog = {
>> +	.length = 0x18,
>>  	.id = VSEC_ID_CRASHLOG,
>> -	.num_entries = 1,
>> -	.entry_size = 4,
>> +	.num_entries = 2,
>> +	.entry_size = 6,
>>  	.tbir = 0,
>>  	.offset = BMG_DISCOVERY_OFFSET + 0x60,
>>  };
>>
>> -static struct intel_vsec_header bmg_oobmsm_crashlog = {
>> -	.length = 0x10,
>> -	.id = VSEC_ID_CRASHLOG,
>> -	.num_entries = 1,
>> -	.entry_size = 4,
>> -	.tbir = 0,
>> -	.offset = BMG_DISCOVERY_OFFSET + 0x78,
>> -};
>> -
>>  static struct intel_vsec_header *bmg_capabilities[] = {
>>  	&bmg_telemetry,
>> -	&bmg_punit_crashlog,
>> -	&bmg_oobmsm_crashlog,
>> +	&bmg_crashlog,
>>  	NULL
>>  };
>>

RE: [PATCH v3 05/11] platform/x86/intel/pmt: use guard(mutex)

[Ruhl, Michael J]

>-----Original Message-----
>From: David E. Box <david.e.box@linux.intel.com>
>Sent: Friday, June 6, 2025 5:21 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
>Subject: Re: [PATCH v3 05/11] platform/x86/intel/pmt: use guard(mutex)
>
>On Thu, 2025-06-05 at 14:44 -0400, Michael J. Ruhl wrote:
>> Update the mutex paths to use the new guard() mechanism.
>>
>> With the removal of goto, do some minor cleanup of the current logic
>> path.
>>
>> Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
>> Signed-off-by: Michael J. Ruhl <michael.j.ruhl@intel.com>
>> ---
>>  drivers/platform/x86/intel/pmt/crashlog.c | 32 +++++++++++------------
>>  1 file changed, 15 insertions(+), 17 deletions(-)
>>
>> diff --git a/drivers/platform/x86/intel/pmt/crashlog.c
>> b/drivers/platform/x86/intel/pmt/crashlog.c
>> index e997fc48b9ce..94858bfb52f8 100644
>> --- a/drivers/platform/x86/intel/pmt/crashlog.c
>> +++ b/drivers/platform/x86/intel/pmt/crashlog.c
>> @@ -155,9 +155,9 @@ enable_store(struct device *dev, struct
>device_attribute
>> *attr,
>>  	if (result)
>>  		return result;
>>
>> -	mutex_lock(&entry->control_mutex);
>> +	guard(mutex)(&entry->control_mutex);
>> +
>>  	pmt_crashlog_set_disable(&entry->entry, !enabled);
>> -	mutex_unlock(&entry->control_mutex);
>>
>>  	return count;
>>  }
>> @@ -189,26 +189,24 @@ trigger_store(struct device *dev, struct
>> device_attribute *attr,
>>  	if (result)
>>  		return result;
>>
>> -	mutex_lock(&entry->control_mutex);
>> +	guard(mutex)(&entry->control_mutex);
>>
>>  	if (!trigger) {
>>  		pmt_crashlog_set_clear(&entry->entry);
>> -	} else if (pmt_crashlog_complete(&entry->entry)) {
>> -		/* we cannot trigger a new crash if one is still pending */
>> -		result = -EEXIST;
>> -		goto err;
>> -	} else if (pmt_crashlog_disabled(&entry->entry)) {
>> -		/* if device is currently disabled, return busy */
>> -		result = -EBUSY;
>> -		goto err;
>> -	} else {
>> -		pmt_crashlog_set_execute(&entry->entry);
>> +		return count;
>>  	}
>>
>> -	result = count;
>> -err:
>> -	mutex_unlock(&entry->control_mutex);
>> -	return result;
>> +	/* we cannot trigger a new crash if one is still pending */
>> +	if (pmt_crashlog_complete(&entry->entry))
>> +		return -EEXIST;
>> +
>> +	/* if device is currently disabled, return busy */
>> +	if (pmt_crashlog_disabled(&entry->entry))
>> +		return -EBUSY;
>> +
>> +	pmt_crashlog_set_execute(&entry->entry);
>> +
>> +	return count;
>>  }
>>  static DEVICE_ATTR_RW(trigger);
>>
>
>Don't see include for cleanup.h. If add then

Missed that.  I will update the file.

>Reviewed-by: David E. Box <david.e.box@linux.intel.com>

Thank!

m

RE: [PATCH v3 10/11] platform/x86/intel/pmt: use a version struct

[Ruhl, Michael J]

>-----Original Message-----
>From: David E. Box <david.e.box@linux.intel.com>
>Sent: Friday, June 6, 2025 6:58 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
>Subject: Re: [PATCH v3 10/11] platform/x86/intel/pmt: use a version struct
>
>On Thu, 2025-06-05 at 14:44 -0400, Michael J. Ruhl wrote:
>> In preparation for supporting multiple crashlog versions, use a struct
>> to keep bit offset info for the status and control bits.
>>
>> Signed-off-by: Michael J. Ruhl <michael.j.ruhl@intel.com>
>> ---
>>  drivers/platform/x86/intel/pmt/crashlog.c | 176 ++++++++++++++--------
>>  1 file changed, 112 insertions(+), 64 deletions(-)
>>
>> diff --git a/drivers/platform/x86/intel/pmt/crashlog.c
>> b/drivers/platform/x86/intel/pmt/crashlog.c
>> index 318d7a21f00e..fe6563721886 100644
>> --- a/drivers/platform/x86/intel/pmt/crashlog.c
>> +++ b/drivers/platform/x86/intel/pmt/crashlog.c
>> @@ -22,21 +22,6 @@
>>  /* Crashlog discovery header types */
>>  #define CRASH_TYPE_OOBMSM	1
>>
>> -/* Control Flags */
>> -#define CRASHLOG_FLAG_DISABLE		BIT(28)
>> -
>> -/*
>> - * Bits 29 and 30 control the state of bit 31.
>> - *
>> - * Bit 29 will clear bit 31, if set, allowing a new crashlog to be captured.
>> - * Bit 30 will immediately trigger a crashlog to be generated, setting bit
>> 31.
>> - * Bit 31 is the read-only status with a 1 indicating log is complete.
>> - */
>> -#define CRASHLOG_FLAG_TRIGGER_CLEAR	BIT(29)
>> -#define CRASHLOG_FLAG_TRIGGER_EXECUTE	BIT(30)
>> -#define CRASHLOG_FLAG_TRIGGER_COMPLETE	BIT(31)
>> -#define CRASHLOG_FLAG_TRIGGER_MASK	GENMASK(31, 28)
>> -
>>  /* Crashlog Discovery Header */
>>  #define CONTROL_OFFSET		0x0
>>  #define GUID_OFFSET		0x4
>> @@ -48,10 +33,63 @@
>>  /* size is in bytes */
>>  #define GET_SIZE(v)		((v) * sizeof(u32))
>>
>> +/*
>> + * Type 1 Version 0
>> + * status and control registers are combined.
>> + *
>> + * Bits 29 and 30 control the state of bit 31.
>> + * Bit 29 will clear bit 31, if set, allowing a new crashlog to be captured.
>> + * Bit 30 will immediately trigger a crashlog to be generated, setting bit
>> 31.
>> + * Bit 31 is the read-only status with a 1 indicating log is complete.
>> + */
>> +#define TYPE1_VER0_STATUS_OFFSET	0x00
>> +#define TYPE1_VER0_CONTROL_OFFSET	0x00
>> +
>> +#define TYPE1_VER0_DISABLE		BIT(28)
>> +#define TYPE1_VER0_CLEAR		BIT(29)
>> +#define TYPE1_VER0_EXECUTE		BIT(30)
>> +#define TYPE1_VER0_COMPLETE		BIT(31)
>> +#define TYPE1_VER0_TRIGGER_MASK		GENMASK(31, 28)
>> +
>> +/* After offset, order alphabetically, not bit ordered */
>> +struct crashlog_status {
>> +	u32 offset;
>> +	u32 clear;
>> +	u32 complete;
>> +	u32 disable;
>> +};
>> +
>> +struct crashlog_control {
>> +	u32 offset;
>> +	u32 trigger_mask;
>> +	u32 clear;
>> +	u32 disable;
>> +	u32 manual;
>> +};
>> +
>> +struct crashlog_info {
>> +	struct crashlog_status status;
>> +	struct crashlog_control control;
>> +};
>> +
>> +const struct crashlog_info crashlog_type1_ver0 = {
>> +	.status.offset = TYPE1_VER0_STATUS_OFFSET,
>> +	.status.clear = TYPE1_VER0_CLEAR,
>> +	.status.complete = TYPE1_VER0_COMPLETE,
>> +	.status.disable = TYPE1_VER0_DISABLE,
>> +
>> +	.control.offset = TYPE1_VER0_CONTROL_OFFSET,
>> +	.control.trigger_mask = TYPE1_VER0_TRIGGER_MASK,
>> +	.control.clear = TYPE1_VER0_CLEAR,
>> +	.control.disable = TYPE1_VER0_DISABLE,
>> +	.control.manual = TYPE1_VER0_EXECUTE,
>> +};
>> +
>
>static?

Will update.

M

>David
>
>>  struct crashlog_entry {
>>  	/* entry must be first member of struct */
>>  	struct intel_pmt_entry		entry;
>>  	struct mutex			control_mutex;
>> +	const struct crashlog_info	*info;
>>  };
>>
>>  struct pmt_crashlog_priv {
>> @@ -59,74 +97,82 @@ struct pmt_crashlog_priv {
>>  	struct crashlog_entry	entry[];
>>  };
>>
>> +/*
>> + * This is the generic access to a PMT struct. So the use of
>> + * struct crashlog_entry
>> + * doesn't "make sense" here.
>> + */
>> +static bool pmt_crashlog_supported(struct intel_pmt_entry *entry)
>> +{
>> +	u32 discovery_header = readl(entry->disc_table + CONTROL_OFFSET);
>> +	u32 crash_type, version;
>> +
>> +	crash_type = GET_TYPE(discovery_header);
>> +	version = GET_VERSION(discovery_header);
>> +
>> +	/*
>> +	 * Currently we only recognize OOBMSM version 0 devices.
>> +	 * We can ignore all other crashlog devices in the system.
>> +	 */
>> +	return crash_type == CRASH_TYPE_OOBMSM && version == 0;
>> +}
>> +
>>  /*
>>   * I/O
>>   */
>> +
>>  #define SET     true
>>  #define CLEAR   false
>>
>> -static void read_modify_write(struct intel_pmt_entry *entry, u32 bit, bool
>> set)
>> +static void read_modify_write(struct crashlog_entry *crashlog, u32 bit, bool
>> set)
>>  {
>> -	u32 reg = readl(entry->disc_table + CONTROL_OFFSET);
>> +	const struct crashlog_control *control = &crashlog->info->control;
>> +	struct intel_pmt_entry *entry = &crashlog->entry;
>> +	u32 reg = readl(entry->disc_table + control->offset);
>>
>> -	reg &= ~CRASHLOG_FLAG_TRIGGER_MASK;
>> +	reg &= ~control->trigger_mask;
>>
>>  	if (set)
>>  		reg |= bit;
>>  	else
>>  		reg &= bit;
>>
>> -	writel(reg, entry->disc_table + CONTROL_OFFSET);
>> +	writel(reg, entry->disc_table + control->offset);
>>  }
>>
>> -static bool read_check(struct intel_pmt_entry *entry, u32 bit)
>> +static bool read_check(struct crashlog_entry *crashlog, u32 bit)
>>  {
>> -	u32 reg = readl(entry->disc_table + CONTROL_OFFSET);
>> +	const struct crashlog_status *status = &crashlog->info->status;
>> +	u32 reg = readl(crashlog->entry.disc_table + status->offset);
>>
>>  	return !!(reg & bit);
>>  }
>>
>> -static bool pmt_crashlog_complete(struct intel_pmt_entry *entry)
>> +static bool pmt_crashlog_complete(struct crashlog_entry *crashlog)
>>  {
>>  	/* return current value of the crashlog complete flag */
>> -	return read_check(entry, CRASHLOG_FLAG_TRIGGER_COMPLETE);
>> +	return read_check(crashlog, crashlog->info->status.complete);
>>  }
>>
>> -static bool pmt_crashlog_disabled(struct intel_pmt_entry *entry)
>> +static bool pmt_crashlog_disabled(struct crashlog_entry *crashlog)
>>  {
>>  	/* return current value of the crashlog disabled flag */
>> -	return read_check(entry, CRASHLOG_FLAG_DISABLE);
>> -}
>> -
>> -static bool pmt_crashlog_supported(struct intel_pmt_entry *entry)
>> -{
>> -	u32 discovery_header = readl(entry->disc_table + CONTROL_OFFSET);
>> -	u32 crash_type, version;
>> -
>> -	crash_type = GET_TYPE(discovery_header);
>> -	version = GET_VERSION(discovery_header);
>> -
>> -	/*
>> -	 * Currently we only recognize OOBMSM version 0 devices.
>> -	 * We can ignore all other crashlog devices in the system.
>> -	 */
>> -	return crash_type == CRASH_TYPE_OOBMSM && version == 0;
>> +	return read_check(crashlog, crashlog->info->status.disable);
>>  }
>>
>> -static void pmt_crashlog_set_disable(struct intel_pmt_entry *entry,
>> -				     bool disable)
>> +static void pmt_crashlog_set_disable(struct crashlog_entry *crashlog, bool
>> disable)
>>  {
>> -	read_modify_write(entry, CRASHLOG_FLAG_DISABLE, disable);
>> +	read_modify_write(crashlog, crashlog->info->control.disable,
>> disable);
>>  }
>>
>> -static void pmt_crashlog_set_clear(struct intel_pmt_entry *entry)
>> +static void pmt_crashlog_set_clear(struct crashlog_entry *crashlog)
>>  {
>> -	read_modify_write(entry, CRASHLOG_FLAG_TRIGGER_CLEAR, SET);
>> +	read_modify_write(crashlog, crashlog->info->control.clear, SET);
>>  }
>>
>> -static void pmt_crashlog_set_execute(struct intel_pmt_entry *entry)
>> +static void pmt_crashlog_set_execute(struct crashlog_entry *crashlog)
>>  {
>> -	read_modify_write(entry, CRASHLOG_FLAG_TRIGGER_EXECUTE, SET);
>> +	read_modify_write(crashlog, crashlog->info->control.manual, SET);
>>  }
>>
>>  /*
>> @@ -135,8 +181,8 @@ static void pmt_crashlog_set_execute(struct
>> intel_pmt_entry *entry)
>>  static ssize_t
>>  enable_show(struct device *dev, struct device_attribute *attr, char *buf)
>>  {
>> -	struct intel_pmt_entry *entry = dev_get_drvdata(dev);
>> -	bool enabled = !pmt_crashlog_disabled(entry);
>> +	struct crashlog_entry *crashlog = dev_get_drvdata(dev);
>> +	bool enabled = !pmt_crashlog_disabled(crashlog);
>>
>>  	return sprintf(buf, "%d\n", enabled);
>>  }
>> @@ -145,19 +191,19 @@ static ssize_t
>>  enable_store(struct device *dev, struct device_attribute *attr,
>>  	     const char *buf, size_t count)
>>  {
>> -	struct crashlog_entry *entry;
>> +	struct crashlog_entry *crashlog;
>>  	bool enabled;
>>  	int result;
>>
>> -	entry = dev_get_drvdata(dev);
>> +	crashlog = dev_get_drvdata(dev);
>>
>>  	result = kstrtobool(buf, &enabled);
>>  	if (result)
>>  		return result;
>>
>> -	guard(mutex)(&entry->control_mutex);
>> +	guard(mutex)(&crashlog->control_mutex);
>>
>> -	pmt_crashlog_set_disable(&entry->entry, !enabled);
>> +	pmt_crashlog_set_disable(crashlog, !enabled);
>>
>>  	return count;
>>  }
>> @@ -166,11 +212,11 @@ static DEVICE_ATTR_RW(enable);
>>  static ssize_t
>>  trigger_show(struct device *dev, struct device_attribute *attr, char *buf)
>>  {
>> -	struct intel_pmt_entry *entry;
>> +	struct crashlog_entry *crashlog;
>>  	bool trigger;
>>
>> -	entry = dev_get_drvdata(dev);
>> -	trigger = pmt_crashlog_complete(entry);
>> +	crashlog = dev_get_drvdata(dev);
>> +	trigger = pmt_crashlog_complete(crashlog);
>>
>>  	return sprintf(buf, "%d\n", trigger);
>>  }
>> @@ -179,32 +225,33 @@ static ssize_t
>>  trigger_store(struct device *dev, struct device_attribute *attr,
>>  	      const char *buf, size_t count)
>>  {
>> -	struct crashlog_entry *entry;
>> +	struct crashlog_entry *crashlog;
>>  	bool trigger;
>>  	int result;
>>
>> -	entry = dev_get_drvdata(dev);
>> +	crashlog = dev_get_drvdata(dev);
>>
>>  	result = kstrtobool(buf, &trigger);
>>  	if (result)
>>  		return result;
>>
>> -	guard(mutex)(&entry->control_mutex);
>> +	guard(mutex)(&crashlog->control_mutex);
>>
>>  	/* if device is currently disabled, return busy */
>> -	if (pmt_crashlog_disabled(&entry->entry))
>> +	if (pmt_crashlog_disabled(crashlog))
>>  		return -EBUSY;
>>
>>  	if (!trigger) {
>> -		pmt_crashlog_set_clear(&entry->entry);
>> +		pmt_crashlog_set_clear(crashlog);
>>  		return count;
>>  	}
>>
>>  	/* we cannot trigger a new crash if one is still pending */
>> -	if (pmt_crashlog_complete(&entry->entry))
>> +	if (pmt_crashlog_complete(crashlog))
>>  		return -EEXIST;
>>
>> -	pmt_crashlog_set_execute(&entry->entry);
>> +	pmt_crashlog_set_execute(crashlog);
>> +
>>
>>  	return count;
>>  }
>> @@ -230,9 +277,10 @@ static int pmt_crashlog_header_decode(struct
>> intel_pmt_entry *entry,
>>  	if (!pmt_crashlog_supported(entry))
>>  		return 1;
>>
>> -	/* initialize control mutex */
>> +	/* initialize the crashlog struct */
>>  	crashlog = container_of(entry, struct crashlog_entry, entry);
>>  	mutex_init(&crashlog->control_mutex);
>> +	crashlog->info = &crashlog_type1_ver0;
>>
>>  	header->access_type = GET_ACCESS(readl(disc_table));
>>  	header->guid = readl(disc_table + GUID_OFFSET);

RE: [PATCH v3 11/11] platform/x86/intel/pmt: support BMG crashlog

[Ruhl, Michael J]

>-----Original Message-----
>From: David E. Box <david.e.box@linux.intel.com>
>Sent: Friday, June 6, 2025 7:21 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
>Subject: Re: [PATCH v3 11/11] platform/x86/intel/pmt: support BMG crashlog
>
>On Thu, 2025-06-05 at 14:44 -0400, Michael J. Ruhl wrote:
>> The Battlemage GPU has the type 1 version 2 crashlog feature.
>>
>> Update the crashlog driver to support this crashlog version.
>>
>> Signed-off-by: Michael J. Ruhl <michael.j.ruhl@intel.com>
>> ---
>>  drivers/platform/x86/intel/pmt/crashlog.c | 283 ++++++++++++++++++++-
>-
>>  1 file changed, 264 insertions(+), 19 deletions(-)
>>
>> diff --git a/drivers/platform/x86/intel/pmt/crashlog.c
>> b/drivers/platform/x86/intel/pmt/crashlog.c
>> index fe6563721886..0fb60036a9bb 100644
>> --- a/drivers/platform/x86/intel/pmt/crashlog.c
>> +++ b/drivers/platform/x86/intel/pmt/crashlog.c
>> @@ -51,20 +51,53 @@
>>  #define TYPE1_VER0_COMPLETE		BIT(31)
>>  #define TYPE1_VER0_TRIGGER_MASK		GENMASK(31, 28)
>>
>> +/*
>> + * Type 1 Version 2
>> + * status and control are two different registers
>> + */
>> +#define TYPE1_VER2_STATUS_OFFSET	0x00
>> +#define TYPE1_VER2_CONTROL_OFFSET	0x14
>> +
>> +/* status register */
>> +#define TYPE1_VER2_CLEAR_SUPPORT	BIT(20)
>> +#define TYPE1_VER2_REARMED		BIT(25)
>> +#define TYPE1_VER2_ERROR		BIT(26)
>> +#define TYPE1_VER2_CONSUMED		BIT(27)
>> +#define TYPE1_VER2_DISABLED		BIT(28)
>> +#define TYPE1_VER2_CLEARED		BIT(29)
>> +#define TYPE1_VER2_IN_PROGRESS		BIT(30)
>> +#define TYPE1_VER2_COMPLETE		BIT(31)
>> +
>> +/* control register */
>> +#define TYPE1_VER2_CONSUME		BIT(25)
>> +#define TYPE1_VER2_REARM		BIT(28)
>> +#define TYPE1_VER2_EXECUTE		BIT(29)
>> +#define TYPE1_VER2_CLEAR		BIT(30)
>> +#define TYPE1_VER2_DISABLE		BIT(31)
>> +#define TYPE1_VER2_TRIGGER_MASK
>	(TYPE1_VER2_CONSUME |
>> TYPE1_VER2_EXECUTE | \
>> +					 TYPE1_VER2_CLEAR |
>> TYPE1_VER2_DISABLE)
>> +
>>  /* After offset, order alphabetically, not bit ordered */
>>  struct crashlog_status {
>>  	u32 offset;
>> -	u32 clear;
>> +	u32 clear_supported;
>> +	u32 cleared;
>>  	u32 complete;
>> -	u32 disable;
>> +	u32 consumed;
>> +	u32 disabled;
>> +	u32 error;
>> +	u32 in_progress;
>> +	u32 rearmed;
>>  };
>
>Since this struct was just introduced, better to use the intended names in the
>original patch to avoid unnecessary churn.
>
>>
>>  struct crashlog_control {
>>  	u32 offset;
>>  	u32 trigger_mask;
>>  	u32 clear;
>> +	u32 consume;
>>  	u32 disable;
>>  	u32 manual;
>> +	u32 rearm;
>>  };
>>
>>  struct crashlog_info {
>> @@ -74,9 +107,9 @@ struct crashlog_info {
>>
>>  const struct crashlog_info crashlog_type1_ver0 = {
>>  	.status.offset = TYPE1_VER0_STATUS_OFFSET,
>> -	.status.clear = TYPE1_VER0_CLEAR,
>> +	.status.cleared = TYPE1_VER0_CLEAR,
>>  	.status.complete = TYPE1_VER0_COMPLETE,
>> -	.status.disable = TYPE1_VER0_DISABLE,
>> +	.status.disabled = TYPE1_VER0_DISABLE,
>>
>>  	.control.offset = TYPE1_VER0_CONTROL_OFFSET,
>>  	.control.trigger_mask = TYPE1_VER0_TRIGGER_MASK,
>> @@ -85,6 +118,26 @@ const struct crashlog_info crashlog_type1_ver0 = {
>>  	.control.manual = TYPE1_VER0_EXECUTE,
>>  };
>>
>> +const struct crashlog_info crashlog_type1_ver2 = {
>> +	.status.offset = TYPE1_VER2_STATUS_OFFSET,
>> +	.status.clear_supported = TYPE1_VER2_CLEAR_SUPPORT,
>> +	.status.cleared = TYPE1_VER2_CLEARED,
>> +	.status.complete = TYPE1_VER2_COMPLETE,
>> +	.status.consumed = TYPE1_VER2_CONSUMED,
>> +	.status.disabled = TYPE1_VER2_DISABLED,
>> +	.status.error = TYPE1_VER2_ERROR,
>> +	.status.in_progress = TYPE1_VER2_IN_PROGRESS,
>> +	.status.rearmed = TYPE1_VER2_REARMED,
>> +
>> +	.control.offset = TYPE1_VER2_CONTROL_OFFSET,
>> +	.control.trigger_mask = TYPE1_VER2_TRIGGER_MASK,
>> +	.control.clear = TYPE1_VER2_CLEAR,
>> +	.control.consume = TYPE1_VER2_CONSUME,
>> +	.control.disable = TYPE1_VER2_DISABLE,
>> +	.control.manual = TYPE1_VER2_EXECUTE,
>> +	.control.rearm = TYPE1_VER2_REARM,
>> +};
>> +
>>  struct crashlog_entry {
>>  	/* entry must be first member of struct */
>>  	struct intel_pmt_entry		entry;
>> @@ -99,30 +152,35 @@ struct pmt_crashlog_priv {
>>
>>  /*
>>   * This is the generic access to a PMT struct. So the use of
>> - * struct crashlog_entry
>> - * doesn't "make sense" here.
>> + *   struct crashlog_entry
>> + * doesn't "make sense" here, i.e. use:
>> + *   struct intel_pmt_entry
>>   */
>
>I don't think this comment is needed. Still, this could have been done in the
>patch that added it.

I will remove.

>> -static bool pmt_crashlog_supported(struct intel_pmt_entry *entry)
>> +static bool pmt_crashlog_supported(struct intel_pmt_entry *entry, u32
>> *crash_type, u32 *version)
>>  {
>>  	u32 discovery_header = readl(entry->disc_table + CONTROL_OFFSET);
>> -	u32 crash_type, version;
>>
>> -	crash_type = GET_TYPE(discovery_header);
>> -	version = GET_VERSION(discovery_header);
>> +	*crash_type = GET_TYPE(discovery_header);
>> +	*version = GET_VERSION(discovery_header);
>>
>>  	/*
>> -	 * Currently we only recognize OOBMSM version 0 devices.
>> -	 * We can ignore all other crashlog devices in the system.
>> +	 * Currently we only recognize OOBMSM (type 1) and version 0 or 2
>> +	 * devices.
>> +	 *
>> +	 * Ignore all other crashlog devices in the system.
>>  	 */
>> -	return crash_type == CRASH_TYPE_OOBMSM && version == 0;
>> +	if (*crash_type == CRASH_TYPE_OOBMSM && (*version == 0 ||
>*version ==
>> 2))
>> +		return true;
>> +
>> +	return false;
>>  }
>>
>>  /*
>>   * I/O
>>   */
>>
>> -#define SET     true
>> -#define CLEAR   false
>> +#define SET	true
>> +#define CLEAR	false
>
>Ditto

Not sure why this happened...I will get this cleaned up.

>>
>>  static void read_modify_write(struct crashlog_entry *crashlog, u32 bit, bool
>> set)
>>  {
>> @@ -157,7 +215,7 @@ static bool pmt_crashlog_complete(struct
>crashlog_entry
>> *crashlog)
>>  static bool pmt_crashlog_disabled(struct crashlog_entry *crashlog)
>>  {
>>  	/* return current value of the crashlog disabled flag */
>> -	return read_check(crashlog, crashlog->info->status.disable);
>> +	return read_check(crashlog, crashlog->info->status.disabled);
>>  }
>>
>>  static void pmt_crashlog_set_disable(struct crashlog_entry *crashlog, bool
>> disable)
>> @@ -175,9 +233,119 @@ static void pmt_crashlog_set_execute(struct
>> crashlog_entry *crashlog)
>>  	read_modify_write(crashlog, crashlog->info->control.manual, SET);
>>  }
>>
>> +/* version 2 support */
>> +static bool pmt_crashlog_cleared(struct crashlog_entry *crashlog)
>> +{
>> +	/* return current value of the crashlog cleared flag */
>> +	return read_check(crashlog, crashlog->info->status.cleared);
>> +}
>> +
>> +static bool pmt_crashlog_consumed(struct crashlog_entry *crashlog)
>> +{
>> +	/* return current value of the crashlog consumedflag */
>> +	return read_check(crashlog, crashlog->info->status.consumed);
>> +}
>> +
>> +static void pmt_crashlog_set_consumed(struct crashlog_entry *crashlog)
>> +{
>> +	read_modify_write(crashlog, crashlog->info->control.consume, SET);
>> +}
>> +
>> +static bool pmt_crashlog_error(struct crashlog_entry *crashlog)
>> +{
>> +	/* return current value of the crashlog error flag */
>> +	return read_check(crashlog, crashlog->info->status.error);
>> +}
>> +
>> +static bool pmt_crashlog_rearm(struct crashlog_entry *crashlog)
>> +{
>> +	/* return current value of the crashlog reamed flag */
>> +	return read_check(crashlog, crashlog->info->status.rearmed);
>> +}
>> +
>> +static void pmt_crashlog_set_rearm(struct crashlog_entry *crashlog)
>> +{
>> +	read_modify_write(crashlog, crashlog->info->control.rearm, SET);
>> +}
>> +
>>  /*
>>   * sysfs
>>   */
>> +static ssize_t
>> +clear_show(struct device *dev, struct device_attribute *attr, char *buf)
>> +{
>> +	struct crashlog_entry *crashlog = dev_get_drvdata(dev);
>> +	bool cleared = pmt_crashlog_cleared(crashlog);
>> +
>> +	return sysfs_emit(buf, "%d\n", cleared);
>> +}
>> +
>> +static ssize_t
>> +clear_store(struct device *dev, struct device_attribute *attr,
>> +	    const char *buf, size_t count)
>> +{
>> +	struct crashlog_entry *crashlog;
>> +	bool clear;
>> +	int result;
>> +
>> +	crashlog = dev_get_drvdata(dev);
>> +
>> +	result = kstrtobool(buf, &clear);
>> +	if (result)
>> +		return result;
>> +
>> +	/* set bit only */
>> +	if (!clear)
>> +		return -EINVAL;
>> +
>> +	guard(mutex)(&crashlog->control_mutex);
>> +
>> +	pmt_crashlog_set_clear(crashlog);
>> +
>> +	return count;
>> +}
>> +static DEVICE_ATTR_RW(clear);
>> +
>> +static ssize_t
>> +consumed_show(struct device *dev, struct device_attribute *attr, char *buf)
>> +{
>> +	struct crashlog_entry *crashlog = dev_get_drvdata(dev);
>> +	bool consumed = pmt_crashlog_consumed(crashlog);
>> +
>> +	return sysfs_emit(buf, "%d\n", consumed);
>> +}
>> +
>> +static ssize_t consumed_store(struct device *dev, struct device_attribute
>> *attr,
>> +			      const char *buf, size_t count)
>> +{
>> +	struct crashlog_entry *crashlog;
>> +	bool consumed;
>> +	int result;
>> +
>> +	crashlog = dev_get_drvdata(dev);
>> +
>> +	result = kstrtobool(buf, &consumed);
>> +	if (result)
>> +		return result;
>> +
>> +	/* set bit only */
>> +	if (!consumed)
>> +		return -EINVAL;
>> +
>> +	guard(mutex)(&crashlog->control_mutex);
>> +
>> +	if (pmt_crashlog_disabled(crashlog))
>> +		return -EBUSY;
>> +
>> +	if (!pmt_crashlog_complete(crashlog))
>> +		return -EEXIST;
>> +
>> +	pmt_crashlog_set_consumed(crashlog);
>> +
>> +	return count;
>> +}
>> +static DEVICE_ATTR_RW(consumed);
>> +
>>  static ssize_t
>>  enable_show(struct device *dev, struct device_attribute *attr, char *buf)
>>  {
>> @@ -209,6 +377,50 @@ enable_store(struct device *dev, struct
>device_attribute
>> *attr,
>>  }
>>  static DEVICE_ATTR_RW(enable);
>>
>> +static ssize_t
>> +error_show(struct device *dev, struct device_attribute *attr, char *buf)
>> +{
>> +	struct crashlog_entry *crashlog = dev_get_drvdata(dev);
>> +	bool error = pmt_crashlog_error(crashlog);
>> +
>> +	return sysfs_emit(buf, "%d\n", error);
>> +}
>> +static DEVICE_ATTR_RO(error);
>> +
>> +static ssize_t
>> +rearm_show(struct device *dev, struct device_attribute *attr, char *buf)
>> +{
>> +	struct crashlog_entry *crashlog = dev_get_drvdata(dev);
>> +	int rearmed = pmt_crashlog_rearm(crashlog);
>> +
>> +	return sysfs_emit(buf, "%d\n", rearmed);
>> +}
>> +
>> +static ssize_t rearm_store(struct device *dev, struct device_attribute *attr,
>> +			   const char *buf, size_t count)
>> +{
>> +	struct crashlog_entry *crashlog;
>> +	bool rearm;
>> +	int result;
>> +
>> +	crashlog = dev_get_drvdata(dev);
>> +
>> +	result = kstrtobool(buf, &rearm);
>> +	if (result)
>> +		return result;
>> +
>> +	/* set only */
>> +	if (!rearm)
>> +		return -EINVAL;
>> +
>> +	guard(mutex)(&crashlog->control_mutex);
>> +
>> +	pmt_crashlog_set_rearm(crashlog);
>> +
>> +	return count;
>> +}
>> +static DEVICE_ATTR_RW(rearm);
>> +
>>  static ssize_t
>>  trigger_show(struct device *dev, struct device_attribute *attr, char *buf)
>>  {
>> @@ -263,24 +475,57 @@ static struct attribute *pmt_crashlog_attrs[] = {
>>  	NULL
>>  };
>>
>
>For what follows ...
>
>>
>> +static struct attribute *pmt_crashlog_ver2_attrs[] = {
>> +	&dev_attr_clear.attr,
>> +	&dev_attr_consumed.attr,
>> +	&dev_attr_enable.attr,
>> +	&dev_attr_error.attr,
>> +	&dev_attr_rearm.attr,
>> +	&dev_attr_trigger.attr,
>> +	NULL
>> +};
>> +
>>  static const struct attribute_group pmt_crashlog_group = {
>>  	.attrs	= pmt_crashlog_attrs,
>>  };
>>
>> +static const struct attribute_group pmt_crashlog_ver2_group = {
>> +	.attrs = pmt_crashlog_ver2_attrs,
>> +};
>
>... I would have the group and attributes both reflect type and version in the
>name. Update pmt_crashlog_group as well.

Makes sense.  I will update.

Thank you for your comments!

Mike

>David
>
>> +
>> +static const struct crashlog_info *select_crashlog_info(u32 type, u32
>> version)
>> +{
>> +	if (version == 0)
>> +		return &crashlog_type1_ver0;
>> +
>> +	return &crashlog_type1_ver2;
>> +}
>> +
>> +static const struct attribute_group *select_sysfs_grp(u32 type, u32 version)
>> +{
>> +	if (version == 0)
>> +		return &pmt_crashlog_group;
>> +
>> +	return &pmt_crashlog_ver2_group;
>> +}
>> +
>>  static int pmt_crashlog_header_decode(struct intel_pmt_entry *entry,
>>  				      struct device *dev)
>>  {
>>  	void __iomem *disc_table = entry->disc_table;
>>  	struct intel_pmt_header *header = &entry->header;
>>  	struct crashlog_entry *crashlog;
>> +	u32 version;
>> +	u32 type;
>>
>> -	if (!pmt_crashlog_supported(entry))
>> +	if (!pmt_crashlog_supported(entry, &type, &version))
>>  		return 1;
>>
>>  	/* initialize the crashlog struct */
>>  	crashlog = container_of(entry, struct crashlog_entry, entry);
>>  	mutex_init(&crashlog->control_mutex);
>> -	crashlog->info = &crashlog_type1_ver0;
>> +
>> +	crashlog->info = select_crashlog_info(type, version);
>>
>>  	header->access_type = GET_ACCESS(readl(disc_table));
>>  	header->guid = readl(disc_table + GUID_OFFSET);
>> @@ -289,7 +534,7 @@ static int pmt_crashlog_header_decode(struct
>> intel_pmt_entry *entry,
>>  	/* Size is measured in DWORDS, but accessor returns bytes */
>>  	header->size = GET_SIZE(readl(disc_table + SIZE_OFFSET));
>>
>> -	entry->attr_grp = &pmt_crashlog_group;
>> +	entry->attr_grp = select_sysfs_grp(type, version);
>>
>>  	return 0;
>>  }

Re: [PATCH v3 01/11] platform/x86/intel: refactor endpoint usage

[David E. Box]

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
>