[PATCH 01/10] drm/xe: Correct BMG VSEC header sizing

[PATCH 01/10] 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.

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 02/10] platform/x86/intel/pmt: white space cleanup

[Michael J. Ruhl]

Noticed two white space issues; cleaned them.

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 6a9eb3c4b313..d40c8e212733 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 03/10] 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.

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 d40c8e212733..c6d8a7a61d39 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 04/10] 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.

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 05/10] 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")
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 c6d8a7a61d39..94858bfb52f8 100644
--- a/drivers/platform/x86/intel/pmt/crashlog.c
+++ b/drivers/platform/x86/intel/pmt/crashlog.c
@@ -250,6 +250,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,
 };
 
 /*
@@ -260,8 +261,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 06/10] 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 94858bfb52f8..09cd0a1346f3 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 07/10] 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 | 177 ++++++++++++++--------
 1 file changed, 113 insertions(+), 64 deletions(-)

diff --git a/drivers/platform/x86/intel/pmt/crashlog.c b/drivers/platform/x86/intel/pmt/crashlog.c
index 09cd0a1346f3..e6eea8809a56 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 = CONTROL_OFFSET,
+	.status.clear = TYPE1_VER0_CLEAR,
+	.status.complete = TYPE1_VER0_COMPLETE,
+	.status.disable = TYPE1_VER0_DISABLE,
+
+	.control.offset = 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 {
@@ -60,24 +98,10 @@ struct pmt_crashlog_priv {
 };
 
 /*
- * I/O
+ * 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_complete(struct intel_pmt_entry *entry)
-{
-	u32 control = readl(entry->disc_table + CONTROL_OFFSET);
-
-	/* return current value of the crashlog complete flag */
-	return !!(control & 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);
-}
-
 static bool pmt_crashlog_supported(struct intel_pmt_entry *entry)
 {
 	u32 discovery_header = readl(entry->disc_table + CONTROL_OFFSET);
@@ -93,40 +117,64 @@ static bool pmt_crashlog_supported(struct intel_pmt_entry *entry)
 	return crash_type == CRASH_TYPE_OOBMSM && version == 0;
 }
 
+/*
+ * I/O
+ */
+static bool pmt_crashlog_complete(struct intel_pmt_entry *entry,
+				  const struct crashlog_status *status)
+{
+	u32 reg = readl(entry->disc_table + status->offset);
+
+	/* return current value of the crashlog complete flag */
+	return !!(reg & status->complete);
+}
+
+static bool pmt_crashlog_disabled(struct intel_pmt_entry *entry,
+				  const struct crashlog_status *status)
+{
+	u32 reg = readl(entry->disc_table + status->offset);
+
+	/* return current value of the crashlog disabled flag */
+	return !!(reg & status->disable);
+}
+
 static void pmt_crashlog_set_disable(struct intel_pmt_entry *entry,
+				     const struct crashlog_control *control,
 				     bool disable)
 {
-	u32 control = readl(entry->disc_table + CONTROL_OFFSET);
+	u32 reg = readl(entry->disc_table + control->offset);
 
 	/* clear trigger bits so we are only modifying disable flag */
-	control &= ~CRASHLOG_FLAG_TRIGGER_MASK;
+	reg &= ~control->trigger_mask;
 
 	if (disable)
-		control |= CRASHLOG_FLAG_DISABLE;
+		reg |= control->disable;
 	else
-		control &= ~CRASHLOG_FLAG_DISABLE;
+		reg &= ~control->disable;
 
-	writel(control, entry->disc_table + CONTROL_OFFSET);
+	writel(reg, entry->disc_table + control->offset);
 }
 
-static void pmt_crashlog_set_clear(struct intel_pmt_entry *entry)
+static void pmt_crashlog_set_clear(struct intel_pmt_entry *entry,
+				   const struct crashlog_control *control)
 {
-	u32 control = readl(entry->disc_table + CONTROL_OFFSET);
+	u32 reg = readl(entry->disc_table + control->offset);
 
-	control &= ~CRASHLOG_FLAG_TRIGGER_MASK;
-	control |= CRASHLOG_FLAG_TRIGGER_CLEAR;
+	reg &= ~control->trigger_mask;
+	reg |= control->clear;
 
-	writel(control, entry->disc_table + CONTROL_OFFSET);
+	writel(reg, entry->disc_table + control->offset);
 }
 
-static void pmt_crashlog_set_execute(struct intel_pmt_entry *entry)
+static void pmt_crashlog_set_execute(struct intel_pmt_entry *entry,
+				     const struct crashlog_control *control)
 {
-	u32 control = readl(entry->disc_table + CONTROL_OFFSET);
+	u32 reg = readl(entry->disc_table + control->offset);
 
-	control &= ~CRASHLOG_FLAG_TRIGGER_MASK;
-	control |= CRASHLOG_FLAG_TRIGGER_EXECUTE;
+	reg &= ~control->trigger_mask;
+	reg |= control->manual;
 
-	writel(control, entry->disc_table + CONTROL_OFFSET);
+	writel(reg, entry->disc_table + control->offset);
 }
 
 /*
@@ -135,8 +183,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);
-	int enabled = !pmt_crashlog_disabled(entry);
+	struct crashlog_entry *crashlog = dev_get_drvdata(dev);
+	int enabled = !pmt_crashlog_disabled(&crashlog->entry, &crashlog->info->status);
 
 	return sprintf(buf, "%d\n", enabled);
 }
@@ -145,19 +193,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->entry, &crashlog->info->control, !enabled);
 
 	return count;
 }
@@ -166,11 +214,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;
 	int trigger;
 
-	entry = dev_get_drvdata(dev);
-	trigger = pmt_crashlog_complete(entry);
+	crashlog = dev_get_drvdata(dev);
+	trigger = pmt_crashlog_complete(&crashlog->entry, &crashlog->info->status);
 
 	return sprintf(buf, "%d\n", trigger);
 }
@@ -179,32 +227,32 @@ 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 (!trigger) {
-		pmt_crashlog_set_clear(&entry->entry);
+		pmt_crashlog_set_clear(&crashlog->entry, &crashlog->info->control);
 		return count;
 	}
 
 	/* we cannot trigger a new crash if one is still pending */
-	if (pmt_crashlog_complete(&entry->entry))
+	if (pmt_crashlog_complete(&crashlog->entry, &crashlog->info->status))
 		return -EEXIST;
 
 	/* if device is currently disabled, return busy */
-	if (pmt_crashlog_disabled(&entry->entry))
+	if (pmt_crashlog_disabled(&crashlog->entry, &crashlog->info->status))
 		return -EBUSY;
 
-	pmt_crashlog_set_execute(&entry->entry);
+	pmt_crashlog_set_execute(&crashlog->entry, &crashlog->info->control);
 
 	return count;
 }
@@ -230,9 +278,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 08/10] 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 | 282 ++++++++++++++++++++--
 1 file changed, 263 insertions(+), 19 deletions(-)

diff --git a/drivers/platform/x86/intel/pmt/crashlog.c b/drivers/platform/x86/intel/pmt/crashlog.c
index e6eea8809a56..7291c93d71df 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 {
@@ -73,18 +106,38 @@ struct crashlog_info {
 };
 
 const struct crashlog_info crashlog_type1_ver0 = {
-	.status.offset = CONTROL_OFFSET,
-	.status.clear = TYPE1_VER0_CLEAR,
+	.status.offset = TYPE1_VER0_STATUS_OFFSET,
+	.status.cleared = TYPE1_VER0_CLEAR,
 	.status.complete = TYPE1_VER0_COMPLETE,
-	.status.disable = TYPE1_VER0_DISABLE,
+	.status.disabled = TYPE1_VER0_DISABLE,
+
 
-	.control.offset = CONTROL_OFFSET,
+	.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,
 };
 
+const struct crashlog_info crashlog_type1_ver2 = {
+	.status.offset = TYPE1_VER2_STATUS_OFFSET,
+	.status.clear_supported = TYPE1_VER2_CLEAR_SUPPORT,
+	.status.disabled = TYPE1_VER2_DISABLED,
+	.status.cleared = TYPE1_VER2_CLEARED,
+	.status.complete = TYPE1_VER2_COMPLETE,
+	.status.rearmed = TYPE1_VER2_REARMED,
+	.status.error = TYPE1_VER2_ERROR,
+	.status.in_progress = TYPE1_VER2_IN_PROGRESS,
+
+	.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,22 +152,27 @@ 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;
 }
 
 /*
@@ -135,7 +193,7 @@ static bool pmt_crashlog_disabled(struct intel_pmt_entry *entry,
 	u32 reg = readl(entry->disc_table + status->offset);
 
 	/* return current value of the crashlog disabled flag */
-	return !!(reg & status->disable);
+	return !!(reg & status->disabled);
 }
 
 static void pmt_crashlog_set_disable(struct intel_pmt_entry *entry,
@@ -177,9 +235,78 @@ static void pmt_crashlog_set_execute(struct intel_pmt_entry *entry,
 	writel(reg, entry->disc_table + control->offset);
 }
 
+/* version 2 support */
+static bool pmt_crashlog_cleared(struct intel_pmt_entry *entry,
+				 const struct crashlog_status *status)
+{
+	u32 reg = readl(entry->disc_table + status->offset);
+
+	/* return current value of the crashlog cleared flag */
+	return !!(reg & status->cleared);
+}
+
+static bool pmt_crashlog_consumed(struct intel_pmt_entry *entry,
+				  const struct crashlog_status *status)
+{
+	u32 reg = readl(entry->disc_table + status->offset);
+
+	/* return current value of the crashlog consumedflag */
+	return !!(reg & status->cleared);
+}
+
+static void pmt_crashlog_set_consumed(struct intel_pmt_entry *entry,
+				      const struct crashlog_control *control)
+{
+	u32 reg = readl(entry->disc_table + control->offset);
+
+	reg &= ~control->trigger_mask;
+	reg |= control->consume;
+
+	writel(reg, entry->disc_table + control->offset);
+}
+
+static bool pmt_crashlog_error(struct intel_pmt_entry *entry,
+			       const struct crashlog_status *status)
+{
+	u32 reg = readl(entry->disc_table + status->offset);
+
+	/* return current value of the crashlog error flag */
+	return !!(reg & status->error);
+}
+
+static bool pmt_crashlog_rearm(struct intel_pmt_entry *entry,
+			       const struct crashlog_status *status)
+{
+	u32 reg = readl(entry->disc_table + status->offset);
+
+	/* return current value of the crashlog reamed flag */
+	return !!(reg & status->rearmed);
+}
+
+static void pmt_crashlog_set_rearm(struct intel_pmt_entry *entry,
+				   const struct crashlog_control *control)
+{
+	u32 reg = readl(entry->disc_table + control->offset);
+
+	reg &= ~control->trigger_mask;
+	reg |= control->rearm;
+
+	writel(reg, entry->disc_table + control->offset);
+}
+
 /*
  * sysfs
  */
+static ssize_t
+clear_show(struct device *dev, struct device_attribute *attr, char *buf)
+{
+	struct crashlog_entry *crashlog = dev_get_drvdata(dev);
+	int cleared = pmt_crashlog_cleared(&crashlog->entry, &crashlog->info->status);
+
+	return sysfs_emit(buf, "%d\n", cleared);
+}
+static DEVICE_ATTR_RO(clear);
+
 static ssize_t
 enable_show(struct device *dev, struct device_attribute *attr, char *buf)
 {
@@ -189,6 +316,46 @@ enable_show(struct device *dev, struct device_attribute *attr, char *buf)
 	return sprintf(buf, "%d\n", enabled);
 }
 
+static ssize_t
+consumed_show(struct device *dev, struct device_attribute *attr, char *buf)
+{
+	struct crashlog_entry *crashlog = dev_get_drvdata(dev);
+	int consumed = pmt_crashlog_consumed(&crashlog->entry, &crashlog->info->status);
+
+	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->entry, &crashlog->info->status))
+		return -EBUSY;
+
+	if (!pmt_crashlog_complete(&crashlog->entry, &crashlog->info->status))
+		return -EEXIST;
+
+	pmt_crashlog_set_consumed(&crashlog->entry, &crashlog->info->control);
+
+	return count;
+}
+static DEVICE_ATTR_RW(consumed);
+
 static ssize_t
 enable_store(struct device *dev, struct device_attribute *attr,
 	     const char *buf, size_t count)
@@ -211,6 +378,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);
+	int error = pmt_crashlog_error(&crashlog->entry, &crashlog->info->status);
+
+	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->entry, &crashlog->info->status);
+
+	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->entry, &crashlog->info->control);
+
+	return count;
+}
+static DEVICE_ATTR_RW(rearm);
+
 static ssize_t
 trigger_show(struct device *dev, struct device_attribute *attr, char *buf)
 {
@@ -264,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);
@@ -290,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

[PATCH 09/10] sysfs debug

[Michael J. Ruhl]

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

diff --git a/drivers/platform/x86/intel/pmt/crashlog.c b/drivers/platform/x86/intel/pmt/crashlog.c
index 7291c93d71df..985f2c685841 100644
--- a/drivers/platform/x86/intel/pmt/crashlog.c
+++ b/drivers/platform/x86/intel/pmt/crashlog.c
@@ -469,6 +469,34 @@ trigger_store(struct device *dev, struct device_attribute *attr,
 }
 static DEVICE_ATTR_RW(trigger);
 
+#define DEBUG_REGISTER_INFO
+#ifdef DEBUG_REGISTER_INFO
+static ssize_t
+status_show(struct device *dev, struct device_attribute *attr, char *buf)
+{
+	struct crashlog_entry *crashlog = dev_get_drvdata(dev);
+	u32 status = readl(crashlog->entry.disc_table + crashlog->info->status.offset);
+	u32 control = readl(crashlog->entry.disc_table + crashlog->info->control.offset);
+	int len = 0;
+
+	len += sysfs_emit_at(buf, len, "clear_support: 0x%08x\n", status & crashlog->info->status.clear_supported);
+	len += sysfs_emit_at(buf, len, "rearmed: 0x%08x\n", status & crashlog->info->status.rearmed);
+	len += sysfs_emit_at(buf, len, "error: 0x%08x\n", status & crashlog->info->status.error);
+	len += sysfs_emit_at(buf, len, "consumed: 0x%08x\n", status & crashlog->info->status.consumed);
+	len += sysfs_emit_at(buf, len, "disable: 0x%08x\n", status & crashlog->info->status.disabled);
+	len += sysfs_emit_at(buf, len, "cleared: 0x%08x\n", status & crashlog->info->status.cleared);
+	len += sysfs_emit_at(buf, len, "in_progress: 0x%08x\n", status & crashlog->info->status.in_progress);
+	len += sysfs_emit_at(buf, len, "complete: 0x%08x\n", status & crashlog->info->status.complete);
+	len += sysfs_emit_at(buf, len, "sts_off: 0x%02x  ctl_off: 0x%02x\n", crashlog->info->status.offset,
+			     crashlog->info->control.offset);
+	len += sysfs_emit_at(buf, len, "status:  0x%08x\n", status);
+	len += sysfs_emit_at(buf, len, "control: 0x%08x\n", control);
+
+	return len;
+}
+static DEVICE_ATTR_RO(status);
+#endif
+
 static struct attribute *pmt_crashlog_attrs[] = {
 	&dev_attr_enable.attr,
 	&dev_attr_trigger.attr,
@@ -482,6 +510,9 @@ static struct attribute *pmt_crashlog_ver2_attrs[] = {
 	&dev_attr_error.attr,
 	&dev_attr_rearm.attr,
 	&dev_attr_trigger.attr,
+#ifdef DEBUG_REGISTER_INFO
+	&dev_attr_status.attr,
+#endif
 	NULL
 };
 
-- 
2.49.0

Re: [PATCH 01/10] drm/xe: Correct BMG VSEC header sizing

[Ilpo Järvinen]

On Fri, 30 May 2025, Michael J. Ruhl wrote:

> The intel_vsec_header information for the crashlog feature
> is incorrect.
>
> Update the VSEC header with correct sizing and count.

Does this warrant a Fixes tag?
 
> 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,

Eh, this change goes way beyond what you said in the changelog, was that 
intentional? If yes, please describe and justify all the changes (and 
consider if some of them belong to a separate patch as it sounds like 
there are two or more changes mixed up into this patch).

>  	NULL
>  };
>  
> 

-- 
 i.

Re: [PATCH 01/10] drm/xe: Correct BMG VSEC header sizing

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 2012 bytes --]

On Sat, 31 May 2025, Ilpo Järvinen wrote:

> On Fri, 30 May 2025, Michael J. Ruhl wrote:
> 
> > The intel_vsec_header information for the crashlog feature
> > is incorrect.
> >
> > Update the VSEC header with correct sizing and count.
> 
> Does this warrant a Fixes tag?
>  
> > 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,
> 
> Eh, this change goes way beyond what you said in the changelog, was that 
> intentional? If yes, please describe and justify all the changes (and 
> consider if some of them belong to a separate patch as it sounds like 
> there are two or more changes mixed up into this patch).

In addition, please send the next version to all relevant parties as 
indicated by the get_maintainers.pl script.

-- 
 i.

Re: [PATCH 02/10] platform/x86/intel/pmt: white space cleanup

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 1267 bytes --]

On Fri, 30 May 2025, Michael J. Ruhl wrote:

> Noticed two white space issues; cleaned them.
> 
> 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 6a9eb3c4b313..d40c8e212733 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: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>

-- 
 i.

Re: [PATCH 03/10] platform/x86/intel/pmt: use guard(mutex)

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 2301 bytes --]

On Fri, 30 May 2025, 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.
> 
> 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 d40c8e212733..c6d8a7a61d39 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);

Thanks, the control flow is very straightforward after this change.

Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>


-- 
 i.

Re: [PATCH 04/10] platform/x86/intel: refactor endpoint usage

[Ilpo Järvinen]

On Fri, 30 May 2025, 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.
> 
> 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.

All these lines look a bit short. Please reflow to 72 chars.

Also check if this isolated or is it a problem in other patches of this 
series too.


The code changes looked fine.

-- 
 i.

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

Re: [PATCH 05/10] platform/x86/intel/pmt: crashlog binary file endpoint

[Ilpo Järvinen]

On Fri, 30 May 2025, 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
> 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")

Add Cc: <stable@vger.kernel.org> and as this is going to stable so you 
should mrk the patches this one depends on with the stable Cc as well,
this is explained in Documentation/process/stable-kernel-rules.rst.

As a general rule, it would be useful to put the patches going to stable
as first and refactor and feature changes only after that (you had some 
whitespace and guard() changes before this patch).

> 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 c6d8a7a61d39..94858bfb52f8 100644
> --- a/drivers/platform/x86/intel/pmt/crashlog.c
> +++ b/drivers/platform/x86/intel/pmt/crashlog.c
> @@ -250,6 +250,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,
>  };
>  
>  /*
> @@ -260,8 +261,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,
> 

-- 
 i.

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

[Ilpo Järvinen]

On Fri, 30 May 2025, 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 | 177 ++++++++++++++--------
>  1 file changed, 113 insertions(+), 64 deletions(-)
> 
> diff --git a/drivers/platform/x86/intel/pmt/crashlog.c b/drivers/platform/x86/intel/pmt/crashlog.c
> index 09cd0a1346f3..e6eea8809a56 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 = CONTROL_OFFSET,
> +	.status.clear = TYPE1_VER0_CLEAR,
> +	.status.complete = TYPE1_VER0_COMPLETE,
> +	.status.disable = TYPE1_VER0_DISABLE,
> +
> +	.control.offset = 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 {
> @@ -60,24 +98,10 @@ struct pmt_crashlog_priv {
>  };
>  
>  /*
> - * I/O
> + * 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_complete(struct intel_pmt_entry *entry)
> -{
> -	u32 control = readl(entry->disc_table + CONTROL_OFFSET);
> -
> -	/* return current value of the crashlog complete flag */
> -	return !!(control & 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);
> -}
> -
>  static bool pmt_crashlog_supported(struct intel_pmt_entry *entry)
>  {
>  	u32 discovery_header = readl(entry->disc_table + CONTROL_OFFSET);
> @@ -93,40 +117,64 @@ static bool pmt_crashlog_supported(struct intel_pmt_entry *entry)
>  	return crash_type == CRASH_TYPE_OOBMSM && version == 0;
>  }
>  
> +/*
> + * I/O
> + */
> +static bool pmt_crashlog_complete(struct intel_pmt_entry *entry,
> +				  const struct crashlog_status *status)

I didn't fine comb this change but IMO it would be better to pass just 
crashlog_info here, the same applies to the other functions too taking 
just one of the substructs.

Overall, this change looks definitely better than the earlier version 
that was based on those if () constructs. Good work! :-)

> +{
> +	u32 reg = readl(entry->disc_table + status->offset);
> +
> +	/* return current value of the crashlog complete flag */
> +	return !!(reg & status->complete);
> +}
> +
> +static bool pmt_crashlog_disabled(struct intel_pmt_entry *entry,
> +				  const struct crashlog_status *status)
> +{
> +	u32 reg = readl(entry->disc_table + status->offset);
> +
> +	/* return current value of the crashlog disabled flag */
> +	return !!(reg & status->disable);
> +}
> +
>  static void pmt_crashlog_set_disable(struct intel_pmt_entry *entry,
> +				     const struct crashlog_control *control,
>  				     bool disable)
>  {
> -	u32 control = readl(entry->disc_table + CONTROL_OFFSET);
> +	u32 reg = readl(entry->disc_table + control->offset);
>  
>  	/* clear trigger bits so we are only modifying disable flag */
> -	control &= ~CRASHLOG_FLAG_TRIGGER_MASK;
> +	reg &= ~control->trigger_mask;
>  
>  	if (disable)
> -		control |= CRASHLOG_FLAG_DISABLE;
> +		reg |= control->disable;
>  	else
> -		control &= ~CRASHLOG_FLAG_DISABLE;
> +		reg &= ~control->disable;
>  
> -	writel(control, entry->disc_table + CONTROL_OFFSET);
> +	writel(reg, entry->disc_table + control->offset);
>  }
>  
> -static void pmt_crashlog_set_clear(struct intel_pmt_entry *entry)
> +static void pmt_crashlog_set_clear(struct intel_pmt_entry *entry,
> +				   const struct crashlog_control *control)
>  {
> -	u32 control = readl(entry->disc_table + CONTROL_OFFSET);
> +	u32 reg = readl(entry->disc_table + control->offset);
>  
> -	control &= ~CRASHLOG_FLAG_TRIGGER_MASK;
> -	control |= CRASHLOG_FLAG_TRIGGER_CLEAR;
> +	reg &= ~control->trigger_mask;
> +	reg |= control->clear;
>  
> -	writel(control, entry->disc_table + CONTROL_OFFSET);
> +	writel(reg, entry->disc_table + control->offset);
>  }
>  
> -static void pmt_crashlog_set_execute(struct intel_pmt_entry *entry)
> +static void pmt_crashlog_set_execute(struct intel_pmt_entry *entry,
> +				     const struct crashlog_control *control)
>  {
> -	u32 control = readl(entry->disc_table + CONTROL_OFFSET);
> +	u32 reg = readl(entry->disc_table + control->offset);
>  
> -	control &= ~CRASHLOG_FLAG_TRIGGER_MASK;
> -	control |= CRASHLOG_FLAG_TRIGGER_EXECUTE;
> +	reg &= ~control->trigger_mask;
> +	reg |= control->manual;
>  
> -	writel(control, entry->disc_table + CONTROL_OFFSET);
> +	writel(reg, entry->disc_table + control->offset);
>  }
>  
>  /*
> @@ -135,8 +183,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);
> -	int enabled = !pmt_crashlog_disabled(entry);
> +	struct crashlog_entry *crashlog = dev_get_drvdata(dev);
> +	int enabled = !pmt_crashlog_disabled(&crashlog->entry, &crashlog->info->status);
>  
>  	return sprintf(buf, "%d\n", enabled);
>  }
> @@ -145,19 +193,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->entry, &crashlog->info->control, !enabled);
>  
>  	return count;
>  }
> @@ -166,11 +214,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;
>  	int trigger;
>  
> -	entry = dev_get_drvdata(dev);
> -	trigger = pmt_crashlog_complete(entry);
> +	crashlog = dev_get_drvdata(dev);
> +	trigger = pmt_crashlog_complete(&crashlog->entry, &crashlog->info->status);
>  
>  	return sprintf(buf, "%d\n", trigger);
>  }
> @@ -179,32 +227,32 @@ 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 (!trigger) {
> -		pmt_crashlog_set_clear(&entry->entry);
> +		pmt_crashlog_set_clear(&crashlog->entry, &crashlog->info->control);
>  		return count;
>  	}
>  
>  	/* we cannot trigger a new crash if one is still pending */
> -	if (pmt_crashlog_complete(&entry->entry))
> +	if (pmt_crashlog_complete(&crashlog->entry, &crashlog->info->status))
>  		return -EEXIST;
>  
>  	/* if device is currently disabled, return busy */
> -	if (pmt_crashlog_disabled(&entry->entry))
> +	if (pmt_crashlog_disabled(&crashlog->entry, &crashlog->info->status))
>  		return -EBUSY;
>  
> -	pmt_crashlog_set_execute(&entry->entry);
> +	pmt_crashlog_set_execute(&crashlog->entry, &crashlog->info->control);
>  
>  	return count;
>  }
> @@ -230,9 +278,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);
> 

-- 
 i.

Re: [PATCH 08/10] platform/x86/intel/pmt: support BMG crashlog

[Ilpo Järvinen]

On Fri, 30 May 2025, 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 | 282 ++++++++++++++++++++--
>  1 file changed, 263 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/platform/x86/intel/pmt/crashlog.c b/drivers/platform/x86/intel/pmt/crashlog.c
> index e6eea8809a56..7291c93d71df 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 {
> @@ -73,18 +106,38 @@ struct crashlog_info {
>  };
>  
>  const struct crashlog_info crashlog_type1_ver0 = {
> -	.status.offset = CONTROL_OFFSET,
> -	.status.clear = TYPE1_VER0_CLEAR,
> +	.status.offset = TYPE1_VER0_STATUS_OFFSET,
> +	.status.cleared = TYPE1_VER0_CLEAR,
>  	.status.complete = TYPE1_VER0_COMPLETE,
> -	.status.disable = TYPE1_VER0_DISABLE,
> +	.status.disabled = TYPE1_VER0_DISABLE,
> +
>  
> -	.control.offset = CONTROL_OFFSET,
> +	.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,
>  };
>  
> +const struct crashlog_info crashlog_type1_ver2 = {
> +	.status.offset = TYPE1_VER2_STATUS_OFFSET,
> +	.status.clear_supported = TYPE1_VER2_CLEAR_SUPPORT,
> +	.status.disabled = TYPE1_VER2_DISABLED,
> +	.status.cleared = TYPE1_VER2_CLEARED,
> +	.status.complete = TYPE1_VER2_COMPLETE,
> +	.status.rearmed = TYPE1_VER2_REARMED,
> +	.status.error = TYPE1_VER2_ERROR,
> +	.status.in_progress = TYPE1_VER2_IN_PROGRESS,
> +
> +	.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,22 +152,27 @@ 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;
>  }
>  
>  /*
> @@ -135,7 +193,7 @@ static bool pmt_crashlog_disabled(struct intel_pmt_entry *entry,
>  	u32 reg = readl(entry->disc_table + status->offset);
>  
>  	/* return current value of the crashlog disabled flag */
> -	return !!(reg & status->disable);
> +	return !!(reg & status->disabled);
>  }
>  
>  static void pmt_crashlog_set_disable(struct intel_pmt_entry *entry,
> @@ -177,9 +235,78 @@ static void pmt_crashlog_set_execute(struct intel_pmt_entry *entry,
>  	writel(reg, entry->disc_table + control->offset);
>  }
>  
> +/* version 2 support */
> +static bool pmt_crashlog_cleared(struct intel_pmt_entry *entry,
> +				 const struct crashlog_status *status)
> +{
> +	u32 reg = readl(entry->disc_table + status->offset);
> +
> +	/* return current value of the crashlog cleared flag */
> +	return !!(reg & status->cleared);
> +}
> +
> +static bool pmt_crashlog_consumed(struct intel_pmt_entry *entry,
> +				  const struct crashlog_status *status)
> +{
> +	u32 reg = readl(entry->disc_table + status->offset);
> +
> +	/* return current value of the crashlog consumedflag */
> +	return !!(reg & status->cleared);
> +}
> +
> +static void pmt_crashlog_set_consumed(struct intel_pmt_entry *entry,
> +				      const struct crashlog_control *control)
> +{
> +	u32 reg = readl(entry->disc_table + control->offset);
> +
> +	reg &= ~control->trigger_mask;
> +	reg |= control->consume;
> +
> +	writel(reg, entry->disc_table + control->offset);
> +}
> +
> +static bool pmt_crashlog_error(struct intel_pmt_entry *entry,
> +			       const struct crashlog_status *status)
> +{
> +	u32 reg = readl(entry->disc_table + status->offset);
> +
> +	/* return current value of the crashlog error flag */
> +	return !!(reg & status->error);
> +}
> +
> +static bool pmt_crashlog_rearm(struct intel_pmt_entry *entry,
> +			       const struct crashlog_status *status)
> +{
> +	u32 reg = readl(entry->disc_table + status->offset);
> +
> +	/* return current value of the crashlog reamed flag */
> +	return !!(reg & status->rearmed);
> +}
> +
> +static void pmt_crashlog_set_rearm(struct intel_pmt_entry *entry,
> +				   const struct crashlog_control *control)
> +{
> +	u32 reg = readl(entry->disc_table + control->offset);
> +
> +	reg &= ~control->trigger_mask;
> +	reg |= control->rearm;
> +
> +	writel(reg, entry->disc_table + control->offset);
> +}
> +
>  /*
>   * sysfs
>   */
> +static ssize_t
> +clear_show(struct device *dev, struct device_attribute *attr, char *buf)
> +{
> +	struct crashlog_entry *crashlog = dev_get_drvdata(dev);
> +	int cleared = pmt_crashlog_cleared(&crashlog->entry, &crashlog->info->status);
> +
> +	return sysfs_emit(buf, "%d\n", cleared);
> +}
> +static DEVICE_ATTR_RO(clear);
> +
>  static ssize_t
>  enable_show(struct device *dev, struct device_attribute *attr, char *buf)
>  {
> @@ -189,6 +316,46 @@ enable_show(struct device *dev, struct device_attribute *attr, char *buf)
>  	return sprintf(buf, "%d\n", enabled);
>  }
>  
> +static ssize_t
> +consumed_show(struct device *dev, struct device_attribute *attr, char *buf)
> +{
> +	struct crashlog_entry *crashlog = dev_get_drvdata(dev);
> +	int consumed = pmt_crashlog_consumed(&crashlog->entry, &crashlog->info->status);

Why you don't match the type with the returned type?

> +	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->entry, &crashlog->info->status))
> +		return -EBUSY;
> +
> +	if (!pmt_crashlog_complete(&crashlog->entry, &crashlog->info->status))
> +		return -EEXIST;
> +
> +	pmt_crashlog_set_consumed(&crashlog->entry, &crashlog->info->control);
> +
> +	return count;
> +}
> +static DEVICE_ATTR_RW(consumed);
> +
>  static ssize_t
>  enable_store(struct device *dev, struct device_attribute *attr,
>  	     const char *buf, size_t count)
> @@ -211,6 +378,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);
> +	int error = pmt_crashlog_error(&crashlog->entry, &crashlog->info->status);
> +
> +	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->entry, &crashlog->info->status);
> +
> +	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->entry, &crashlog->info->control);
> +
> +	return count;
> +}
> +static DEVICE_ATTR_RW(rearm);
> +
>  static ssize_t
>  trigger_show(struct device *dev, struct device_attribute *attr, char *buf)
>  {
> @@ -264,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);
> @@ -290,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;
>  }
> 

-- 
 i.

Re: [PATCH 09/10] sysfs debug

[Ilpo Järvinen]

On Fri, 30 May 2025, Michael J. Ruhl wrote:

No changelog.

> Signed-off-by: Michael J. Ruhl <michael.j.ruhl@intel.com>
> ---
>  drivers/platform/x86/intel/pmt/crashlog.c | 31 +++++++++++++++++++++++
>  1 file changed, 31 insertions(+)
> 
> diff --git a/drivers/platform/x86/intel/pmt/crashlog.c b/drivers/platform/x86/intel/pmt/crashlog.c
> index 7291c93d71df..985f2c685841 100644
> --- a/drivers/platform/x86/intel/pmt/crashlog.c
> +++ b/drivers/platform/x86/intel/pmt/crashlog.c
> @@ -469,6 +469,34 @@ trigger_store(struct device *dev, struct device_attribute *attr,
>  }
>  static DEVICE_ATTR_RW(trigger);
>  
> +#define DEBUG_REGISTER_INFO
> +#ifdef DEBUG_REGISTER_INFO

Ah, I see, this is probably your debug/test patch you included by 
accident. :-)

> +static ssize_t
> +status_show(struct device *dev, struct device_attribute *attr, char *buf)
> +{
> +	struct crashlog_entry *crashlog = dev_get_drvdata(dev);
> +	u32 status = readl(crashlog->entry.disc_table + crashlog->info->status.offset);
> +	u32 control = readl(crashlog->entry.disc_table + crashlog->info->control.offset);
> +	int len = 0;
> +
> +	len += sysfs_emit_at(buf, len, "clear_support: 0x%08x\n", status & crashlog->info->status.clear_supported);
> +	len += sysfs_emit_at(buf, len, "rearmed: 0x%08x\n", status & crashlog->info->status.rearmed);
> +	len += sysfs_emit_at(buf, len, "error: 0x%08x\n", status & crashlog->info->status.error);
> +	len += sysfs_emit_at(buf, len, "consumed: 0x%08x\n", status & crashlog->info->status.consumed);
> +	len += sysfs_emit_at(buf, len, "disable: 0x%08x\n", status & crashlog->info->status.disabled);
> +	len += sysfs_emit_at(buf, len, "cleared: 0x%08x\n", status & crashlog->info->status.cleared);
> +	len += sysfs_emit_at(buf, len, "in_progress: 0x%08x\n", status & crashlog->info->status.in_progress);
> +	len += sysfs_emit_at(buf, len, "complete: 0x%08x\n", status & crashlog->info->status.complete);
> +	len += sysfs_emit_at(buf, len, "sts_off: 0x%02x  ctl_off: 0x%02x\n", crashlog->info->status.offset,
> +			     crashlog->info->control.offset);
> +	len += sysfs_emit_at(buf, len, "status:  0x%08x\n", status);
> +	len += sysfs_emit_at(buf, len, "control: 0x%08x\n", control);
> +
> +	return len;
> +}
> +static DEVICE_ATTR_RO(status);
> +#endif
> +
>  static struct attribute *pmt_crashlog_attrs[] = {
>  	&dev_attr_enable.attr,
>  	&dev_attr_trigger.attr,
> @@ -482,6 +510,9 @@ static struct attribute *pmt_crashlog_ver2_attrs[] = {
>  	&dev_attr_error.attr,
>  	&dev_attr_rearm.attr,
>  	&dev_attr_trigger.attr,
> +#ifdef DEBUG_REGISTER_INFO
> +	&dev_attr_status.attr,
> +#endif
>  	NULL
>  };
>  
> 

-- 
 i.

RE: [PATCH 01/10] drm/xe: Correct BMG VSEC header sizing

[Ruhl, Michael J]

>-----Original Message-----
>From: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
>Sent: Saturday, May 31, 2025 1:18 AM
>To: Ruhl, Michael J <michael.j.ruhl@intel.com>
>Cc: platform-driver-x86@vger.kernel.org; intel-xe@lists.freedesktop.org; Hans
>de Goede <hdegoede@redhat.com>; De Marchi, Lucas
><lucas.demarchi@intel.com>; Vivi, Rodrigo <rodrigo.vivi@intel.com>
>Subject: Re: [PATCH 01/10] drm/xe: Correct BMG VSEC header sizing
>
>On Fri, 30 May 2025, Michael J. Ruhl wrote:
>
>> The intel_vsec_header information for the crashlog feature
>> is incorrect.
>>
>> Update the VSEC header with correct sizing and count.
>
>Does this warrant a Fixes tag?

Since the crashlog support doesn't exist yet (for this instance), I don't think a fixes
is warranted.

>> 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,
>
>Eh, this change goes way beyond what you said in the changelog, was that
>intentional? If yes, please describe and justify all the changes (and
>consider if some of them belong to a separate patch as it sounds like
>there are two or more changes mixed up into this patch).

With num_entries (the count) being update, this is in line with the change.  Originally they were
separate entries (punit and oobsms), but once I sized things correctly (0x18 vs 0x10), only one
capability entry was needed (similar to the telemetry entry).

I will expand the commit message.

M


>>  	NULL
>>  };
>>
>>
>
>--
> i.

RE: [PATCH 03/10] platform/x86/intel/pmt: use guard(mutex)

[Ruhl, Michael J]

>-----Original Message-----
>From: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
>Sent: Saturday, May 31, 2025 1:24 AM
>To: Ruhl, Michael J <michael.j.ruhl@intel.com>
>Cc: platform-driver-x86@vger.kernel.org; intel-xe@lists.freedesktop.org; Hans
>de Goede <hdegoede@redhat.com>; De Marchi, Lucas
><lucas.demarchi@intel.com>; Vivi, Rodrigo <rodrigo.vivi@intel.com>
>Subject: Re: [PATCH 03/10] platform/x86/intel/pmt: use guard(mutex)
>
>On Fri, 30 May 2025, 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.
>>
>> 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 d40c8e212733..c6d8a7a61d39 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);
>
>Thanks, the control flow is very straightforward after this change.

Checking for the disable bit after checking for the complete doesn't really make sense to me, 
but I was concerned with "changing" the user experience...

Is this something that can be "updated"? (i.e. swapping the complete and disabled checks),

>Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>

Thanks!

M

>
>
>--
> i.

RE: [PATCH 04/10] platform/x86/intel: refactor endpoint usage

[Ruhl, Michael J]

>-----Original Message-----
>From: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
>Sent: Saturday, May 31, 2025 1:29 AM
>To: Ruhl, Michael J <michael.j.ruhl@intel.com>
>Cc: platform-driver-x86@vger.kernel.org; intel-xe@lists.freedesktop.org; Hans
>de Goede <hdegoede@redhat.com>; De Marchi, Lucas
><lucas.demarchi@intel.com>; Vivi, Rodrigo <rodrigo.vivi@intel.com>
>Subject: Re: [PATCH 04/10] platform/x86/intel: refactor endpoint usage
>
>On Fri, 30 May 2025, 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.
>>
>> 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.
>
>All these lines look a bit short. Please reflow to 72 chars.

They are (short)... I have been limiting them to 60 chars...  Not sure why I have that limit...
I will fix.

Mike

>Also check if this isolated or is it a problem in other patches of this
>series too.
>
>
>The code changes looked fine.
>
>--
> i.
>
>> 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
>>

RE: [PATCH 05/10] platform/x86/intel/pmt: crashlog binary file endpoint

[Ruhl, Michael J]

>-----Original Message-----
>From: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
>Sent: Saturday, May 31, 2025 1:37 AM
>To: Ruhl, Michael J <michael.j.ruhl@intel.com>
>Cc: platform-driver-x86@vger.kernel.org; intel-xe@lists.freedesktop.org; Hans
>de Goede <hdegoede@redhat.com>; De Marchi, Lucas
><lucas.demarchi@intel.com>; Vivi, Rodrigo <rodrigo.vivi@intel.com>
>Subject: Re: [PATCH 05/10] platform/x86/intel/pmt: crashlog binary file
>endpoint
>
>On Fri, 30 May 2025, 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
>> 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")
>
>Add Cc: <stable@vger.kernel.org> and as this is going to stable so you
>should mrk the patches this one depends on with the stable Cc as well,
>this is explained in Documentation/process/stable-kernel-rules.rst.

I will review this and make sure they are correct for my next update.

>As a general rule, it would be useful to put the patches going to stable
>as first and refactor and feature changes only after that (you had some
>whitespace and guard() changes before this patch).

That makes sense.  I will re-order the patches.

M

>> 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 c6d8a7a61d39..94858bfb52f8 100644
>> --- a/drivers/platform/x86/intel/pmt/crashlog.c
>> +++ b/drivers/platform/x86/intel/pmt/crashlog.c
>> @@ -250,6 +250,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,
>>  };
>>
>>  /*
>> @@ -260,8 +261,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,
>>
>
>--
> i.

RE: [PATCH 09/10] sysfs debug

[Ruhl, Michael J]

>-----Original Message-----
>From: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
>Sent: Saturday, May 31, 2025 1:54 AM
>To: Ruhl, Michael J <michael.j.ruhl@intel.com>
>Cc: platform-driver-x86@vger.kernel.org; intel-xe@lists.freedesktop.org; Hans
>de Goede <hdegoede@redhat.com>; De Marchi, Lucas
><lucas.demarchi@intel.com>; Vivi, Rodrigo <rodrigo.vivi@intel.com>
>Subject: Re: [PATCH 09/10] sysfs debug
>
>On Fri, 30 May 2025, Michael J. Ruhl wrote:
>
>No changelog.
>
>> Signed-off-by: Michael J. Ruhl <michael.j.ruhl@intel.com>
>> ---
>>  drivers/platform/x86/intel/pmt/crashlog.c | 31
>+++++++++++++++++++++++
>>  1 file changed, 31 insertions(+)
>>
>> diff --git a/drivers/platform/x86/intel/pmt/crashlog.c
>b/drivers/platform/x86/intel/pmt/crashlog.c
>> index 7291c93d71df..985f2c685841 100644
>> --- a/drivers/platform/x86/intel/pmt/crashlog.c
>> +++ b/drivers/platform/x86/intel/pmt/crashlog.c
>> @@ -469,6 +469,34 @@ trigger_store(struct device *dev, struct
>device_attribute *attr,
>>  }
>>  static DEVICE_ATTR_RW(trigger);
>>
>> +#define DEBUG_REGISTER_INFO
>> +#ifdef DEBUG_REGISTER_INFO
>
>Ah, I see, this is probably your debug/test patch you included by
>accident. :-)

Yup.  Sorry for the noise.  I will attempt to send only relevant patches next time.

Thanks,

M

>> +static ssize_t
>> +status_show(struct device *dev, struct device_attribute *attr, char *buf)
>> +{
>> +	struct crashlog_entry *crashlog = dev_get_drvdata(dev);
>> +	u32 status = readl(crashlog->entry.disc_table + crashlog->info-
>>status.offset);
>> +	u32 control = readl(crashlog->entry.disc_table + crashlog->info-
>>control.offset);
>> +	int len = 0;
>> +
>> +	len += sysfs_emit_at(buf, len, "clear_support: 0x%08x\n", status &
>crashlog->info->status.clear_supported);
>> +	len += sysfs_emit_at(buf, len, "rearmed: 0x%08x\n", status & crashlog-
>>info->status.rearmed);
>> +	len += sysfs_emit_at(buf, len, "error: 0x%08x\n", status & crashlog-
>>info->status.error);
>> +	len += sysfs_emit_at(buf, len, "consumed: 0x%08x\n", status &
>crashlog->info->status.consumed);
>> +	len += sysfs_emit_at(buf, len, "disable: 0x%08x\n", status & crashlog-
>>info->status.disabled);
>> +	len += sysfs_emit_at(buf, len, "cleared: 0x%08x\n", status & crashlog-
>>info->status.cleared);
>> +	len += sysfs_emit_at(buf, len, "in_progress: 0x%08x\n", status &
>crashlog->info->status.in_progress);
>> +	len += sysfs_emit_at(buf, len, "complete: 0x%08x\n", status & crashlog-
>>info->status.complete);
>> +	len += sysfs_emit_at(buf, len, "sts_off: 0x%02x  ctl_off: 0x%02x\n",
>crashlog->info->status.offset,
>> +			     crashlog->info->control.offset);
>> +	len += sysfs_emit_at(buf, len, "status:  0x%08x\n", status);
>> +	len += sysfs_emit_at(buf, len, "control: 0x%08x\n", control);
>> +
>> +	return len;
>> +}
>> +static DEVICE_ATTR_RO(status);
>> +#endif
>> +
>>  static struct attribute *pmt_crashlog_attrs[] = {
>>  	&dev_attr_enable.attr,
>>  	&dev_attr_trigger.attr,
>> @@ -482,6 +510,9 @@ static struct attribute *pmt_crashlog_ver2_attrs[] = {
>>  	&dev_attr_error.attr,
>>  	&dev_attr_rearm.attr,
>>  	&dev_attr_trigger.attr,
>> +#ifdef DEBUG_REGISTER_INFO
>> +	&dev_attr_status.attr,
>> +#endif
>>  	NULL
>>  };
>>
>>
>
>--
> i.

RE: [PATCH 03/10] platform/x86/intel/pmt: use guard(mutex)

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 3593 bytes --]

On Mon, 2 Jun 2025, Ruhl, Michael J wrote:

> >-----Original Message-----
> >From: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> >Sent: Saturday, May 31, 2025 1:24 AM
> >To: Ruhl, Michael J <michael.j.ruhl@intel.com>
> >Cc: platform-driver-x86@vger.kernel.org; intel-xe@lists.freedesktop.org; Hans
> >de Goede <hdegoede@redhat.com>; De Marchi, Lucas
> ><lucas.demarchi@intel.com>; Vivi, Rodrigo <rodrigo.vivi@intel.com>
> >Subject: Re: [PATCH 03/10] platform/x86/intel/pmt: use guard(mutex)
> >
> >On Fri, 30 May 2025, 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.
> >>
> >> 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 d40c8e212733..c6d8a7a61d39 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);
> >
> >Thanks, the control flow is very straightforward after this change.
> 
> Checking for the disable bit after checking for the complete doesn't really make sense to me, 
> but I was concerned with "changing" the user experience...
> 
> Is this something that can be "updated"? (i.e. swapping the complete and disabled checks),

TBH, I wouldn't worry over changing the precedence of returned error 
codes so just go ahead with that change (but please make it in a 
separate patch so it would be easy to revert in the very unlikely case 
that somebody depends on the old behavior).

> >Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> 
> Thanks!
> 
> M
> 
> >
> >
> >--
> > i.
> 

-- 
 i.

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

[Ruhl, Michael J]

>-----Original Message-----
>From: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
>Sent: Saturday, May 31, 2025 1:46 AM
>To: Ruhl, Michael J <michael.j.ruhl@intel.com>
>Cc: platform-driver-x86@vger.kernel.org; intel-xe@lists.freedesktop.org; Hans
>de Goede <hdegoede@redhat.com>; De Marchi, Lucas
><lucas.demarchi@intel.com>; Vivi, Rodrigo <rodrigo.vivi@intel.com>
>Subject: Re: [PATCH 07/10] platform/x86/intel/pmt: use a version struct
>
>On Fri, 30 May 2025, 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 | 177 ++++++++++++++--------
>>  1 file changed, 113 insertions(+), 64 deletions(-)
>>
>> diff --git a/drivers/platform/x86/intel/pmt/crashlog.c
>b/drivers/platform/x86/intel/pmt/crashlog.c
>> index 09cd0a1346f3..e6eea8809a56 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 = CONTROL_OFFSET,
>> +	.status.clear = TYPE1_VER0_CLEAR,
>> +	.status.complete = TYPE1_VER0_COMPLETE,
>> +	.status.disable = TYPE1_VER0_DISABLE,
>> +
>> +	.control.offset = 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 {
>> @@ -60,24 +98,10 @@ struct pmt_crashlog_priv {
>>  };
>>
>>  /*
>> - * I/O
>> + * 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_complete(struct intel_pmt_entry *entry)
>> -{
>> -	u32 control = readl(entry->disc_table + CONTROL_OFFSET);
>> -
>> -	/* return current value of the crashlog complete flag */
>> -	return !!(control & 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);
>> -}
>> -
>>  static bool pmt_crashlog_supported(struct intel_pmt_entry *entry)
>>  {
>>  	u32 discovery_header = readl(entry->disc_table + CONTROL_OFFSET);
>> @@ -93,40 +117,64 @@ static bool pmt_crashlog_supported(struct
>intel_pmt_entry *entry)
>>  	return crash_type == CRASH_TYPE_OOBMSM && version == 0;
>>  }
>>
>> +/*
>> + * I/O
>> + */
>> +static bool pmt_crashlog_complete(struct intel_pmt_entry *entry,
>> +				  const struct crashlog_status *status)
>
>I didn't fine comb this change but IMO it would be better to pass just
>crashlog_info here, the same applies to the other functions too taking
>just one of the substructs.

The issue I ran into was that the info and entry are separate members...  So I would pass crashlog_entry
and then do this:


static bool pmt_crashlog_complete(struct crashlog_entry *crashlog) 
{
	struct intel_pmt_entry *entry = &crashlog->entry;
	struct crashlog_status *status = &crashlog->info->status;
	int reg = readl(entry->disc_table + status->offset;

	return !!(reg & status->complete);
}

If that is preferred, I will update.

M

>Overall, this change looks definitely better than the earlier version
>that was based on those if () constructs. Good work! :-)

I agree.  This is a lot cleaner than the previous attempt.  Thanks for the hints! 😊

M

>> +{
>> +	u32 reg = readl(entry->disc_table + status->offset);
>> +
>> +	/* return current value of the crashlog complete flag */
>> +	return !!(reg & status->complete);
>> +}
>> +
>> +static bool pmt_crashlog_disabled(struct intel_pmt_entry *entry,
>> +				  const struct crashlog_status *status)
>> +{
>> +	u32 reg = readl(entry->disc_table + status->offset);
>> +
>> +	/* return current value of the crashlog disabled flag */
>> +	return !!(reg & status->disable);
>> +}
>> +
>>  static void pmt_crashlog_set_disable(struct intel_pmt_entry *entry,
>> +				     const struct crashlog_control *control,
>>  				     bool disable)
>>  {
>> -	u32 control = readl(entry->disc_table + CONTROL_OFFSET);
>> +	u32 reg = readl(entry->disc_table + control->offset);
>>
>>  	/* clear trigger bits so we are only modifying disable flag */
>> -	control &= ~CRASHLOG_FLAG_TRIGGER_MASK;
>> +	reg &= ~control->trigger_mask;
>>
>>  	if (disable)
>> -		control |= CRASHLOG_FLAG_DISABLE;
>> +		reg |= control->disable;
>>  	else
>> -		control &= ~CRASHLOG_FLAG_DISABLE;
>> +		reg &= ~control->disable;
>>
>> -	writel(control, entry->disc_table + CONTROL_OFFSET);
>> +	writel(reg, entry->disc_table + control->offset);
>>  }
>>
>> -static void pmt_crashlog_set_clear(struct intel_pmt_entry *entry)
>> +static void pmt_crashlog_set_clear(struct intel_pmt_entry *entry,
>> +				   const struct crashlog_control *control)
>>  {
>> -	u32 control = readl(entry->disc_table + CONTROL_OFFSET);
>> +	u32 reg = readl(entry->disc_table + control->offset);
>>
>> -	control &= ~CRASHLOG_FLAG_TRIGGER_MASK;
>> -	control |= CRASHLOG_FLAG_TRIGGER_CLEAR;
>> +	reg &= ~control->trigger_mask;
>> +	reg |= control->clear;
>>
>> -	writel(control, entry->disc_table + CONTROL_OFFSET);
>> +	writel(reg, entry->disc_table + control->offset);
>>  }
>>
>> -static void pmt_crashlog_set_execute(struct intel_pmt_entry *entry)
>> +static void pmt_crashlog_set_execute(struct intel_pmt_entry *entry,
>> +				     const struct crashlog_control *control)
>>  {
>> -	u32 control = readl(entry->disc_table + CONTROL_OFFSET);
>> +	u32 reg = readl(entry->disc_table + control->offset);
>>
>> -	control &= ~CRASHLOG_FLAG_TRIGGER_MASK;
>> -	control |= CRASHLOG_FLAG_TRIGGER_EXECUTE;
>> +	reg &= ~control->trigger_mask;
>> +	reg |= control->manual;
>>
>> -	writel(control, entry->disc_table + CONTROL_OFFSET);
>> +	writel(reg, entry->disc_table + control->offset);
>>  }
>>
>>  /*
>> @@ -135,8 +183,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);
>> -	int enabled = !pmt_crashlog_disabled(entry);
>> +	struct crashlog_entry *crashlog = dev_get_drvdata(dev);
>> +	int enabled = !pmt_crashlog_disabled(&crashlog->entry, &crashlog-
>>info->status);
>>
>>  	return sprintf(buf, "%d\n", enabled);
>>  }
>> @@ -145,19 +193,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->entry, &crashlog->info->control,
>!enabled);
>>
>>  	return count;
>>  }
>> @@ -166,11 +214,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;
>>  	int trigger;
>>
>> -	entry = dev_get_drvdata(dev);
>> -	trigger = pmt_crashlog_complete(entry);
>> +	crashlog = dev_get_drvdata(dev);
>> +	trigger = pmt_crashlog_complete(&crashlog->entry, &crashlog->info-
>>status);
>>
>>  	return sprintf(buf, "%d\n", trigger);
>>  }
>> @@ -179,32 +227,32 @@ 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 (!trigger) {
>> -		pmt_crashlog_set_clear(&entry->entry);
>> +		pmt_crashlog_set_clear(&crashlog->entry, &crashlog->info-
>>control);
>>  		return count;
>>  	}
>>
>>  	/* we cannot trigger a new crash if one is still pending */
>> -	if (pmt_crashlog_complete(&entry->entry))
>> +	if (pmt_crashlog_complete(&crashlog->entry, &crashlog->info-
>>status))
>>  		return -EEXIST;
>>
>>  	/* if device is currently disabled, return busy */
>> -	if (pmt_crashlog_disabled(&entry->entry))
>> +	if (pmt_crashlog_disabled(&crashlog->entry, &crashlog->info->status))
>>  		return -EBUSY;
>>
>> -	pmt_crashlog_set_execute(&entry->entry);
>> +	pmt_crashlog_set_execute(&crashlog->entry, &crashlog->info-
>>control);
>>
>>  	return count;
>>  }
>> @@ -230,9 +278,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);
>>
>
>--
> i.

RE: [PATCH 08/10] platform/x86/intel/pmt: support BMG crashlog

[Ruhl, Michael J]

>-----Original Message-----
>From: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
>Sent: Saturday, May 31, 2025 1:52 AM
>To: Ruhl, Michael J <michael.j.ruhl@intel.com>
>Cc: platform-driver-x86@vger.kernel.org; intel-xe@lists.freedesktop.org; Hans
>de Goede <hdegoede@redhat.com>; De Marchi, Lucas
><lucas.demarchi@intel.com>; Vivi, Rodrigo <rodrigo.vivi@intel.com>
>Subject: Re: [PATCH 08/10] platform/x86/intel/pmt: support BMG crashlog
>
>On Fri, 30 May 2025, 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 | 282 ++++++++++++++++++++-
>-
>>  1 file changed, 263 insertions(+), 19 deletions(-)
>>
>> diff --git a/drivers/platform/x86/intel/pmt/crashlog.c
>b/drivers/platform/x86/intel/pmt/crashlog.c
>> index e6eea8809a56..7291c93d71df 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 {
>> @@ -73,18 +106,38 @@ struct crashlog_info {
>>  };
>>
>>  const struct crashlog_info crashlog_type1_ver0 = {
>> -	.status.offset = CONTROL_OFFSET,
>> -	.status.clear = TYPE1_VER0_CLEAR,
>> +	.status.offset = TYPE1_VER0_STATUS_OFFSET,
>> +	.status.cleared = TYPE1_VER0_CLEAR,
>>  	.status.complete = TYPE1_VER0_COMPLETE,
>> -	.status.disable = TYPE1_VER0_DISABLE,
>> +	.status.disabled = TYPE1_VER0_DISABLE,
>> +
>>
>> -	.control.offset = CONTROL_OFFSET,
>> +	.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,
>>  };
>>
>> +const struct crashlog_info crashlog_type1_ver2 = {
>> +	.status.offset = TYPE1_VER2_STATUS_OFFSET,
>> +	.status.clear_supported = TYPE1_VER2_CLEAR_SUPPORT,
>> +	.status.disabled = TYPE1_VER2_DISABLED,
>> +	.status.cleared = TYPE1_VER2_CLEARED,
>> +	.status.complete = TYPE1_VER2_COMPLETE,
>> +	.status.rearmed = TYPE1_VER2_REARMED,
>> +	.status.error = TYPE1_VER2_ERROR,
>> +	.status.in_progress = TYPE1_VER2_IN_PROGRESS,
>> +
>> +	.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,22 +152,27 @@ 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;
>>  }
>>
>>  /*
>> @@ -135,7 +193,7 @@ static bool pmt_crashlog_disabled(struct
>intel_pmt_entry *entry,
>>  	u32 reg = readl(entry->disc_table + status->offset);
>>
>>  	/* return current value of the crashlog disabled flag */
>> -	return !!(reg & status->disable);
>> +	return !!(reg & status->disabled);
>>  }
>>
>>  static void pmt_crashlog_set_disable(struct intel_pmt_entry *entry,
>> @@ -177,9 +235,78 @@ static void pmt_crashlog_set_execute(struct
>intel_pmt_entry *entry,
>>  	writel(reg, entry->disc_table + control->offset);
>>  }
>>
>> +/* version 2 support */
>> +static bool pmt_crashlog_cleared(struct intel_pmt_entry *entry,
>> +				 const struct crashlog_status *status)
>> +{
>> +	u32 reg = readl(entry->disc_table + status->offset);
>> +
>> +	/* return current value of the crashlog cleared flag */
>> +	return !!(reg & status->cleared);
>> +}
>> +
>> +static bool pmt_crashlog_consumed(struct intel_pmt_entry *entry,
>> +				  const struct crashlog_status *status)
>> +{
>> +	u32 reg = readl(entry->disc_table + status->offset);
>> +
>> +	/* return current value of the crashlog consumedflag */
>> +	return !!(reg & status->cleared);
>> +}
>> +
>> +static void pmt_crashlog_set_consumed(struct intel_pmt_entry *entry,
>> +				      const struct crashlog_control *control)
>> +{
>> +	u32 reg = readl(entry->disc_table + control->offset);
>> +
>> +	reg &= ~control->trigger_mask;
>> +	reg |= control->consume;
>> +
>> +	writel(reg, entry->disc_table + control->offset);
>> +}
>> +
>> +static bool pmt_crashlog_error(struct intel_pmt_entry *entry,
>> +			       const struct crashlog_status *status)
>> +{
>> +	u32 reg = readl(entry->disc_table + status->offset);
>> +
>> +	/* return current value of the crashlog error flag */
>> +	return !!(reg & status->error);
>> +}
>> +
>> +static bool pmt_crashlog_rearm(struct intel_pmt_entry *entry,
>> +			       const struct crashlog_status *status)
>> +{
>> +	u32 reg = readl(entry->disc_table + status->offset);
>> +
>> +	/* return current value of the crashlog reamed flag */
>> +	return !!(reg & status->rearmed);
>> +}
>> +
>> +static void pmt_crashlog_set_rearm(struct intel_pmt_entry *entry,
>> +				   const struct crashlog_control *control)
>> +{
>> +	u32 reg = readl(entry->disc_table + control->offset);
>> +
>> +	reg &= ~control->trigger_mask;
>> +	reg |= control->rearm;
>> +
>> +	writel(reg, entry->disc_table + control->offset);
>> +}
>> +
>>  /*
>>   * sysfs
>>   */
>> +static ssize_t
>> +clear_show(struct device *dev, struct device_attribute *attr, char *buf)
>> +{
>> +	struct crashlog_entry *crashlog = dev_get_drvdata(dev);
>> +	int cleared = pmt_crashlog_cleared(&crashlog->entry, &crashlog->info-
>>status);
>> +
>> +	return sysfs_emit(buf, "%d\n", cleared);
>> +}
>> +static DEVICE_ATTR_RO(clear);
>> +
>>  static ssize_t
>>  enable_show(struct device *dev, struct device_attribute *attr, char *buf)
>>  {
>> @@ -189,6 +316,46 @@ enable_show(struct device *dev, struct
>device_attribute *attr, char *buf)
>>  	return sprintf(buf, "%d\n", enabled);
>>  }
>>
>> +static ssize_t
>> +consumed_show(struct device *dev, struct device_attribute *attr, char *buf)
>> +{
>> +	struct crashlog_entry *crashlog = dev_get_drvdata(dev);
>> +	int consumed = pmt_crashlog_consumed(&crashlog->entry, &crashlog-
>>info->status);
>
>Why you don't match the type with the returned type?

Probably a missed edit.. I will update.

Thanks!

M

>> +	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->entry, &crashlog->info->status))
>> +		return -EBUSY;
>> +
>> +	if (!pmt_crashlog_complete(&crashlog->entry, &crashlog->info-
>>status))
>> +		return -EEXIST;
>> +
>> +	pmt_crashlog_set_consumed(&crashlog->entry, &crashlog->info-
>>control);
>> +
>> +	return count;
>> +}
>> +static DEVICE_ATTR_RW(consumed);
>> +
>>  static ssize_t
>>  enable_store(struct device *dev, struct device_attribute *attr,
>>  	     const char *buf, size_t count)
>> @@ -211,6 +378,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);
>> +	int error = pmt_crashlog_error(&crashlog->entry, &crashlog->info-
>>status);
>> +
>> +	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->entry, &crashlog->info-
>>status);
>> +
>> +	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->entry, &crashlog->info->control);
>> +
>> +	return count;
>> +}
>> +static DEVICE_ATTR_RW(rearm);
>> +
>>  static ssize_t
>>  trigger_show(struct device *dev, struct device_attribute *attr, char *buf)
>>  {
>> @@ -264,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);
>> @@ -290,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;
>>  }
>>
>
>--
> i.

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

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 12859 bytes --]

On Mon, 2 Jun 2025, Ruhl, Michael J wrote:

> >-----Original Message-----
> >From: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> >Sent: Saturday, May 31, 2025 1:46 AM
> >To: Ruhl, Michael J <michael.j.ruhl@intel.com>
> >Cc: platform-driver-x86@vger.kernel.org; intel-xe@lists.freedesktop.org; Hans
> >de Goede <hdegoede@redhat.com>; De Marchi, Lucas
> ><lucas.demarchi@intel.com>; Vivi, Rodrigo <rodrigo.vivi@intel.com>
> >Subject: Re: [PATCH 07/10] platform/x86/intel/pmt: use a version struct
> >
> >On Fri, 30 May 2025, 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 | 177 ++++++++++++++--------
> >>  1 file changed, 113 insertions(+), 64 deletions(-)
> >>
> >> diff --git a/drivers/platform/x86/intel/pmt/crashlog.c
> >b/drivers/platform/x86/intel/pmt/crashlog.c
> >> index 09cd0a1346f3..e6eea8809a56 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 = CONTROL_OFFSET,
> >> +	.status.clear = TYPE1_VER0_CLEAR,
> >> +	.status.complete = TYPE1_VER0_COMPLETE,
> >> +	.status.disable = TYPE1_VER0_DISABLE,
> >> +
> >> +	.control.offset = 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 {
> >> @@ -60,24 +98,10 @@ struct pmt_crashlog_priv {
> >>  };
> >>
> >>  /*
> >> - * I/O
> >> + * 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_complete(struct intel_pmt_entry *entry)
> >> -{
> >> -	u32 control = readl(entry->disc_table + CONTROL_OFFSET);
> >> -
> >> -	/* return current value of the crashlog complete flag */
> >> -	return !!(control & 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);
> >> -}
> >> -
> >>  static bool pmt_crashlog_supported(struct intel_pmt_entry *entry)
> >>  {
> >>  	u32 discovery_header = readl(entry->disc_table + CONTROL_OFFSET);
> >> @@ -93,40 +117,64 @@ static bool pmt_crashlog_supported(struct
> >intel_pmt_entry *entry)
> >>  	return crash_type == CRASH_TYPE_OOBMSM && version == 0;
> >>  }
> >>
> >> +/*
> >> + * I/O
> >> + */
> >> +static bool pmt_crashlog_complete(struct intel_pmt_entry *entry,
> >> +				  const struct crashlog_status *status)
> >
> >I didn't fine comb this change but IMO it would be better to pass just
> >crashlog_info here, the same applies to the other functions too taking
> >just one of the substructs.
> 
> The issue I ran into was that the info and entry are separate members...  So I would pass crashlog_entry
> and then do this:
> 
> 
> static bool pmt_crashlog_complete(struct crashlog_entry *crashlog) 
> {
> 	struct intel_pmt_entry *entry = &crashlog->entry;
> 	struct crashlog_status *status = &crashlog->info->status;
> 	int reg = readl(entry->disc_table + status->offset;
> 
> 	return !!(reg & status->complete);
> }
> 
> If that is preferred, I will update.

This look okay to me.

-- 
 i.

> >Overall, this change looks definitely better than the earlier version
> >that was based on those if () constructs. Good work! :-)
> 
> I agree.  This is a lot cleaner than the previous attempt.  Thanks for the hints! 😊
> 
> M
> 
> >> +{
> >> +	u32 reg = readl(entry->disc_table + status->offset);
> >> +
> >> +	/* return current value of the crashlog complete flag */
> >> +	return !!(reg & status->complete);
> >> +}
> >> +
> >> +static bool pmt_crashlog_disabled(struct intel_pmt_entry *entry,
> >> +				  const struct crashlog_status *status)
> >> +{
> >> +	u32 reg = readl(entry->disc_table + status->offset);
> >> +
> >> +	/* return current value of the crashlog disabled flag */
> >> +	return !!(reg & status->disable);
> >> +}
> >> +
> >>  static void pmt_crashlog_set_disable(struct intel_pmt_entry *entry,
> >> +				     const struct crashlog_control *control,
> >>  				     bool disable)
> >>  {
> >> -	u32 control = readl(entry->disc_table + CONTROL_OFFSET);
> >> +	u32 reg = readl(entry->disc_table + control->offset);
> >>
> >>  	/* clear trigger bits so we are only modifying disable flag */
> >> -	control &= ~CRASHLOG_FLAG_TRIGGER_MASK;
> >> +	reg &= ~control->trigger_mask;
> >>
> >>  	if (disable)
> >> -		control |= CRASHLOG_FLAG_DISABLE;
> >> +		reg |= control->disable;
> >>  	else
> >> -		control &= ~CRASHLOG_FLAG_DISABLE;
> >> +		reg &= ~control->disable;
> >>
> >> -	writel(control, entry->disc_table + CONTROL_OFFSET);
> >> +	writel(reg, entry->disc_table + control->offset);
> >>  }
> >>
> >> -static void pmt_crashlog_set_clear(struct intel_pmt_entry *entry)
> >> +static void pmt_crashlog_set_clear(struct intel_pmt_entry *entry,
> >> +				   const struct crashlog_control *control)
> >>  {
> >> -	u32 control = readl(entry->disc_table + CONTROL_OFFSET);
> >> +	u32 reg = readl(entry->disc_table + control->offset);
> >>
> >> -	control &= ~CRASHLOG_FLAG_TRIGGER_MASK;
> >> -	control |= CRASHLOG_FLAG_TRIGGER_CLEAR;
> >> +	reg &= ~control->trigger_mask;
> >> +	reg |= control->clear;
> >>
> >> -	writel(control, entry->disc_table + CONTROL_OFFSET);
> >> +	writel(reg, entry->disc_table + control->offset);
> >>  }
> >>
> >> -static void pmt_crashlog_set_execute(struct intel_pmt_entry *entry)
> >> +static void pmt_crashlog_set_execute(struct intel_pmt_entry *entry,
> >> +				     const struct crashlog_control *control)
> >>  {
> >> -	u32 control = readl(entry->disc_table + CONTROL_OFFSET);
> >> +	u32 reg = readl(entry->disc_table + control->offset);
> >>
> >> -	control &= ~CRASHLOG_FLAG_TRIGGER_MASK;
> >> -	control |= CRASHLOG_FLAG_TRIGGER_EXECUTE;
> >> +	reg &= ~control->trigger_mask;
> >> +	reg |= control->manual;
> >>
> >> -	writel(control, entry->disc_table + CONTROL_OFFSET);
> >> +	writel(reg, entry->disc_table + control->offset);
> >>  }
> >>
> >>  /*
> >> @@ -135,8 +183,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);
> >> -	int enabled = !pmt_crashlog_disabled(entry);
> >> +	struct crashlog_entry *crashlog = dev_get_drvdata(dev);
> >> +	int enabled = !pmt_crashlog_disabled(&crashlog->entry, &crashlog-
> >>info->status);
> >>
> >>  	return sprintf(buf, "%d\n", enabled);
> >>  }
> >> @@ -145,19 +193,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->entry, &crashlog->info->control,
> >!enabled);
> >>
> >>  	return count;
> >>  }
> >> @@ -166,11 +214,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;
> >>  	int trigger;
> >>
> >> -	entry = dev_get_drvdata(dev);
> >> -	trigger = pmt_crashlog_complete(entry);
> >> +	crashlog = dev_get_drvdata(dev);
> >> +	trigger = pmt_crashlog_complete(&crashlog->entry, &crashlog->info-
> >>status);
> >>
> >>  	return sprintf(buf, "%d\n", trigger);
> >>  }
> >> @@ -179,32 +227,32 @@ 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 (!trigger) {
> >> -		pmt_crashlog_set_clear(&entry->entry);
> >> +		pmt_crashlog_set_clear(&crashlog->entry, &crashlog->info-
> >>control);
> >>  		return count;
> >>  	}
> >>
> >>  	/* we cannot trigger a new crash if one is still pending */
> >> -	if (pmt_crashlog_complete(&entry->entry))
> >> +	if (pmt_crashlog_complete(&crashlog->entry, &crashlog->info-
> >>status))
> >>  		return -EEXIST;
> >>
> >>  	/* if device is currently disabled, return busy */
> >> -	if (pmt_crashlog_disabled(&entry->entry))
> >> +	if (pmt_crashlog_disabled(&crashlog->entry, &crashlog->info->status))
> >>  		return -EBUSY;
> >>
> >> -	pmt_crashlog_set_execute(&entry->entry);
> >> +	pmt_crashlog_set_execute(&crashlog->entry, &crashlog->info-
> >>control);
> >>
> >>  	return count;
> >>  }
> >> @@ -230,9 +278,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);
> >>
> >
> >--
> > i.
> 
>