[PATCH v5 00/18] Crescent Island PMT support

[PATCH v5 00/18] Crescent Island PMT support

[Michael J. Ruhl]

The Crescent Island (CRI) GPU supports PMT via Xe driver
registration.

The CRI PMT MMIO mapping is shared for each PMT feature and it is
necessary to set an index register value to access the correct memory
space.

The PMT driver has a callback infrastructure to access data areas with
driver specific access. It is does not support the PMT control access.

CRI discovery/control spaces are part of the access window so a driver
callback is necessary to allow for the correct window to be exposed.

Add control path callback support to the PMT driver.

Add CRI PMT discovery information.

Update the Xe driver to support the CRI PMT access.

The crashlog access needs to be done with power enabled (telem is only
available when the device is powered).

Determine which access is requested, and do the appropriate power
request.

CRI FW is loaded at device power.  Support the late-binding API to
wait for FW readiness. (pending sysctl patch set implements the API).

v2:
  Address AI feedback:
  - add missing entry->cb check
  - address SRIOV VF concerns
  - use xe_pm_runtime_put instead of _get in error path

  Address review comments:
  - redo power/lock sequencing
  - support device hotplug events

v3:
  - cleanup power manangement flow
  - fixed an out of order power management locking issue
  - add a GUID caching mechanism, for GUID read
  - removed unused parameter (mmio)

v4:
 - address various review comments
 - refactor crashlog to allow for return values
 - for callback usage, decouple PMT remap of data areas
 - use IS_SRIOV_VF() rather than expose soc-remapper internals
 - separate locking fixes intos distinct steps
 - refactor define update patches to seperate BMG and CRI updates
 - separate internal GUID into separate steps

v5:
 - removed callback helpers
 - do not return error message with sysfs
 - fix missed return code check in trigger_store
 - return error value not false
 - use DEFINE_RES_MEM
 - document when base is unused
 - return error on invalid GUID request

Michael J. Ruhl (18):
  platform/x86/intel/pmt: complete pcidev to device update
  platform/x86/intel/pmt: refactor rmw with a return value
  platform/x86/intel/pmt: refactor rc with a return value
  platform/x86/intel/pmt: Add register access callbacks
  platform/x86/intel/pmt: Do not remap when using callbacks
  drm/xe/vsec: Do not register BMG PMT for VF
  drm/xe/vsec: Correct locking order
  drm/xe/vsec: Use correct pm state get
  drm/xe/vsec: Add DOC text for VSEC
  drm/xe/vsec: Support possible hotplug exit
  drm/xe/vsec: Support Crescent Island PMT
  drm/xe/vsec: Refactor BattleMage PMT defines
  drm/xe/vsec: Crescent Island PMT decode
  drm/xe/vsec: Crescent Island PMT callbacks
  drm/xe/vsec: Support late bind fw information
  drm/xe/vsec: Add PMT GUID internal access
  drm/xe/vsec: Update PMT internal access
  drm/xe/vsec: Refactor platform check

 Documentation/gpu/xe/xe_vsec.rst           |  14 +
 drivers/gpu/drm/xe/regs/xe_pmt.h           |  12 +-
 drivers/gpu/drm/xe/xe_debugfs.c            |  45 +-
 drivers/gpu/drm/xe/xe_device.c             |   4 +-
 drivers/gpu/drm/xe/xe_device_types.h       |   9 +
 drivers/gpu/drm/xe/xe_hwmon.c              |  10 +-
 drivers/gpu/drm/xe/xe_pcode.c              |  10 +-
 drivers/gpu/drm/xe/xe_vsec.c               | 539 +++++++++++++++++++--
 drivers/gpu/drm/xe/xe_vsec.h               |   3 +-
 drivers/platform/x86/intel/pmt/class.c     |  26 +-
 drivers/platform/x86/intel/pmt/class.h     |   5 +-
 drivers/platform/x86/intel/pmt/crashlog.c  | 237 ++++++---
 drivers/platform/x86/intel/pmt/discovery.c |   2 +-
 include/linux/intel_vsec.h                 |  14 +-
 14 files changed, 766 insertions(+), 164 deletions(-)
 create mode 100644 Documentation/gpu/xe/xe_vsec.rst

-- 
2.43.0

[PATCH v5 01/18] platform/x86/intel/pmt: complete pcidev to device update

[Michael J. Ruhl]

The update that moved struct pci_dev usage to struct device is
incomplete. Only telemetry endpoints are covered.

Other PMT features (crashlog) are now blocked from using the callback
mechanism.

Change struct intel_pmt_entry pci_dev member to device.

Update callback usage to use the intel_pmt_entry rather than the
telemetry endpoint.

Fixes: 353042d54d82 ("platform/x86/intel/vsec: Switch exported helpers from pci_dev to device")
Signed-off-by: Michael J. Ruhl <michael.j.ruhl@intel.com>
---
 drivers/platform/x86/intel/pmt/class.c     | 5 ++---
 drivers/platform/x86/intel/pmt/class.h     | 3 +--
 drivers/platform/x86/intel/pmt/discovery.c | 2 +-
 3 files changed, 4 insertions(+), 6 deletions(-)

diff --git a/drivers/platform/x86/intel/pmt/class.c b/drivers/platform/x86/intel/pmt/class.c
index d0ab8e33c62a..402d51df834a 100644
--- a/drivers/platform/x86/intel/pmt/class.c
+++ b/drivers/platform/x86/intel/pmt/class.c
@@ -100,7 +100,7 @@ intel_pmt_read(struct file *filp, struct kobject *kobj,
 	if (count > entry->size - off)
 		count = entry->size - off;
 
-	count = pmt_telem_read_mmio(entry->ep->dev, entry->cb, entry->header.guid, buf,
+	count = pmt_telem_read_mmio(entry->dev, entry->cb, entry->header.guid, buf,
 				    entry->base, off, count);
 
 	return count;
@@ -286,8 +286,6 @@ static int pmt_resolve_access_pci(struct intel_pmt_entry *entry,
 		return -EINVAL;
 	}
 
-	entry->pcidev = pci_dev;
-
 	return 0;
 }
 
@@ -365,6 +363,7 @@ static int intel_pmt_populate_entry(struct intel_pmt_entry *entry,
 	entry->guid = header->guid;
 	entry->size = header->size;
 	entry->cb = ivdev->priv_data;
+	entry->dev = ivdev->dev;
 
 	return 0;
 }
diff --git a/drivers/platform/x86/intel/pmt/class.h b/drivers/platform/x86/intel/pmt/class.h
index a0ece4fc3837..258cb460e61c 100644
--- a/drivers/platform/x86/intel/pmt/class.h
+++ b/drivers/platform/x86/intel/pmt/class.h
@@ -20,7 +20,6 @@
 #define GET_ADDRESS(v)		((v) & GENMASK(31, 3))
 
 struct device;
-struct pci_dev;
 extern struct class intel_pmt_class;
 
 struct telem_endpoint {
@@ -42,7 +41,7 @@ struct intel_pmt_header {
 
 struct intel_pmt_entry {
 	struct telem_endpoint	*ep;
-	struct pci_dev		*pcidev;
+	struct device		*dev;
 	struct intel_pmt_header	header;
 	u32			disc_header[PMT_DISC_DWORDS];
 	struct bin_attribute	pmt_bin_attr;
diff --git a/drivers/platform/x86/intel/pmt/discovery.c b/drivers/platform/x86/intel/pmt/discovery.c
index c482368bfaae..f4203d240f54 100644
--- a/drivers/platform/x86/intel/pmt/discovery.c
+++ b/drivers/platform/x86/intel/pmt/discovery.c
@@ -609,7 +609,7 @@ void intel_pmt_get_features(struct intel_pmt_entry *entry)
 
 	mutex_lock(&feature_list_lock);
 	list_for_each_entry(feature, &pmt_feature_list, list) {
-		if (feature->priv->parent != entry->ep->dev)
+		if (feature->priv->parent != entry->dev)
 			continue;
 
 		pmt_get_features(entry, feature);
-- 
2.43.0

Re: [PATCH v5 01/18] platform/x86/intel/pmt: complete pcidev to device update

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] Type confusion in `pmt_copy_region` when casting an ACPI device to `struct pci_dev`.
--

commit db191f282e57b915c53aca0bc92d3bca969e1af4
Author: Michael J. Ruhl <michael.j.ruhl@intel.com>

platform/x86/intel/pmt: complete pcidev to device update

The update that moved struct pci_dev usage to struct device is
incomplete. Only telemetry endpoints are covered. This patch changes
the struct intel_pmt_entry pci_dev member to device, and updates
callback usage to use the intel_pmt_entry rather than the telemetry
endpoint.

> diff --git a/drivers/platform/x86/intel/pmt/class.h b/drivers/platform/x86/intel/pmt/class.h
> index a0ece4fc3837d..258cb460e61c2 100644
> --- a/drivers/platform/x86/intel/pmt/class.h
> +++ b/drivers/platform/x86/intel/pmt/class.h
[ ... ]
> @@ -42,7 +41,7 @@ struct intel_pmt_header {
>  
>  struct intel_pmt_entry {
>  	struct telem_endpoint	*ep;

[Severity: High]
This is a pre-existing issue, but does this lead to type confusion in
pmt_copy_region() when an ACPI device has registered telemetry?

In drivers/platform/x86/intel/pmt/telemetry.c:pmt_copy_region(), there is an
unconditional cast of this device (accessed via entry->ep->dev) to a PCI
device:

	struct pci_dev *pdev = to_pci_dev(entry->ep->dev);
	struct oobmsm_plat_info *plat_info;

	plat_info = intel_vsec_get_mapping(pdev);

If the generic struct device is actually an ACPI device (as during runtime
hardware enumeration of ACPI devices that expose PMT telemetry), can
intel_vsec_get_mapping() calling pci_match_id() end up dereferencing
PCI-specific fields (like vendor and device ID) at invalid memory offsets
and cause an out-of-bounds memory read?

This issue appears to have been introduced in commit 353042d54d82 and does
not appear to be resolved by this patch.

> -	struct pci_dev		*pcidev;
> +	struct device		*dev;
>  	struct intel_pmt_header	header;
>  	u32			disc_header[PMT_DISC_DWORDS];
>  	struct bin_attribute	pmt_bin_attr;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260903201002.1064889-20-michael.j.ruhl@intel.com?part=1

[PATCH v5 02/18] platform/x86/intel/pmt: refactor rmw with a return value

[Michael J. Ruhl]

Upcoming changes will include possible failures from HW accesses.

Refactor pmt_crashlog_rwm() with a return value.

Update all necessary usage to use the return code.

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

diff --git a/drivers/platform/x86/intel/pmt/crashlog.c b/drivers/platform/x86/intel/pmt/crashlog.c
index f936daf99e4d..831cff743803 100644
--- a/drivers/platform/x86/intel/pmt/crashlog.c
+++ b/drivers/platform/x86/intel/pmt/crashlog.c
@@ -125,7 +125,7 @@ struct pmt_crashlog_priv {
  */
 
 /* Read, modify, write the control register, setting or clearing @bit based on @set */
-static void pmt_crashlog_rmw(struct crashlog_entry *crashlog, u32 bit, bool set)
+static int pmt_crashlog_rmw(struct crashlog_entry *crashlog, u32 bit, bool set)
 {
 	const struct crashlog_control *control = &crashlog->info->control;
 	struct intel_pmt_entry *entry = &crashlog->entry;
@@ -139,6 +139,8 @@ static void pmt_crashlog_rmw(struct crashlog_entry *crashlog, u32 bit, bool set)
 		reg &= ~bit;
 
 	writel(reg, entry->disc_table + control->offset);
+
+	return 0;
 }
 
 /* Read the status register and see if the specified @bit is set */
@@ -181,20 +183,20 @@ static bool pmt_crashlog_supported(struct intel_pmt_entry *entry, u32 *crash_typ
 	return false;
 }
 
-static void pmt_crashlog_set_disable(struct crashlog_entry *crashlog,
-				     bool disable)
+static int pmt_crashlog_set_disable(struct crashlog_entry *crashlog,
+				    bool disable)
 {
-	pmt_crashlog_rmw(crashlog, crashlog->info->control.disable, disable);
+	return pmt_crashlog_rmw(crashlog, crashlog->info->control.disable, disable);
 }
 
-static void pmt_crashlog_set_clear(struct crashlog_entry *crashlog)
+static int pmt_crashlog_set_clear(struct crashlog_entry *crashlog)
 {
-	pmt_crashlog_rmw(crashlog, crashlog->info->control.clear, true);
+	return pmt_crashlog_rmw(crashlog, crashlog->info->control.clear, true);
 }
 
-static void pmt_crashlog_set_execute(struct crashlog_entry *crashlog)
+static int pmt_crashlog_set_execute(struct crashlog_entry *crashlog)
 {
-	pmt_crashlog_rmw(crashlog, crashlog->info->control.manual, true);
+	return pmt_crashlog_rmw(crashlog, crashlog->info->control.manual, true);
 }
 
 static bool pmt_crashlog_cleared(struct crashlog_entry *crashlog)
@@ -207,9 +209,9 @@ static bool pmt_crashlog_consumed(struct crashlog_entry *crashlog)
 	return pmt_crashlog_rc(crashlog, crashlog->info->status.consumed);
 }
 
-static void pmt_crashlog_set_consumed(struct crashlog_entry *crashlog)
+static int pmt_crashlog_set_consumed(struct crashlog_entry *crashlog)
 {
-	pmt_crashlog_rmw(crashlog, crashlog->info->control.consume, true);
+	return pmt_crashlog_rmw(crashlog, crashlog->info->control.consume, true);
 }
 
 static bool pmt_crashlog_error(struct crashlog_entry *crashlog)
@@ -222,9 +224,9 @@ static bool pmt_crashlog_rearm(struct crashlog_entry *crashlog)
 	return pmt_crashlog_rc(crashlog, crashlog->info->status.rearmed);
 }
 
-static void pmt_crashlog_set_rearm(struct crashlog_entry *crashlog)
+static int pmt_crashlog_set_rearm(struct crashlog_entry *crashlog)
 {
-	pmt_crashlog_rmw(crashlog, crashlog->info->control.rearm, true);
+	return pmt_crashlog_rmw(crashlog, crashlog->info->control.rearm, true);
 }
 
 /*
@@ -245,13 +247,13 @@ clear_store(struct device *dev, struct device_attribute *attr,
 {
 	struct crashlog_entry *crashlog;
 	bool clear;
-	int result;
+	int ret;
 
 	crashlog = dev_get_drvdata(dev);
 
-	result = kstrtobool(buf, &clear);
-	if (result)
-		return result;
+	ret = kstrtobool(buf, &clear);
+	if (ret)
+		return ret;
 
 	/* set bit only */
 	if (!clear)
@@ -259,9 +261,9 @@ clear_store(struct device *dev, struct device_attribute *attr,
 
 	guard(mutex)(&crashlog->control_mutex);
 
-	pmt_crashlog_set_clear(crashlog);
+	ret = pmt_crashlog_set_clear(crashlog);
 
-	return count;
+	return ret ?: count;
 }
 static DEVICE_ATTR_RW(clear);
 
@@ -280,13 +282,13 @@ consumed_store(struct device *dev, struct device_attribute *attr, const char *bu
 {
 	struct crashlog_entry *crashlog;
 	bool consumed;
-	int result;
+	int ret;
 
 	crashlog = dev_get_drvdata(dev);
 
-	result = kstrtobool(buf, &consumed);
-	if (result)
-		return result;
+	ret = kstrtobool(buf, &consumed);
+	if (ret)
+		return ret;
 
 	/* set bit only */
 	if (!consumed)
@@ -300,9 +302,9 @@ consumed_store(struct device *dev, struct device_attribute *attr, const char *bu
 	if (!pmt_crashlog_complete(crashlog))
 		return -EEXIST;
 
-	pmt_crashlog_set_consumed(crashlog);
+	ret = pmt_crashlog_set_consumed(crashlog);
 
-	return count;
+	return ret ?: count;
 }
 static DEVICE_ATTR_RW(consumed);
 
@@ -321,19 +323,19 @@ enable_store(struct device *dev, struct device_attribute *attr,
 {
 	struct crashlog_entry *crashlog;
 	bool enabled;
-	int result;
+	int ret;
 
 	crashlog = dev_get_drvdata(dev);
 
-	result = kstrtobool(buf, &enabled);
-	if (result)
-		return result;
+	ret = kstrtobool(buf, &enabled);
+	if (ret)
+		return ret;
 
 	guard(mutex)(&crashlog->control_mutex);
 
-	pmt_crashlog_set_disable(crashlog, !enabled);
+	ret = pmt_crashlog_set_disable(crashlog, !enabled);
 
-	return count;
+	return ret ?: count;
 }
 static DEVICE_ATTR_RW(enable);
 
@@ -362,13 +364,13 @@ rearm_store(struct device *dev, struct device_attribute *attr, const char *buf,
 {
 	struct crashlog_entry *crashlog;
 	bool rearm;
-	int result;
+	int ret;
 
 	crashlog = dev_get_drvdata(dev);
 
-	result = kstrtobool(buf, &rearm);
-	if (result)
-		return result;
+	ret = kstrtobool(buf, &rearm);
+	if (ret)
+		return ret;
 
 	/* set only */
 	if (!rearm)
@@ -376,9 +378,9 @@ rearm_store(struct device *dev, struct device_attribute *attr, const char *buf,
 
 	guard(mutex)(&crashlog->control_mutex);
 
-	pmt_crashlog_set_rearm(crashlog);
+	ret = pmt_crashlog_set_rearm(crashlog);
 
-	return count;
+	return ret ?: count;
 }
 static DEVICE_ATTR_RW(rearm);
 
@@ -400,13 +402,13 @@ trigger_store(struct device *dev, struct device_attribute *attr,
 {
 	struct crashlog_entry *crashlog;
 	bool trigger;
-	int result;
+	int ret;
 
 	crashlog = dev_get_drvdata(dev);
 
-	result = kstrtobool(buf, &trigger);
-	if (result)
-		return result;
+	ret = kstrtobool(buf, &trigger);
+	if (ret)
+		return ret;
 
 	guard(mutex)(&crashlog->control_mutex);
 
@@ -415,17 +417,17 @@ trigger_store(struct device *dev, struct device_attribute *attr,
 		return -EBUSY;
 
 	if (!trigger) {
-		pmt_crashlog_set_clear(crashlog);
-		return count;
+		ret = pmt_crashlog_set_clear(crashlog);
+		return ret ?: count;
 	}
 
 	/* we cannot trigger a new crash if one is still pending */
 	if (pmt_crashlog_complete(crashlog))
 		return -EEXIST;
 
-	pmt_crashlog_set_execute(crashlog);
+	ret = pmt_crashlog_set_execute(crashlog);
 
-	return count;
+	return ret ?: count;
 }
 static DEVICE_ATTR_RW(trigger);
 
-- 
2.43.0

[PATCH v5 03/18] platform/x86/intel/pmt: refactor rc with a return value

[Michael J. Ruhl]

Upcoming changes will include possible failures from HW accesses.

Refactor pmt_crashlog_rw() with a return value.

Update all necessary usage to use the return code.

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

diff --git a/drivers/platform/x86/intel/pmt/crashlog.c b/drivers/platform/x86/intel/pmt/crashlog.c
index 831cff743803..808f2b695a7f 100644
--- a/drivers/platform/x86/intel/pmt/crashlog.c
+++ b/drivers/platform/x86/intel/pmt/crashlog.c
@@ -144,27 +144,29 @@ static int pmt_crashlog_rmw(struct crashlog_entry *crashlog, u32 bit, bool set)
 }
 
 /* Read the status register and see if the specified @bit is set */
-static bool pmt_crashlog_rc(struct crashlog_entry *crashlog, u32 bit)
+static int pmt_crashlog_rc(struct crashlog_entry *crashlog, u32 bit, bool *state)
 {
 	const struct crashlog_status *status = &crashlog->info->status;
 	u32 reg = readl(crashlog->entry.disc_table + status->offset);
 
-	return !!(reg & bit);
+	*state = !!(reg & bit);
+
+	return 0;
 }
 
-static bool pmt_crashlog_complete(struct crashlog_entry *crashlog)
+static int pmt_crashlog_complete(struct crashlog_entry *crashlog, bool *state)
 {
 	/* return current value of the crashlog complete flag */
-	return pmt_crashlog_rc(crashlog, crashlog->info->status.complete);
+	return pmt_crashlog_rc(crashlog, crashlog->info->status.complete, state);
 }
 
-static bool pmt_crashlog_disabled(struct crashlog_entry *crashlog)
+static int pmt_crashlog_disabled(struct crashlog_entry *crashlog, bool *state)
 {
 	/* return current value of the crashlog disabled flag */
-	return pmt_crashlog_rc(crashlog, crashlog->info->status.disabled);
+	return pmt_crashlog_rc(crashlog, crashlog->info->status.disabled, state);
 }
 
-static bool pmt_crashlog_supported(struct intel_pmt_entry *entry, u32 *crash_type, u32 *version)
+static int pmt_crashlog_supported(struct intel_pmt_entry *entry, u32 *crash_type, u32 *version)
 {
 	u32 discovery_header = readl(entry->disc_table + CONTROL_OFFSET);
 
@@ -199,14 +201,14 @@ static int pmt_crashlog_set_execute(struct crashlog_entry *crashlog)
 	return pmt_crashlog_rmw(crashlog, crashlog->info->control.manual, true);
 }
 
-static bool pmt_crashlog_cleared(struct crashlog_entry *crashlog)
+static int pmt_crashlog_cleared(struct crashlog_entry *crashlog, bool *state)
 {
-	return pmt_crashlog_rc(crashlog, crashlog->info->status.cleared);
+	return pmt_crashlog_rc(crashlog, crashlog->info->status.cleared, state);
 }
 
-static bool pmt_crashlog_consumed(struct crashlog_entry *crashlog)
+static int pmt_crashlog_consumed(struct crashlog_entry *crashlog, bool *state)
 {
-	return pmt_crashlog_rc(crashlog, crashlog->info->status.consumed);
+	return pmt_crashlog_rc(crashlog, crashlog->info->status.consumed, state);
 }
 
 static int pmt_crashlog_set_consumed(struct crashlog_entry *crashlog)
@@ -214,14 +216,14 @@ static int pmt_crashlog_set_consumed(struct crashlog_entry *crashlog)
 	return pmt_crashlog_rmw(crashlog, crashlog->info->control.consume, true);
 }
 
-static bool pmt_crashlog_error(struct crashlog_entry *crashlog)
+static int pmt_crashlog_error(struct crashlog_entry *crashlog, bool *state)
 {
-	return pmt_crashlog_rc(crashlog, crashlog->info->status.error);
+	return pmt_crashlog_rc(crashlog, crashlog->info->status.error, state);
 }
 
-static bool pmt_crashlog_rearm(struct crashlog_entry *crashlog)
+static int pmt_crashlog_rearm(struct crashlog_entry *crashlog, bool *state)
 {
-	return pmt_crashlog_rc(crashlog, crashlog->info->status.rearmed);
+	return pmt_crashlog_rc(crashlog, crashlog->info->status.rearmed, state);
 }
 
 static int pmt_crashlog_set_rearm(struct crashlog_entry *crashlog)
@@ -236,7 +238,12 @@ static ssize_t
 clear_show(struct device *dev, struct device_attribute *attr, char *buf)
 {
 	struct crashlog_entry *crashlog = dev_get_drvdata(dev);
-	bool cleared = pmt_crashlog_cleared(crashlog);
+	bool cleared;
+	int ret;
+
+	ret = pmt_crashlog_cleared(crashlog, &cleared);
+	if (ret)
+		return ret;
 
 	return sysfs_emit(buf, "%d\n", cleared);
 }
@@ -271,7 +278,12 @@ static ssize_t
 consumed_show(struct device *dev, struct device_attribute *attr, char *buf)
 {
 	struct crashlog_entry *crashlog = dev_get_drvdata(dev);
-	bool consumed = pmt_crashlog_consumed(crashlog);
+	bool consumed;
+	int ret;
+
+	ret = pmt_crashlog_consumed(crashlog, &consumed);
+	if (ret)
+		return ret;
 
 	return sysfs_emit(buf, "%d\n", consumed);
 }
@@ -281,7 +293,9 @@ consumed_store(struct device *dev, struct device_attribute *attr, const char *bu
 	       size_t count)
 {
 	struct crashlog_entry *crashlog;
+	bool complete;
 	bool consumed;
+	bool disabled;
 	int ret;
 
 	crashlog = dev_get_drvdata(dev);
@@ -296,10 +310,16 @@ consumed_store(struct device *dev, struct device_attribute *attr, const char *bu
 
 	guard(mutex)(&crashlog->control_mutex);
 
-	if (pmt_crashlog_disabled(crashlog))
+	ret = pmt_crashlog_disabled(crashlog, &disabled);
+	if (ret)
+		return ret;
+	if (disabled)
 		return -EBUSY;
 
-	if (!pmt_crashlog_complete(crashlog))
+	ret = pmt_crashlog_complete(crashlog, &complete);
+	if (ret)
+		return ret;
+	if (!complete)
 		return -EEXIST;
 
 	ret = pmt_crashlog_set_consumed(crashlog);
@@ -312,9 +332,14 @@ static ssize_t
 enable_show(struct device *dev, struct device_attribute *attr, char *buf)
 {
 	struct crashlog_entry *crashlog = dev_get_drvdata(dev);
-	bool enabled = !pmt_crashlog_disabled(crashlog);
+	bool enabled;
+	int ret;
+
+	ret = pmt_crashlog_disabled(crashlog, &enabled);
+	if (ret)
+		return ret;
 
-	return sprintf(buf, "%d\n", enabled);
+	return sysfs_emit(buf, "%d\n", enabled);
 }
 
 static ssize_t
@@ -343,7 +368,12 @@ static ssize_t
 error_show(struct device *dev, struct device_attribute *attr, char *buf)
 {
 	struct crashlog_entry *crashlog = dev_get_drvdata(dev);
-	bool error = pmt_crashlog_error(crashlog);
+	bool error;
+	int ret;
+
+	ret = pmt_crashlog_error(crashlog, &error);
+	if (ret)
+		return ret;
 
 	return sysfs_emit(buf, "%d\n", error);
 }
@@ -353,7 +383,12 @@ 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);
+	bool rearmed;
+	int ret;
+
+	ret = pmt_crashlog_rearm(crashlog, &rearmed);
+	if (ret)
+		return ret;
 
 	return sysfs_emit(buf, "%d\n", rearmed);
 }
@@ -389,11 +424,15 @@ trigger_show(struct device *dev, struct device_attribute *attr, char *buf)
 {
 	struct crashlog_entry *crashlog;
 	bool trigger;
+	int ret;
 
 	crashlog = dev_get_drvdata(dev);
-	trigger = pmt_crashlog_complete(crashlog);
 
-	return sprintf(buf, "%d\n", trigger);
+	ret = pmt_crashlog_complete(crashlog, &trigger);
+	if (ret)
+		return ret;
+
+	return sysfs_emit(buf, "%d\n", trigger);
 }
 
 static ssize_t
@@ -401,6 +440,8 @@ trigger_store(struct device *dev, struct device_attribute *attr,
 	      const char *buf, size_t count)
 {
 	struct crashlog_entry *crashlog;
+	bool complete;
+	bool disabled;
 	bool trigger;
 	int ret;
 
@@ -413,7 +454,10 @@ trigger_store(struct device *dev, struct device_attribute *attr,
 	guard(mutex)(&crashlog->control_mutex);
 
 	/* if device is currently disabled, return busy */
-	if (pmt_crashlog_disabled(crashlog))
+	ret = pmt_crashlog_disabled(crashlog, &disabled);
+	if (ret)
+		return ret;
+	if (disabled)
 		return -EBUSY;
 
 	if (!trigger) {
@@ -422,7 +466,10 @@ trigger_store(struct device *dev, struct device_attribute *attr,
 	}
 
 	/* we cannot trigger a new crash if one is still pending */
-	if (pmt_crashlog_complete(crashlog))
+	ret = pmt_crashlog_complete(crashlog, &complete);
+	if (ret)
+		return ret;
+	if (complete)
 		return -EEXIST;
 
 	ret = pmt_crashlog_set_execute(crashlog);
-- 
2.43.0

Re: [PATCH v5 03/18] platform/x86/intel/pmt: refactor rc with a return value

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] Inverted logic in `enable_show` causes the sysfs attribute to output 1 when the device is disabled and 0 when it is enabled.
--

commit 861c9b2ba4669926e83219067272a187332bb551
Author: Michael J. Ruhl <michael.j.ruhl@intel.com>

platform/x86/intel/pmt: refactor rc with a return value

Upcoming changes will include possible failures from HW accesses.
Refactor pmt_crashlog_rw() with a return value.
Update all necessary usage to use the return code.

> diff --git a/drivers/platform/x86/intel/pmt/crashlog.c b/drivers/platform/x86/intel/pmt/crashlog.c
> index 831cff743803e..808f2b695a7f7 100644
> --- a/drivers/platform/x86/intel/pmt/crashlog.c
> +++ b/drivers/platform/x86/intel/pmt/crashlog.c

[ ... ]

> @@ -312,9 +332,14 @@ static ssize_t
>  enable_show(struct device *dev, struct device_attribute *attr, char *buf)
>  {
>  	struct crashlog_entry *crashlog = dev_get_drvdata(dev);
> -	bool enabled = !pmt_crashlog_disabled(crashlog);
> +	bool enabled;
> +	int ret;
> +
> +	ret = pmt_crashlog_disabled(crashlog, &enabled);
> +	if (ret)
> +		return ret;
>  
> -	return sprintf(buf, "%d\n", enabled);
> +	return sysfs_emit(buf, "%d\n", enabled);

[Severity: Medium]
Does this code invert the logic for the enable sysfs attribute?

Looking at enable_show() in drivers/platform/x86/intel/pmt/crashlog.c,
pmt_crashlog_disabled() populates the enabled variable with the hardware's
disabled state (true if disabled, false if enabled).

Because the value is no longer negated like the original code
(!pmt_crashlog_disabled(crashlog)), reading the enable sysfs file will
incorrectly output 1 when the device is disabled and 0 when it is enabled.

>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260903201002.1064889-20-michael.j.ruhl@intel.com?part=3

RE: [PATCH v5 03/18] platform/x86/intel/pmt: refactor rc with a return value

[Ruhl, Michael J]

>-----Original Message-----
>From: sashiko-bot@kernel.org <sashiko-bot@kernel.org>
>Sent: Thursday, September 3, 2026 4:19 PM
>To: Ruhl, Michael J <michael.j.ruhl@intel.com>
>Cc: intel-xe@lists.freedesktop.org
>Subject: Re: [PATCH v5 03/18] platform/x86/intel/pmt: refactor rc with a
>return value
>
>Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to
>consider:
>- [Medium] Inverted logic in `enable_show` causes the sysfs attribute to output
>1 when the device is disabled and 0 when it is enabled.
>--
>
>commit 861c9b2ba4669926e83219067272a187332bb551
>Author: Michael J. Ruhl <michael.j.ruhl@intel.com>
>
>platform/x86/intel/pmt: refactor rc with a return value
>
>Upcoming changes will include possible failures from HW accesses.
>Refactor pmt_crashlog_rw() with a return value.
>Update all necessary usage to use the return code.
>
>> diff --git a/drivers/platform/x86/intel/pmt/crashlog.c
>b/drivers/platform/x86/intel/pmt/crashlog.c
>> index 831cff743803e..808f2b695a7f7 100644
>> --- a/drivers/platform/x86/intel/pmt/crashlog.c
>> +++ b/drivers/platform/x86/intel/pmt/crashlog.c
>
>[ ... ]
>
>> @@ -312,9 +332,14 @@ static ssize_t
>>  enable_show(struct device *dev, struct device_attribute *attr, char *buf)
>>  {
>>  	struct crashlog_entry *crashlog = dev_get_drvdata(dev);
>> -	bool enabled = !pmt_crashlog_disabled(crashlog);
>> +	bool enabled;
>> +	int ret;
>> +
>> +	ret = pmt_crashlog_disabled(crashlog, &enabled);
>> +	if (ret)
>> +		return ret;
>>
>> -	return sprintf(buf, "%d\n", enabled);
>> +	return sysfs_emit(buf, "%d\n", enabled);
>
>[Severity: Medium]
>Does this code invert the logic for the enable sysfs attribute?

Yes it does.  I will fix.

M

>Looking at enable_show() in drivers/platform/x86/intel/pmt/crashlog.c,
>pmt_crashlog_disabled() populates the enabled variable with the hardware's
>disabled state (true if disabled, false if enabled).
>
>Because the value is no longer negated like the original code
>(!pmt_crashlog_disabled(crashlog)), reading the enable sysfs file will
>incorrectly output 1 when the device is disabled and 0 when it is enabled.
>
>>  }
>
>--
>Sashiko AI review ·
>https://sashiko.dev/#/patchset/20260903201002.1064889-20-
>michael.j.ruhl@intel.com?part=3

[PATCH v5 04/18] platform/x86/intel/pmt: Add register access callbacks

[Michael J. Ruhl]

Some HW does not have direct MMIO access to PMT control and data
features.

Augment the current callback infrastructure (data access) to allow
a registered driver to customize read/write access to the control
paths for PMT usage.

Signed-off-by: Michael J. Ruhl <michael.j.ruhl@intel.com>
---
 drivers/platform/x86/intel/pmt/crashlog.c | 46 +++++++++++++++++++++--
 include/linux/intel_vsec.h                | 14 ++++++-
 2 files changed, 56 insertions(+), 4 deletions(-)

diff --git a/drivers/platform/x86/intel/pmt/crashlog.c b/drivers/platform/x86/intel/pmt/crashlog.c
index 808f2b695a7f..9311475b2bb3 100644
--- a/drivers/platform/x86/intel/pmt/crashlog.c
+++ b/drivers/platform/x86/intel/pmt/crashlog.c
@@ -11,10 +11,12 @@
 #include <linux/auxiliary_bus.h>
 #include <linux/cleanup.h>
 #include <linux/intel_vsec.h>
+#include <linux/io.h>
 #include <linux/kernel.h>
 #include <linux/module.h>
 #include <linux/mutex.h>
 #include <linux/pci.h>
+#include <linux/printk.h>
 #include <linux/slab.h>
 #include <linux/uaccess.h>
 #include <linux/overflow.h>
@@ -124,12 +126,36 @@ struct pmt_crashlog_priv {
  * I/O
  */
 
+static int pmt_crashlog_read_reg(struct intel_pmt_entry *entry, u32 *reg, u32 offset)
+{
+	int ret;
+
+	*reg = 0;
+
+	if (entry->cb && entry->cb->read_reg) {
+		ret = entry->cb->read_reg(entry->dev, entry->header.guid, reg, offset);
+		if (ret) {
+			pr_err_ratelimited("failed to read reg: %d\n", ret);
+			return ret;
+		}
+	} else {
+		*reg = readl(entry->disc_table + offset);
+	}
+
+	return 0;
+}
+
 /* Read, modify, write the control register, setting or clearing @bit based on @set */
 static int pmt_crashlog_rmw(struct crashlog_entry *crashlog, u32 bit, bool set)
 {
 	const struct crashlog_control *control = &crashlog->info->control;
 	struct intel_pmt_entry *entry = &crashlog->entry;
-	u32 reg = readl(entry->disc_table + control->offset);
+	u32 reg;
+	int ret;
+
+	ret = pmt_crashlog_read_reg(entry, &reg, control->offset);
+	if (ret)
+		return ret;
 
 	reg &= ~control->trigger_mask;
 
@@ -138,7 +164,15 @@ static int pmt_crashlog_rmw(struct crashlog_entry *crashlog, u32 bit, bool set)
 	else
 		reg &= ~bit;
 
-	writel(reg, entry->disc_table + control->offset);
+	if (entry->cb && entry->cb->write_reg) {
+		ret = entry->cb->write_reg(entry->dev, entry->header.guid, reg, control->offset);
+		if (ret) {
+			pr_err_ratelimited("failed to write reg: %d\n", ret);
+			return ret;
+		}
+	} else {
+		writel(reg, entry->disc_table + control->offset);
+	}
 
 	return 0;
 }
@@ -147,7 +181,13 @@ static int pmt_crashlog_rmw(struct crashlog_entry *crashlog, u32 bit, bool set)
 static int pmt_crashlog_rc(struct crashlog_entry *crashlog, u32 bit, bool *state)
 {
 	const struct crashlog_status *status = &crashlog->info->status;
-	u32 reg = readl(crashlog->entry.disc_table + status->offset);
+	struct intel_pmt_entry *entry = &crashlog->entry;
+	u32 reg;
+	int ret;
+
+	ret = pmt_crashlog_read_reg(entry, &reg, status->offset);
+	if (ret)
+		return ret;
 
 	*state = !!(reg & bit);
 
diff --git a/include/linux/intel_vsec.h b/include/linux/intel_vsec.h
index 843cda8f8644..917d9397a993 100644
--- a/include/linux/intel_vsec.h
+++ b/include/linux/intel_vsec.h
@@ -90,13 +90,25 @@ enum intel_vsec_quirks {
  * @read_telem: when specified, called by client driver to access PMT
  * data (instead of direct copy).
  * * dev:   device reference for the callback's use
- * * guid:  ID of data to acccss
+ * * guid:  ID of data to access
  * * data:  buffer for the data to be copied
  * * off:   offset into the requested buffer
  * * count: size of buffer
+ * @read_reg: when specified called by client driver to read PMT state
+ * * dev:      device reference for the callback's use
+ * * guid:     ID of data to access
+ * * reg_data: register data
+ * * offset:   offset of register to read
+ * @write_reg: when specified called by client driver to write PMT state
+ * * dev:      device reference for the callback's use
+ * * guid:     ID of data to access
+ * * reg_data: register data
+ * * offset:   offset of register to write
  */
 struct pmt_callbacks {
 	int (*read_telem)(struct device *dev, u32 guid, u64 *data, loff_t off, u32 count);
+	int (*read_reg)(struct device *dev, u32 guid, u32 *reg_data, u32 offset);
+	int (*write_reg)(struct device *dev, u32 guid, u32 reg_data, u32 offset);
 };
 
 struct vsec_feature_dependency {
-- 
2.43.0

[PATCH v5 05/18] platform/x86/intel/pmt: Do not remap when using callbacks

[Michael J. Ruhl]

A callback indicates that the parent driver is responsible for accessing
the data area.  Creating a PMT memory remap is redundant.

If a read_telem callback has been provided, do not create a remap.

Signed-off-by: Michael J. Ruhl <michael.j.ruhl@intel.com>
---
 drivers/platform/x86/intel/pmt/class.c | 21 +++++++++++++--------
 drivers/platform/x86/intel/pmt/class.h |  2 +-
 2 files changed, 14 insertions(+), 9 deletions(-)

diff --git a/drivers/platform/x86/intel/pmt/class.c b/drivers/platform/x86/intel/pmt/class.c
index 402d51df834a..1c7a372b4fc4 100644
--- a/drivers/platform/x86/intel/pmt/class.c
+++ b/drivers/platform/x86/intel/pmt/class.c
@@ -12,6 +12,7 @@
 #include <linux/log2.h>
 #include <linux/intel_vsec.h>
 #include <linux/io-64-nonatomic-lo-hi.h>
+#include <linux/ioport.h>
 #include <linux/minmax.h>
 #include <linux/module.h>
 #include <linux/mm.h>
@@ -373,7 +374,6 @@ static int intel_pmt_dev_register(struct intel_pmt_entry *entry,
 				  struct device *parent)
 {
 	struct intel_vsec_device *ivdev = dev_to_ivdev(parent);
-	struct resource res = {0};
 	struct device *dev;
 	int ret;
 
@@ -403,14 +403,19 @@ static int intel_pmt_dev_register(struct intel_pmt_entry *entry,
 	if (!entry->size)
 		return 0;
 
-	res.start = entry->base_addr;
-	res.end = res.start + entry->size - 1;
-	res.flags = IORESOURCE_MEM;
+	/*
+	 * The read_telem callback is responsible for this mapping, and may have
+	 * different requirements for use. If the callback is present do not
+	 * create the map.
+	 */
+	if (!(entry->cb && entry->cb->read_telem)) {
+		struct resource res = DEFINE_RES_MEM(entry->base_addr, entry->size);
 
-	entry->base = devm_ioremap_resource(dev, &res);
-	if (IS_ERR(entry->base)) {
-		ret = PTR_ERR(entry->base);
-		goto fail_ioremap;
+		entry->base = devm_ioremap_resource(dev, &res);
+		if (IS_ERR(entry->base)) {
+			ret = PTR_ERR(entry->base);
+			goto fail_ioremap;
+		}
 	}
 
 	sysfs_bin_attr_init(&entry->pmt_bin_attr);
diff --git a/drivers/platform/x86/intel/pmt/class.h b/drivers/platform/x86/intel/pmt/class.h
index 258cb460e61c..aeb04afb67cf 100644
--- a/drivers/platform/x86/intel/pmt/class.h
+++ b/drivers/platform/x86/intel/pmt/class.h
@@ -48,7 +48,7 @@ struct intel_pmt_entry {
 	const struct attribute_group *attr_grp;
 	struct kobject		*kobj;
 	void __iomem		*disc_table;
-	void __iomem		*base;
+	void __iomem		*base;  /* unused if cb->read_telem is in use */
 	struct pmt_callbacks	*cb;
 	unsigned long		base_addr;
 	size_t			size;
-- 
2.43.0

Re: [PATCH v5 05/18] platform/x86/intel/pmt: Do not remap when using callbacks

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

New issues:
- [High] NULL pointer dereference in `pmt_telem_read32()` when accessing telemetry endpoints that provide a callback.
- [Medium] PMT regions using callbacks break the `telemetry_region` API contract, causing KUnit test failures and silently dropped telemetry data.

Pre-existing issues:
- [High] Userspace mmap unconditionally exposes telemetry memory, allowing it to bypass the `read_telem` callback and hardware protections.
--

commit 12a3129bb64bc1b8ec1d2e409cf11d07b5c7a14a
Author: Michael J. Ruhl <michael.j.ruhl@intel.com>
Subject: platform/x86/intel/pmt: Do not remap when using callbacks

This commit updates the Intel PMT driver to skip creating a memory remap when
a read_telem callback is provided. It avoids redundant mappings for devices
where the parent driver manages data access.

> diff --git a/drivers/platform/x86/intel/pmt/class.c b/drivers/platform/x86/intel/pmt/class.c
> index 402d51df834a6..1c7a372b4fc42 100644
> --- a/drivers/platform/x86/intel/pmt/class.c
> +++ b/drivers/platform/x86/intel/pmt/class.c
[ ... ]
> @@ -403,14 +403,19 @@ static int intel_pmt_dev_register(struct intel_pmt_entry *entry,
>  	if (!entry->size)
>  		return 0;
>  
> -	res.start = entry->base_addr;
> -	res.end = res.start + entry->size - 1;
> -	res.flags = IORESOURCE_MEM;
> +	/*
> +	 * The read_telem callback is responsible for this mapping, and may have
> +	 * different requirements for use. If the callback is present do not
> +	 * create the map.
> +	 */
> +	if (!(entry->cb && entry->cb->read_telem)) {
> +		struct resource res = DEFINE_RES_MEM(entry->base_addr, entry->size);
>  
> -	entry->base = devm_ioremap_resource(dev, &res);
> -	if (IS_ERR(entry->base)) {
> -		ret = PTR_ERR(entry->base);
> -		goto fail_ioremap;
> +		entry->base = devm_ioremap_resource(dev, &res);
> +		if (IS_ERR(entry->base)) {
> +			ret = PTR_ERR(entry->base);
> +			goto fail_ioremap;
> +		}
>  	}

[Severity: High]
Does skipping the memory mapping here cause a NULL pointer dereference in
exported APIs?  When pmt_telem_read32() is called on a callback-enabled
endpoint, it unconditionally dereferences ep->base without checking:

drivers/platform/x86/intel/pmt/telemetry.c:pmt_telem_read32() {
    ...
    memcpy_fromio(data, ep->base + offset, NUM_BYTES_DWORD(count));
    ...
}

Since this patch leaves entry->base (and subsequently ep->base) as NULL,
will this result in a kernel panic when consumers use pmt_telem_read32()?

[Severity: Medium]
Similarly, does skipping the map break the telemetry_region API contract?
In pmt_copy_region(), the region address is populated using ep->base:

drivers/platform/x86/intel/pmt/telemetry.c:pmt_copy_region() {
    ...
    region->addr = entry->ep->base;
    region->size = entry->size;
    ...
}

This sets region->addr to NULL for callback-enabled devices. Existing
consumers expect a valid address, and the KUnit test for PMT regions
explicitly fails on this:

drivers/platform/x86/intel/pmt/discovery-kunit.c:validate_pmt_regions() {
    ...
    KUNIT_ASSERT_NOT_ERR_OR_NULL(test, (__force const void *)region->addr);
    ...
}

>  
>  	sysfs_bin_attr_init(&entry->pmt_bin_attr);
>  	entry->pmt_bin_attr.attr.name = ns->name;
>  	entry->pmt_bin_attr.attr.mode = 0440;
>  	entry->pmt_bin_attr.mmap = intel_pmt_mmap;

[Severity: High]
This is a pre-existing issue, but does assigning this mmap handler
unconditionally expose telemetry memory and bypass the callbacks?

If a userspace process calls mmap on the PMT sysfs file, the
intel_pmt_mmap handler maps the raw physical memory directly into
userspace using io_remap_pfn_range().  For endpoints that provide a
read_telem callback, they rely on software mediation to safely access
data (like waking the device, acquiring locks, etc.).

Could userspace completely circumvent these protections and potentially
cause hardware lockups or incorrect data reads by mapping the physical
memory directly?

>  	entry->pmt_bin_attr.read = intel_pmt_read;
>  	entry->pmt_bin_attr.size = entry->size;
>

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260903201002.1064889-20-michael.j.ruhl@intel.com?part=5

RE: [PATCH v5 05/18] platform/x86/intel/pmt: Do not remap when using callbacks

[Ruhl, Michael J]

>-----Original Message-----
>From: sashiko-bot@kernel.org <sashiko-bot@kernel.org>
>Sent: Thursday, September 3, 2026 4:24 PM
>To: Ruhl, Michael J <michael.j.ruhl@intel.com>
>Cc: intel-xe@lists.freedesktop.org
>Subject: Re: [PATCH v5 05/18] platform/x86/intel/pmt: Do not remap when
>using callbacks
>
>Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to
>consider:
>
>New issues:
>- [High] NULL pointer dereference in `pmt_telem_read32()` when accessing
>telemetry endpoints that provide a callback.
>- [Medium] PMT regions using callbacks break the `telemetry_region` API
>contract, causing KUnit test failures and silently dropped telemetry data.
>
>Pre-existing issues:
>- [High] Userspace mmap unconditionally exposes telemetry memory, allowing
>it to bypass the `read_telem` callback and hardware protections.
>--
>
>commit 12a3129bb64bc1b8ec1d2e409cf11d07b5c7a14a
>Author: Michael J. Ruhl <michael.j.ruhl@intel.com>
>Subject: platform/x86/intel/pmt: Do not remap when using callbacks
>
>This commit updates the Intel PMT driver to skip creating a memory remap
>when
>a read_telem callback is provided. It avoids redundant mappings for devices
>where the parent driver manages data access.
>
>> diff --git a/drivers/platform/x86/intel/pmt/class.c
>b/drivers/platform/x86/intel/pmt/class.c
>> index 402d51df834a6..1c7a372b4fc42 100644
>> --- a/drivers/platform/x86/intel/pmt/class.c
>> +++ b/drivers/platform/x86/intel/pmt/class.c
>[ ... ]
>> @@ -403,14 +403,19 @@ static int intel_pmt_dev_register(struct
>intel_pmt_entry *entry,
>>  	if (!entry->size)
>>  		return 0;
>>
>> -	res.start = entry->base_addr;
>> -	res.end = res.start + entry->size - 1;
>> -	res.flags = IORESOURCE_MEM;
>> +	/*
>> +	 * The read_telem callback is responsible for this mapping, and may
>have
>> +	 * different requirements for use. If the callback is present do not
>> +	 * create the map.
>> +	 */
>> +	if (!(entry->cb && entry->cb->read_telem)) {
>> +		struct resource res = DEFINE_RES_MEM(entry->base_addr,
>entry->size);
>>
>> -	entry->base = devm_ioremap_resource(dev, &res);
>> -	if (IS_ERR(entry->base)) {
>> -		ret = PTR_ERR(entry->base);
>> -		goto fail_ioremap;
>> +		entry->base = devm_ioremap_resource(dev, &res);
>> +		if (IS_ERR(entry->base)) {
>> +			ret = PTR_ERR(entry->base);
>> +			goto fail_ioremap;
>> +		}
>>  	}
>
>[Severity: High]
>Does skipping the memory mapping here cause a NULL pointer dereference in
>exported APIs?  When pmt_telem_read32() is called on a callback-enabled
>endpoint, it unconditionally dereferences ep->base without checking:


pmt_telem_read32() cannot be used in the callback scenario, so this will not occur.

>drivers/platform/x86/intel/pmt/telemetry.c:pmt_telem_read32() {
>    ...
>    memcpy_fromio(data, ep->base + offset, NUM_BYTES_DWORD(count));
>    ...
>}
>
>Since this patch leaves entry->base (and subsequently ep->base) as NULL,
>will this result in a kernel panic when consumers use pmt_telem_read32()?
>
>[Severity: Medium]
>Similarly, does skipping the map break the telemetry_region API contract?
>In pmt_copy_region(), the region address is populated using ep->base:

uses of the pmt_copy_region cannot be accessed with the callback scenario.

So this is not an issue.


>drivers/platform/x86/intel/pmt/telemetry.c:pmt_copy_region() {
>    ...
>    region->addr = entry->ep->base;
>    region->size = entry->size;
>    ...
>}
>
>This sets region->addr to NULL for callback-enabled devices. Existing
>consumers expect a valid address, and the KUnit test for PMT regions
>explicitly fails on this:
>
>drivers/platform/x86/intel/pmt/discovery-kunit.c:validate_pmt_regions() {
>    ...
>    KUNIT_ASSERT_NOT_ERR_OR_NULL(test, (__force const void *)region-
>>addr);
>    ...
>}
>
>>
>>  	sysfs_bin_attr_init(&entry->pmt_bin_attr);
>>  	entry->pmt_bin_attr.attr.name = ns->name;
>>  	entry->pmt_bin_attr.attr.mode = 0440;
>>  	entry->pmt_bin_attr.mmap = intel_pmt_mmap;
>
>[Severity: High]
>This is a pre-existing issue, but does assigning this mmap handler
>unconditionally expose telemetry memory and bypass the callbacks?
>
>If a userspace process calls mmap on the PMT sysfs file, the
>intel_pmt_mmap handler maps the raw physical memory directly into
>userspace using io_remap_pfn_range().  For endpoints that provide a
>read_telem callback, they rely on software mediation to safely access
>data (like waking the device, acquiring locks, etc.).
>
>Could userspace completely circumvent these protections and potentially
>cause hardware lockups or incorrect data reads by mapping the physical
>memory directly?
>
>>  	entry->pmt_bin_attr.read = intel_pmt_read;
>>  	entry->pmt_bin_attr.size = entry->size;
>>
>
>--
>Sashiko AI review ·
>https://sashiko.dev/#/patchset/20260903201002.1064889-20-
>michael.j.ruhl@intel.com?part=5

[PATCH v5 06/18] drm/xe/vsec: Do not register BMG PMT for VF

[Michael J. Ruhl]

BMG PMT is not supported for VF nodes.

If IS_SRIOV_VF() is true, do NOT register with PMT.

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

diff --git a/drivers/gpu/drm/xe/xe_vsec.c b/drivers/gpu/drm/xe/xe_vsec.c
index a9baf0bfe572..71bc359b2ad1 100644
--- a/drivers/gpu/drm/xe/xe_vsec.c
+++ b/drivers/gpu/drm/xe/xe_vsec.c
@@ -149,6 +149,9 @@ int xe_pmt_telem_read(struct device *dev, u32 guid, u64 *data, loff_t user_offse
 	u32 offset;
 	int ret;
 
+	if (IS_SRIOV_VF(xe))
+		return -EINVAL;
+
 	ret = xe_guid_decode(guid, &mem_region, &offset);
 	if (ret)
 		return ret;
@@ -157,9 +160,6 @@ int xe_pmt_telem_read(struct device *dev, u32 guid, u64 *data, loff_t user_offse
 
 	guard(mutex)(&xe->pmt.lock);
 
-	if (!xe->soc_remapper.set_telem_region)
-		return -ENODEV;
-
 	/* indicate that we are not at an appropriate power level */
 	if (!xe_pm_runtime_get_if_active(xe))
 		return -ENODATA;
@@ -210,6 +210,8 @@ void xe_vsec_init(struct xe_device *xe)
 
 	switch (platform) {
 	case XE_VSEC_BMG:
+		if (IS_SRIOV_VF(xe))
+			return;
 		info->priv_data = &xe_pmt_cb;
 		break;
 	default:
-- 
2.43.0

Re: [PATCH v5 06/18] drm/xe/vsec: Do not register BMG PMT for VF

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Critical] Removing the `!xe->soc_remapper.set_telem_region` check introduces a NULL pointer dereference during PMT telemetry reads when the device is in boot survivability mode.
--

commit e9ec971f3f576a6d5f7a1224c18c5b1a5428ab39
Author: Michael J. Ruhl <michael.j.ruhl@intel.com>

drm/xe/vsec: Do not register BMG PMT for VF

This commit disables BMG PMT registration for Virtual Function (VF) nodes by
adding SR-IOV checks. It also removes the soc_remapper pointer check in the
telemetry read path.

> diff --git a/drivers/gpu/drm/xe/xe_vsec.c b/drivers/gpu/drm/xe/xe_vsec.c
> index a9baf0bfe572c..71bc359b2ad14 100644
> --- a/drivers/gpu/drm/xe/xe_vsec.c
> +++ b/drivers/gpu/drm/xe/xe_vsec.c
[ ... ]
> @@ -157,9 +160,6 @@ int xe_pmt_telem_read(struct device *dev, u32 guid, u64 *data, loff_t user_offse
>  
>  	guard(mutex)(&xe->pmt.lock);
>  
> -	if (!xe->soc_remapper.set_telem_region)
> -		return -ENODEV;
> -

[Severity: Critical]
Does removing this check introduce a NULL pointer dereference during PMT
telemetry reads when the device is in boot survivability mode?

If the device enters boot survivability mode (e.g., due to pcode failure or
configfs), xe_pci_probe() returns early. This skips xe_device_probe() and
xe_soc_remapper_init(), leaving xe->soc_remapper.set_telem_region as NULL.

However, xe_vsec_init() still registers the PMT capabilities during boot
survivability mode, exposing them via sysfs. Because the PCI core leaves
the device's runtime PM state as RPM_ACTIVE, a userspace read of the PMT
sysfs entry will bypass the xe_pm_runtime_get_if_active() check and
unconditionally dereference the NULL set_telem_region pointer later in this
function:

drivers/gpu/drm/xe/xe_vsec.c:xe_pmt_telem_read() {
	...
	/* Unconditional dereference of set_telem_region */
	xe->soc_remapper.set_telem_region(xe, mem_region);
	...
}

>  	/* indicate that we are not at an appropriate power level */
>  	if (!xe_pm_runtime_get_if_active(xe))
>  		return -ENODATA;
[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260903201002.1064889-20-michael.j.ruhl@intel.com?part=6

[PATCH v5 07/18] drm/xe/vsec: Correct locking order

[Michael J. Ruhl]

Xe locking order is power state first, and then local locks.

Reorder locking to correct usage.
Use explicit functions because of multiple locks.

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

diff --git a/drivers/gpu/drm/xe/xe_vsec.c b/drivers/gpu/drm/xe/xe_vsec.c
index 71bc359b2ad1..dd7723ba6c1c 100644
--- a/drivers/gpu/drm/xe/xe_vsec.c
+++ b/drivers/gpu/drm/xe/xe_vsec.c
@@ -158,16 +158,19 @@ int xe_pmt_telem_read(struct device *dev, u32 guid, u64 *data, loff_t user_offse
 
 	telem_addr += offset + user_offset;
 
-	guard(mutex)(&xe->pmt.lock);
-
 	/* indicate that we are not at an appropriate power level */
 	if (!xe_pm_runtime_get_if_active(xe))
 		return -ENODATA;
 
+	mutex_lock(&xe->pmt.lock);
+
 	/* set SoC re-mapper index register based on GUID memory region */
 	xe->soc_remapper.set_telem_region(xe, mem_region);
 
 	memcpy_fromio(data, telem_addr, count);
+
+	mutex_unlock(&xe->pmt.lock);
+
 	xe_pm_runtime_put(xe);
 
 	return count;
-- 
2.43.0

[PATCH v5 08/18] drm/xe/vsec: Use correct pm state get

[Michael J. Ruhl]

Crashlog needs to be collected at all times.  The current pm check
assumes telemetry only.

Update read path to enable device for crashlog instances.

Include check of the WATCHER CAP_TYPE for completeness.
WATCHER is a possible type and needs to be accounted for, but is not
supported.

Signed-off-by: Michael J. Ruhl <michael.j.ruhl@intel.com>
Fixes: 2c402a801c19 ("platform/x86/intel/pmt: support BMG crashlog")
---
 drivers/gpu/drm/xe/xe_vsec.c | 16 +++++++++++++---
 1 file changed, 13 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/xe/xe_vsec.c b/drivers/gpu/drm/xe/xe_vsec.c
index dd7723ba6c1c..468a221378fa 100644
--- a/drivers/gpu/drm/xe/xe_vsec.c
+++ b/drivers/gpu/drm/xe/xe_vsec.c
@@ -145,6 +145,7 @@ int xe_pmt_telem_read(struct device *dev, u32 guid, u64 *data, loff_t user_offse
 {
 	struct xe_device *xe = kdev_to_xe_device(dev);
 	void __iomem *telem_addr = xe->mmio.regs + BMG_TELEMETRY_OFFSET;
+	u32 cap_type = FIELD_GET(GUID_CAP_TYPE, guid);
 	u32 mem_region;
 	u32 offset;
 	int ret;
@@ -158,9 +159,18 @@ int xe_pmt_telem_read(struct device *dev, u32 guid, u64 *data, loff_t user_offse
 
 	telem_addr += offset + user_offset;
 
-	/* indicate that we are not at an appropriate power level */
-	if (!xe_pm_runtime_get_if_active(xe))
-		return -ENODATA;
+	/* Always allow crashlog. Telemetry, only when powered */
+	switch (cap_type) {
+	case CRASHLOG:
+		xe_pm_runtime_get(xe);
+		break;
+	case TELEMETRY:
+		if (!xe_pm_runtime_get_if_active(xe))
+			return -ENODATA;
+		break;
+	case WATCHER:
+		return -EINVAL;
+	}
 
 	mutex_lock(&xe->pmt.lock);
 
-- 
2.43.0

Re: [PATCH v5 08/18] drm/xe/vsec: Use correct pm state get

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] Missing `.read_reg` and `.write_reg` callbacks in `xe_pmt_cb` leaves PMT crashlog control registers unprotected against accesses while the GPU is in D3cold.
--

commit a8bb406382664649dde97aed435f17b6cfd1536b
Author: Michael J. Ruhl <michael.j.ruhl@intel.com>

drm/xe/vsec: Use correct pm state get

This commit updates the PM runtime checks in the telemetry read path to ensure
the device is properly awakened when reading crashlog data, as crashlog needs
to be collected at all times. It also adds a check for the WATCHER capability
type for completeness.

> diff --git a/drivers/gpu/drm/xe/xe_vsec.c b/drivers/gpu/drm/xe/xe_vsec.c
> index dd7723ba6c1c2..468a221378faf 100644
> --- a/drivers/gpu/drm/xe/xe_vsec.c
> +++ b/drivers/gpu/drm/xe/xe_vsec.c
> @@ -158,9 +159,18 @@ int xe_pmt_telem_read(struct device *dev, u32 guid, u64 *data, loff_t user_offse
>  
>  	telem_addr += offset + user_offset;
>  
> -	/* indicate that we are not at an appropriate power level */
> -	if (!xe_pm_runtime_get_if_active(xe))
> -		return -ENODATA;
> +	/* Always allow crashlog. Telemetry, only when powered */
> +	switch (cap_type) {
> +	case CRASHLOG:
> +		xe_pm_runtime_get(xe);
> +		break;
> +	case TELEMETRY:
> +		if (!xe_pm_runtime_get_if_active(xe))
> +			return -ENODATA;
> +		break;
> +	case WATCHER:
> +		return -EINVAL;
> +	}

[Severity: High]
This is a pre-existing issue, but since this patch is addressing PM runtime
protection for the crashlog data path, it seems the control registers are still
left entirely unprotected.

While xe_pmt_telem_read() now handles PM state correctly for data reads,
xe_pmt_cb lacks .read_reg and .write_reg callbacks:

drivers/gpu/drm/xe/xe_vsec.c:
static struct pmt_callbacks xe_pmt_cb = {
	.read_telem = xe_pmt_telem_read,
};

When userspace accesses PMT crashlog control attributes (like enable, clear, or
trigger) via sysfs while the GPU is in D3cold, pmt_crashlog_read_reg() and
pmt_crashlog_rmw() fall back to direct MMIO access:

drivers/platform/x86/intel/pmt/crashlog.c:pmt_crashlog_read_reg() {
	...
	if (entry->pdata->cb && entry->pdata->cb->read_reg)
		return entry->pdata->cb->read_reg(entry->pdata->cb_data, offset, reg);
	*reg = readl(entry->disc_table + offset);
	...
}

Because the callbacks are NULL, the fallback access hits powered-down PCIe MMIO
space. Does this cause PCIe unsupported requests (UR) and lead to Advanced Error
Reporting (AER) panics, system freezes, or returning invalid data to userspace?

Could .read_reg and .write_reg callbacks be added to xe_pmt_cb to protect the
control registers during D3cold?

>  
>  	mutex_lock(&xe->pmt.lock);
>

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260903201002.1064889-20-michael.j.ruhl@intel.com?part=8

RE: [PATCH v5 08/18] drm/xe/vsec: Use correct pm state get

[Ruhl, Michael J]

>-----Original Message-----
>From: sashiko-bot@kernel.org <sashiko-bot@kernel.org>
>Sent: Thursday, September 3, 2026 4:27 PM
>To: Ruhl, Michael J <michael.j.ruhl@intel.com>
>Cc: intel-xe@lists.freedesktop.org
>Subject: Re: [PATCH v5 08/18] drm/xe/vsec: Use correct pm state get
>
>Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to
>consider:
>
>Pre-existing issues:
>- [High] Missing `.read_reg` and `.write_reg` callbacks in `xe_pmt_cb` leaves
>PMT crashlog control registers unprotected against accesses while the GPU is in
>D3cold.
>--
>
>commit a8bb406382664649dde97aed435f17b6cfd1536b
>Author: Michael J. Ruhl <michael.j.ruhl@intel.com>
>
>drm/xe/vsec: Use correct pm state get
>
>This commit updates the PM runtime checks in the telemetry read path to
>ensure
>the device is properly awakened when reading crashlog data, as crashlog needs
>to be collected at all times. It also adds a check for the WATCHER capability
>type for completeness.
>
>> diff --git a/drivers/gpu/drm/xe/xe_vsec.c b/drivers/gpu/drm/xe/xe_vsec.c
>> index dd7723ba6c1c2..468a221378faf 100644
>> --- a/drivers/gpu/drm/xe/xe_vsec.c
>> +++ b/drivers/gpu/drm/xe/xe_vsec.c
>> @@ -158,9 +159,18 @@ int xe_pmt_telem_read(struct device *dev, u32
>guid, u64 *data, loff_t user_offse
>>
>>  	telem_addr += offset + user_offset;
>>
>> -	/* indicate that we are not at an appropriate power level */
>> -	if (!xe_pm_runtime_get_if_active(xe))
>> -		return -ENODATA;
>> +	/* Always allow crashlog. Telemetry, only when powered */
>> +	switch (cap_type) {
>> +	case CRASHLOG:
>> +		xe_pm_runtime_get(xe);
>> +		break;
>> +	case TELEMETRY:
>> +		if (!xe_pm_runtime_get_if_active(xe))
>> +			return -ENODATA;
>> +		break;
>> +	case WATCHER:
>> +		return -EINVAL;
>> +	}
>
>[Severity: High]
>This is a pre-existing issue, but since this patch is addressing PM runtime
>protection for the crashlog data path, it seems the control registers are still
>left entirely unprotected.

the control register API is not used for BMG.  So these callbacks to NOT need to be
populated.


>While xe_pmt_telem_read() now handles PM state correctly for data reads,
>xe_pmt_cb lacks .read_reg and .write_reg callbacks:
>
>drivers/gpu/drm/xe/xe_vsec.c:
>static struct pmt_callbacks xe_pmt_cb = {
>	.read_telem = xe_pmt_telem_read,
>};
>
>When userspace accesses PMT crashlog control attributes (like enable, clear, or
>trigger) via sysfs while the GPU is in D3cold, pmt_crashlog_read_reg() and
>pmt_crashlog_rmw() fall back to direct MMIO access:
>
>drivers/platform/x86/intel/pmt/crashlog.c:pmt_crashlog_read_reg() {
>	...
>	if (entry->pdata->cb && entry->pdata->cb->read_reg)
>		return entry->pdata->cb->read_reg(entry->pdata->cb_data,
>offset, reg);
>	*reg = readl(entry->disc_table + offset);
>	...
>}
>
>Because the callbacks are NULL, the fallback access hits powered-down PCIe
>MMIO
>space. Does this cause PCIe unsupported requests (UR) and lead to Advanced
>Error
>Reporting (AER) panics, system freezes, or returning invalid data to userspace?
>
>Could .read_reg and .write_reg callbacks be added to xe_pmt_cb to protect the
>control registers during D3cold?
>
>>
>>  	mutex_lock(&xe->pmt.lock);
>>
>
>--
>Sashiko AI review ·
>https://sashiko.dev/#/patchset/20260903201002.1064889-20-
>michael.j.ruhl@intel.com?part=8

[PATCH v5 09/18] drm/xe/vsec: Add DOC text for VSEC

[Michael J. Ruhl]

Add the necessary files and text to use the DOC system.

Signed-off-by: Michael J. Ruhl <michael.j.ruhl@intel.com>
---
 Documentation/gpu/xe/xe_vsec.rst | 14 ++++++++++++++
 drivers/gpu/drm/xe/xe_vsec.c     | 22 ++++++++++++++++++++++
 2 files changed, 36 insertions(+)
 create mode 100644 Documentation/gpu/xe/xe_vsec.rst

diff --git a/Documentation/gpu/xe/xe_vsec.rst b/Documentation/gpu/xe/xe_vsec.rst
new file mode 100644
index 000000000000..1a87548489ea
--- /dev/null
+++ b/Documentation/gpu/xe/xe_vsec.rst
@@ -0,0 +1,14 @@
+.. SPDX-License-Identifier: (GPL-2.0+ OR MIT)
+
+=====
+VSEC
+=====
+
+.. kernel-doc:: drivers/gpu/drm/xe/xe_vsec.c
+   :doc: VSEC
+
+Internal API
+============
+
+.. kernel-doc:: drivers/gpu/drm/xe/xe_vsec.c
+   :internal:
diff --git a/drivers/gpu/drm/xe/xe_vsec.c b/drivers/gpu/drm/xe/xe_vsec.c
index 468a221378fa..5223bfe50ede 100644
--- a/drivers/gpu/drm/xe/xe_vsec.c
+++ b/drivers/gpu/drm/xe/xe_vsec.c
@@ -19,6 +19,17 @@
 
 #include "regs/xe_pmt.h"
 
+/**
+ * DOC: VSEC
+ *
+ * Some GPUs support the PMT feature.  For those that do, this API provides
+ * probe, discovery and usage (control and access) for GPUs that have PMT.
+ *
+ * PMT is usually accessed through a PCI function.  However, that is not always
+ * available, so the PMT intel_vsec_register() function can be used to setup up
+ * PMT usage.
+ */
+
 /* PMT GUID value for BMG devices.  NOTE: this is NOT a PCI id */
 #define BMG_DEVICE_ID 0xE2F8
 
@@ -140,6 +151,17 @@ static int xe_guid_decode(u32 guid, int *index, u32 *offset)
 	return 0;
 }
 
+/**
+ * xe_pmt_telem_read - Given a device and a PMT GUID, read data into a buffer
+ * @dev: valid Xe device
+ * @guid: PMT GUID (from the PMT discovery struct)
+ * @data: buffer to place data in
+ * @user_offset: a user offset relative to the beginning of the data to read
+ * @count: bytes to read
+ *
+ * Returns -EINVAL for VF and PMT WATCHER requests
+ *         count   number of bytes read on success
+ */
 int xe_pmt_telem_read(struct device *dev, u32 guid, u64 *data, loff_t user_offset,
 		      u32 count)
 {
-- 
2.43.0

Re: [PATCH v5 09/18] drm/xe/vsec: Add DOC text for VSEC

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Low] Incorrect kernel-doc tag for return value in `xe_pmt_telem_read`.
- [Low] The newly added documentation file `xe_vsec.rst` is not included in any Sphinx `toctree`.
--

commit 011373dcf20ed7018b1279370ae8b1af346ffff5
Author: Michael J. Ruhl <michael.j.ruhl@intel.com>

drm/xe/vsec: Add DOC text for VSEC

This commit adds the necessary files and documentation text to use the DOC
system for the VSEC component.

> diff --git a/Documentation/gpu/xe/xe_vsec.rst b/Documentation/gpu/xe/xe_vsec.rst
> new file mode 100644
> index 0000000000000..1a87548489ea9
> --- /dev/null
> +++ b/Documentation/gpu/xe/xe_vsec.rst
> @@ -0,0 +1,14 @@
> +.. SPDX-License-Identifier: (GPL-2.0+ OR MIT)
> +
> +=====
> +VSEC
> +=====

[Severity: Low]
This isn't a bug, but should this newly added documentation file be included
in a Sphinx toctree, such as the one in Documentation/gpu/xe/index.rst? 

Without a toctree entry, the document remains orphaned. This triggers a Sphinx
build warning stating that the document isn't included in any toctree, and 
makes the text unreachable from the main generated documentation tree.

[ ... ]
> diff --git a/drivers/gpu/drm/xe/xe_vsec.c b/drivers/gpu/drm/xe/xe_vsec.c
> index 468a221378faf..5223bfe50ede6 100644
> --- a/drivers/gpu/drm/xe/xe_vsec.c
> +++ b/drivers/gpu/drm/xe/xe_vsec.c
[ ... ]
> @@ -140,6 +151,17 @@ static int xe_guid_decode(u32 guid, int *index, u32 *offset)
>  	return 0;
>  }
>  
> +/**
> + * xe_pmt_telem_read - Given a device and a PMT GUID, read data into a buffer
> + * @dev: valid Xe device
> + * @guid: PMT GUID (from the PMT discovery struct)
> + * @data: buffer to place data in
> + * @user_offset: a user offset relative to the beginning of the data to read
> + * @count: bytes to read
> + *
> + * Returns -EINVAL for VF and PMT WATCHER requests
> + *         count   number of bytes read on success
> + */

[Severity: Low]
This isn't a bug, but does this return value description violate the 
kernel-doc format? 

The scripts/kernel-doc tool expects return values to be formatted using the 
"Return:" section header. Not formatting this correctly causes the tool to fail 
to parse the return value, which can trigger a warning and render incorrectly 
in the generated HTML documentation.

>  int xe_pmt_telem_read(struct device *dev, u32 guid, u64 *data, loff_t user_offset,
>  		      u32 count)
>  {

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260903201002.1064889-20-michael.j.ruhl@intel.com?part=9

RE: [PATCH v5 09/18] drm/xe/vsec: Add DOC text for VSEC

[Ruhl, Michael J]

>-----Original Message-----
>From: sashiko-bot@kernel.org <sashiko-bot@kernel.org>
>Sent: Thursday, September 3, 2026 4:18 PM
>To: Ruhl, Michael J <michael.j.ruhl@intel.com>
>Cc: intel-xe@lists.freedesktop.org
>Subject: Re: [PATCH v5 09/18] drm/xe/vsec: Add DOC text for VSEC
>
>Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to
>consider:
>- [Low] Incorrect kernel-doc tag for return value in `xe_pmt_telem_read`.
>- [Low] The newly added documentation file `xe_vsec.rst` is not included in any
>Sphinx `toctree`.
>--
>
>commit 011373dcf20ed7018b1279370ae8b1af346ffff5
>Author: Michael J. Ruhl <michael.j.ruhl@intel.com>
>
>drm/xe/vsec: Add DOC text for VSEC
>
>This commit adds the necessary files and documentation text to use the DOC
>system for the VSEC component.
>
>> diff --git a/Documentation/gpu/xe/xe_vsec.rst
>b/Documentation/gpu/xe/xe_vsec.rst
>> new file mode 100644
>> index 0000000000000..1a87548489ea9
>> --- /dev/null
>> +++ b/Documentation/gpu/xe/xe_vsec.rst
>> @@ -0,0 +1,14 @@
>> +.. SPDX-License-Identifier: (GPL-2.0+ OR MIT)
>> +
>> +=====
>> +VSEC
>> +=====
>
>[Severity: Low]
>This isn't a bug, but should this newly added documentation file be included
>in a Sphinx toctree, such as the one in Documentation/gpu/xe/index.rst?
>
>Without a toctree entry, the document remains orphaned. This triggers a
>Sphinx
>build warning stating that the document isn't included in any toctree, and
>makes the text unreachable from the main generated documentation tree.

Yes, this is missing from the index.  I will add.

m

>[ ... ]
>> diff --git a/drivers/gpu/drm/xe/xe_vsec.c b/drivers/gpu/drm/xe/xe_vsec.c
>> index 468a221378faf..5223bfe50ede6 100644
>> --- a/drivers/gpu/drm/xe/xe_vsec.c
>> +++ b/drivers/gpu/drm/xe/xe_vsec.c
>[ ... ]
>> @@ -140,6 +151,17 @@ static int xe_guid_decode(u32 guid, int *index, u32
>*offset)
>>  	return 0;
>>  }
>>
>> +/**
>> + * xe_pmt_telem_read - Given a device and a PMT GUID, read data into a
>buffer
>> + * @dev: valid Xe device
>> + * @guid: PMT GUID (from the PMT discovery struct)
>> + * @data: buffer to place data in
>> + * @user_offset: a user offset relative to the beginning of the data to read
>> + * @count: bytes to read
>> + *
>> + * Returns -EINVAL for VF and PMT WATCHER requests
>> + *         count   number of bytes read on success
>> + */
>
>[Severity: Low]
>This isn't a bug, but does this return value description violate the
>kernel-doc format?
>
>The scripts/kernel-doc tool expects return values to be formatted using the
>"Return:" section header. Not formatting this correctly causes the tool to fail
>to parse the return value, which can trigger a warning and render incorrectly
>in the generated HTML documentation.
>
>>  int xe_pmt_telem_read(struct device *dev, u32 guid, u64 *data, loff_t
>user_offset,
>>  		      u32 count)
>>  {
>
>--
>Sashiko AI review ·
>https://sashiko.dev/#/patchset/20260903201002.1064889-20-
>michael.j.ruhl@intel.com?part=9

[PATCH v5 10/18] drm/xe/vsec: Support possible hotplug exit

[Michael J. Ruhl]

DRM has an API that will verify that a device is valid in the hotplug
context.

Verify device is valid before access in the VSEC callback API.

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

diff --git a/drivers/gpu/drm/xe/xe_vsec.c b/drivers/gpu/drm/xe/xe_vsec.c
index 5223bfe50ede..01df339ff62f 100644
--- a/drivers/gpu/drm/xe/xe_vsec.c
+++ b/drivers/gpu/drm/xe/xe_vsec.c
@@ -10,6 +10,8 @@
 #include <linux/pci.h>
 #include <linux/types.h>
 
+#include <drm/drm_drv.h>
+
 #include "xe_device.h"
 #include "xe_device_types.h"
 #include "xe_mmio.h"
@@ -159,8 +161,13 @@ static int xe_guid_decode(u32 guid, int *index, u32 *offset)
  * @user_offset: a user offset relative to the beginning of the data to read
  * @count: bytes to read
  *
- * Returns -EINVAL for VF and PMT WATCHER requests
- *         count   number of bytes read on success
+ * This is a callback API.  I.e this can be accessed external to the XE driver
+ * (PMT driver scope).  Because of this, DRM hotplug needs to be verified
+ * (drm_dev_enter()).
+ *
+ * Returns	-ENODEV device is not available (hotplug)
+ *		-EINVAL for VF and PMT WATCHER requests
+ *		count   number of bytes read on success
  */
 int xe_pmt_telem_read(struct device *dev, u32 guid, u64 *data, loff_t user_offset,
 		      u32 count)
@@ -170,14 +177,20 @@ int xe_pmt_telem_read(struct device *dev, u32 guid, u64 *data, loff_t user_offse
 	u32 cap_type = FIELD_GET(GUID_CAP_TYPE, guid);
 	u32 mem_region;
 	u32 offset;
-	int ret;
+	int ret = 0;
+	int idx;
 
-	if (IS_SRIOV_VF(xe))
-		return -EINVAL;
+	if (!drm_dev_enter(&xe->drm, &idx))
+		return -ENODEV;
+
+	if (IS_SRIOV_VF(xe)) {
+		ret = -EINVAL;
+		goto dev_exit;
+	}
 
 	ret = xe_guid_decode(guid, &mem_region, &offset);
 	if (ret)
-		return ret;
+		goto dev_exit;
 
 	telem_addr += offset + user_offset;
 
@@ -187,11 +200,14 @@ int xe_pmt_telem_read(struct device *dev, u32 guid, u64 *data, loff_t user_offse
 		xe_pm_runtime_get(xe);
 		break;
 	case TELEMETRY:
-		if (!xe_pm_runtime_get_if_active(xe))
-			return -ENODATA;
+		if (!xe_pm_runtime_get_if_active(xe)) {
+			ret = -ENODATA;
+			goto dev_exit;
+		}
 		break;
 	case WATCHER:
-		return -EINVAL;
+		ret = -EINVAL;
+		goto dev_exit;
 	}
 
 	mutex_lock(&xe->pmt.lock);
@@ -205,7 +221,10 @@ int xe_pmt_telem_read(struct device *dev, u32 guid, u64 *data, loff_t user_offse
 
 	xe_pm_runtime_put(xe);
 
-	return count;
+dev_exit:
+	drm_dev_exit(idx);
+
+	return ret == 0 ? count : ret;
 }
 
 static struct pmt_callbacks xe_pmt_cb = {
-- 
2.43.0

[PATCH v5 11/18] drm/xe/vsec: Support Crescent Island PMT

[Michael J. Ruhl]

Crescent Island (CRI) supports PMT telemetry and crashlog.

Add Crescent Island (CRI) discovery structure (DVSEC) information to
allow for Xe registration.

Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
Signed-off-by: Michael J. Ruhl <michael.j.ruhl@intel.com>
---
 drivers/gpu/drm/xe/regs/xe_pmt.h |  8 +++--
 drivers/gpu/drm/xe/xe_vsec.c     | 51 +++++++++++++++++++++++++++++---
 2 files changed, 53 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/xe/regs/xe_pmt.h b/drivers/gpu/drm/xe/regs/xe_pmt.h
index a62ab05c6b4c..2f6658395587 100644
--- a/drivers/gpu/drm/xe/regs/xe_pmt.h
+++ b/drivers/gpu/drm/xe/regs/xe_pmt.h
@@ -17,8 +17,12 @@
 
 #define PUNIT_VERSION_OFFSET		0xA0
 
-#define BMG_TELEMETRY_BASE_OFFSET	0xE0000
-#define BMG_TELEMETRY_OFFSET		(SOC_BASE + BMG_TELEMETRY_BASE_OFFSET)
+#define XE_PMT_BASE_OFFSET		0xE0000
+
+#define BMG_TELEMETRY_OFFSET		(SOC_BASE + XE_PMT_BASE_OFFSET)
+
+/* for CRI discovery and telemetry are in an indexed window */
+#define CRI_PMT_OFFSET			(SOC_BASE + XE_PMT_BASE_OFFSET)
 
 #define BMG_MODS_RESIDENCY_OFFSET		(0x4D0)
 #define BMG_G2_RESIDENCY_OFFSET		(0x530)
diff --git a/drivers/gpu/drm/xe/xe_vsec.c b/drivers/gpu/drm/xe/xe_vsec.c
index 01df339ff62f..59140ac85e25 100644
--- a/drivers/gpu/drm/xe/xe_vsec.c
+++ b/drivers/gpu/drm/xe/xe_vsec.c
@@ -8,6 +8,7 @@
 #include <linux/module.h>
 #include <linux/mutex.h>
 #include <linux/pci.h>
+#include <linux/sizes.h>
 #include <linux/types.h>
 
 #include <drm/drm_drv.h>
@@ -32,12 +33,23 @@
  * PMT usage.
  */
 
-/* PMT GUID value for BMG devices.  NOTE: this is NOT a PCI id */
+/* PMT GUID value for BMG and CRI devices.  NOTE: this is NOT a PCI id */
 #define BMG_DEVICE_ID 0xE2F8
+#define CRI_DEVICE_ID 0xE2FA
+
+/*
+ * sizeof(Crashlog Type1 Version2) = 0x18 (24) bytes
+ * For BMG and CRI PUNIT and OOBMSMS crashlogs are consecutive.
+ */
+#define BMG_CRASHLOG_PUNIT_DISC_OFFSET (0x60)
+#define BMG_CRASHLOG_OOBMSM_DISC_OFFSET (BMG_CRASHLOG_PUNIT_DISC_OFFSET + 0x18)
+
+#define CRI_CRASHLOG_PUNIT_DISC_OFFSET (0x80)
+#define CRI_CRASHLOG_OOBMSM_DISC_OFFSET (CRI_CRASHLOG_PUNIT_DISC_OFFSET + 0x18)
 
 static struct intel_vsec_header bmg_telemetry = {
 	.rev = 1,
-	.length = 0x10,
+	.length = SZ_16,
 	.id = VSEC_ID_TELEMETRY,
 	.num_entries = 2,
 	.entry_size = 4,
@@ -47,12 +59,12 @@ static struct intel_vsec_header bmg_telemetry = {
 
 static struct intel_vsec_header bmg_crashlog = {
 	.rev = 1,
-	.length = 0x10,
+	.length = SZ_16,
 	.id = VSEC_ID_CRASHLOG,
 	.num_entries = 2,
 	.entry_size = 6,
 	.tbir = 0,
-	.offset = BMG_DISCOVERY_OFFSET + 0x60,
+	.offset = BMG_DISCOVERY_OFFSET + BMG_CRASHLOG_PUNIT_DISC_OFFSET,
 };
 
 static struct intel_vsec_header *bmg_capabilities[] = {
@@ -61,9 +73,36 @@ static struct intel_vsec_header *bmg_capabilities[] = {
 	NULL
 };
 
+static struct intel_vsec_header cri_telemetry = {
+	.rev = 1,
+	.length = SZ_16,
+	.id = VSEC_ID_TELEMETRY,
+	.num_entries = 3,
+	.entry_size = 4,
+	.tbir = 0,
+	.offset = CRI_PMT_OFFSET,
+};
+
+static struct intel_vsec_header cri_crashlog = {
+	.rev = 1,
+	.length = SZ_16,
+	.id = VSEC_ID_CRASHLOG,
+	.num_entries = 2,
+	.entry_size = 6,
+	.tbir = 0,
+	.offset = CRI_PMT_OFFSET + CRI_CRASHLOG_PUNIT_DISC_OFFSET,
+};
+
+static struct intel_vsec_header *cri_capabilities[] = {
+	&cri_telemetry,
+	&cri_crashlog,
+	NULL
+};
+
 enum xe_vsec {
 	XE_VSEC_UNKNOWN = 0,
 	XE_VSEC_BMG,
+	XE_VSEC_CRI,
 };
 
 static struct intel_vsec_platform_info xe_vsec_info[] = {
@@ -71,6 +110,10 @@ static struct intel_vsec_platform_info xe_vsec_info[] = {
 		.caps = VSEC_CAP_TELEMETRY | VSEC_CAP_CRASHLOG,
 		.headers = bmg_capabilities,
 	},
+	[XE_VSEC_CRI] = {
+		.caps = VSEC_CAP_TELEMETRY | VSEC_CAP_CRASHLOG,
+		.headers = cri_capabilities,
+	},
 	{ }
 };
 
-- 
2.43.0

[PATCH v5 12/18] drm/xe/vsec: Refactor BattleMage PMT defines

[Michael J. Ruhl]

Rename generic defines to be BMG specific.
Define several magic numbers.
Add a base offset for more flexibility.

Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
Signed-off-by: Michael J. Ruhl <michael.j.ruhl@intel.com>
---
 drivers/gpu/drm/xe/xe_device_types.h |  2 ++
 drivers/gpu/drm/xe/xe_vsec.c         | 32 +++++++++++++++++-----------
 2 files changed, 21 insertions(+), 13 deletions(-)

diff --git a/drivers/gpu/drm/xe/xe_device_types.h b/drivers/gpu/drm/xe/xe_device_types.h
index 180d450a6deb..3f1a70813a99 100644
--- a/drivers/gpu/drm/xe/xe_device_types.h
+++ b/drivers/gpu/drm/xe/xe_device_types.h
@@ -466,6 +466,8 @@ struct xe_device {
 	struct {
 		/** @pmt.lock: protect access for telemetry data */
 		struct mutex lock;
+		/** @pmt.base_offset: device specific base offset */
+		u64 base_offset;
 	} pmt;
 
 	/** @soc_remapper: SoC remapper object */
diff --git a/drivers/gpu/drm/xe/xe_vsec.c b/drivers/gpu/drm/xe/xe_vsec.c
index 59140ac85e25..d856a38f4d89 100644
--- a/drivers/gpu/drm/xe/xe_vsec.c
+++ b/drivers/gpu/drm/xe/xe_vsec.c
@@ -133,10 +133,15 @@ static struct intel_vsec_platform_info xe_vsec_info[] = {
 #define GUID_CAP_TYPE		GENMASK(29, 28)
 #define GUID_RECORD_ID		GENMASK(31, 30)
 
-#define PUNIT_TELEMETRY_OFFSET		0x0200
-#define PUNIT_WATCHER_OFFSET		0x14A0
-#define OOBMSM_0_WATCHER_OFFSET		0x18D8
-#define OOBMSM_1_TELEMETRY_OFFSET	0x1000
+#define BMG_IDX_TELEM_PUNIT		0x00
+#define BMG_IDX_TELEM_OOBMSM		0x01
+#define BMG_IDX_CRASHLOG_PUNIT		0x02
+#define BMG_IDX_CRASHLOG_OOBMSM		0x04
+
+#define BMG_PUNIT_TELEMETRY_OFFSET	0x0200
+#define BMG_PUNIT_WATCHER_OFFSET	0x14A0
+#define BMG_OOBMSM_0_WATCHER_OFFSET	0x18D8
+#define BMG_OOBMSM_1_TELEMETRY_OFFSET	0x1000
 
 enum record_id {
 	PUNIT,
@@ -165,29 +170,29 @@ static int xe_guid_decode(u32 guid, int *index, u32 *offset)
 	*offset = 0;
 
 	if (cap_type == CRASHLOG) {
-		*index = record_id == PUNIT ? 2 : 4;
+		*index = record_id == PUNIT ? BMG_IDX_CRASHLOG_PUNIT : BMG_IDX_CRASHLOG_OOBMSM;
 		return 0;
 	}
 
 	switch (record_id) {
 	case PUNIT:
-		*index = 0;
+		*index = BMG_IDX_TELEM_PUNIT;
 		if (cap_type == TELEMETRY)
-			*offset = PUNIT_TELEMETRY_OFFSET;
+			*offset = BMG_PUNIT_TELEMETRY_OFFSET;
 		else
-			*offset = PUNIT_WATCHER_OFFSET;
+			*offset = BMG_PUNIT_WATCHER_OFFSET;
 		break;
 
 	case OOBMSM_0:
-		*index = 1;
+		*index = BMG_IDX_TELEM_OOBMSM;
 		if (cap_type == WATCHER)
-			*offset = OOBMSM_0_WATCHER_OFFSET;
+			*offset = BMG_OOBMSM_0_WATCHER_OFFSET;
 		break;
 
 	case OOBMSM_1:
-		*index = 1;
+		*index = BMG_IDX_TELEM_OOBMSM;
 		if (cap_type == TELEMETRY)
-			*offset = OOBMSM_1_TELEMETRY_OFFSET;
+			*offset = BMG_OOBMSM_1_TELEMETRY_OFFSET;
 		break;
 	default:
 		return -EINVAL;
@@ -216,8 +221,8 @@ int xe_pmt_telem_read(struct device *dev, u32 guid, u64 *data, loff_t user_offse
 		      u32 count)
 {
 	struct xe_device *xe = kdev_to_xe_device(dev);
-	void __iomem *telem_addr = xe->mmio.regs + BMG_TELEMETRY_OFFSET;
 	u32 cap_type = FIELD_GET(GUID_CAP_TYPE, guid);
+	void __iomem *telem_addr = xe->mmio.regs + xe->pmt.base_offset;
 	u32 mem_region;
 	u32 offset;
 	int ret = 0;
@@ -309,6 +314,7 @@ void xe_vsec_init(struct xe_device *xe)
 	case XE_VSEC_BMG:
 		if (IS_SRIOV_VF(xe))
 			return;
+		xe->pmt.base_offset = BMG_TELEMETRY_OFFSET;
 		info->priv_data = &xe_pmt_cb;
 		break;
 	default:
-- 
2.43.0

[PATCH v5 13/18] drm/xe/vsec: Crescent Island PMT decode

[Michael J. Ruhl]

Crescent Island (CRI) has different index and offset values for
accessing the PMT data area.

Update the decode path to support the CRI device.

Update the data read callback so to support the CRI usage.

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

diff --git a/drivers/gpu/drm/xe/xe_vsec.c b/drivers/gpu/drm/xe/xe_vsec.c
index d856a38f4d89..0438810337dd 100644
--- a/drivers/gpu/drm/xe/xe_vsec.c
+++ b/drivers/gpu/drm/xe/xe_vsec.c
@@ -120,19 +120,25 @@ static struct intel_vsec_platform_info xe_vsec_info[] = {
 /*
  * The GUID will have the following bits to decode:
  *   [0:3]   - {Telemetry space iteration number (0,1,..)}
- *   [4:7]   - Segment (SEGMENT_INDEPENDENT-0, Client-1, Server-2)
+ *   [4:7]   - BMG Segment (SEGMENT_INDEPENDENT-0, Client-1, Server-2)
+ *   [4:5]   - CRI Segment (SEGMENT_INDEPENDENT-0, Client-1, Server-2)
+ *   [6:7]   - CRI Instance
  *   [8:11]  - SOC_SKU
  *   [12:27] – Device ID – changes for each down bin SKU’s
  *   [28:29] - Capability Type (Crashlog-0, Telemetry Aggregator-1, Watcher-2)
  *   [30:31] - Record-ID (0-PUNIT, 1-OOBMSM_0, 2-OOBMSM_1)
  */
 #define GUID_TELEM_ITERATION	GENMASK(3, 0)
-#define GUID_SEGMENT		GENMASK(7, 4)
 #define GUID_SOC_SKU		GENMASK(11, 8)
 #define GUID_DEVICE_ID		GENMASK(27, 12)
 #define GUID_CAP_TYPE		GENMASK(29, 28)
 #define GUID_RECORD_ID		GENMASK(31, 30)
 
+#define BMG_GUID_SEGMENT	GENMASK(7, 4)
+
+#define CRI_GUID_SEGMENT	GENMASK(5, 4)
+#define CRI_GUID_INSTANCE	GENMASK(7, 6)
+
 #define BMG_IDX_TELEM_PUNIT		0x00
 #define BMG_IDX_TELEM_OOBMSM		0x01
 #define BMG_IDX_CRASHLOG_PUNIT		0x02
@@ -143,6 +149,18 @@ static struct intel_vsec_platform_info xe_vsec_info[] = {
 #define BMG_OOBMSM_0_WATCHER_OFFSET	0x18D8
 #define BMG_OOBMSM_1_TELEMETRY_OFFSET	0x1000
 
+#define CRI_IDX_TELEM_DISCOVERY		0x00
+#define CRI_IDX_TELEM_PUNIT		0x01
+#define CRI_IDX_TELEM_OOBMSM		0x02
+#define CRI_IDX_CRASHLOG_PUNIT		0x03
+#define CRI_IDX_WATCHER_OOBMSM		0x03  /* PUNIT and OOBMSM share this index */
+#define CRI_IDX_CRASHLOG_OOBMSM		0x04
+
+#define CRI_PUNIT_WATCHER_OFFSET		0x08A0
+#define CRI_OOBMSM_WATCHER_OFFSET		0x0CF8
+#define CRI_OOBMSM_GFSP_TELEMETRY_OFFSET	0x1600
+#define CRI_PUNIT_CRASHLOG_OFFSET		0x0E78
+
 enum record_id {
 	PUNIT,
 	OOBMSM_0,
@@ -155,17 +173,10 @@ enum capability {
 	WATCHER,
 };
 
-static int xe_guid_decode(u32 guid, int *index, u32 *offset)
+static int bmg_guid_decode(u32 guid, int *index, u32 *offset)
 {
 	u32 record_id = FIELD_GET(GUID_RECORD_ID, guid);
 	u32 cap_type  = FIELD_GET(GUID_CAP_TYPE, guid);
-	u32 device_id = FIELD_GET(GUID_DEVICE_ID, guid);
-
-	if (device_id != BMG_DEVICE_ID)
-		return -ENODEV;
-
-	if (cap_type > WATCHER)
-		return -EINVAL;
 
 	*offset = 0;
 
@@ -201,6 +212,75 @@ static int xe_guid_decode(u32 guid, int *index, u32 *offset)
 	return 0;
 }
 
+static int cri_guid_decode(u32 guid, int *index, u32 *offset)
+{
+	u32 record_id = FIELD_GET(GUID_RECORD_ID, guid);
+	u32 cap_type  = FIELD_GET(GUID_CAP_TYPE, guid);
+	u32 instance  = FIELD_GET(CRI_GUID_INSTANCE, guid);
+
+	*offset = 0;
+
+	if (cap_type == CRASHLOG) {
+		if (record_id == PUNIT) {
+			*index = CRI_IDX_CRASHLOG_PUNIT;
+			*offset = CRI_PUNIT_CRASHLOG_OFFSET;
+		} else {
+			*index = CRI_IDX_CRASHLOG_OOBMSM;
+		}
+		return 0;
+	}
+
+	switch (record_id) {
+	case PUNIT:
+		*index = CRI_IDX_TELEM_PUNIT;
+		if (cap_type == WATCHER)
+			*offset = CRI_PUNIT_WATCHER_OFFSET;
+		break;
+
+	case OOBMSM_0:
+		*index = CRI_IDX_TELEM_OOBMSM;
+		switch (instance) {
+		case 0:
+			if (cap_type == WATCHER) {
+				*index = CRI_IDX_WATCHER_OOBMSM;
+				*offset = CRI_OOBMSM_WATCHER_OFFSET;
+			}
+			break;
+
+		case 1:
+			if (cap_type == TELEMETRY)
+				*offset = CRI_OOBMSM_GFSP_TELEMETRY_OFFSET;
+			break;
+
+		default:
+			return -EINVAL;
+		}
+		break;
+
+	default:
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+static int xe_guid_decode(u32 guid, int *index, u32 *offset)
+{
+	u32 cap_type  = FIELD_GET(GUID_CAP_TYPE, guid);
+	u32 device_id = FIELD_GET(GUID_DEVICE_ID, guid);
+
+	if (cap_type > WATCHER)
+		return -EINVAL;
+
+	if (device_id == BMG_DEVICE_ID)
+		return bmg_guid_decode(guid, index, offset);
+
+	if (device_id == CRI_DEVICE_ID)
+		return cri_guid_decode(guid, index, offset);
+
+	return -ENODEV;
+}
+
 /**
  * xe_pmt_telem_read - Given a device and a PMT GUID, read data into a buffer
  * @dev: valid Xe device
-- 
2.43.0

[PATCH v5 14/18] drm/xe/vsec: Crescent Island PMT callbacks

[Michael J. Ruhl]

CRI PMT support requires callbacks to access the discovery status and
control areas. Access is a common MMIO area that requires an index to
be set before access is allowed.

Introduce the necessary callbacks to get the status and control
information for CRI PMT usage.

Add the glue logic to register the CRI PMT functionality.

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

diff --git a/drivers/gpu/drm/xe/xe_vsec.c b/drivers/gpu/drm/xe/xe_vsec.c
index 0438810337dd..336c4b7d284b 100644
--- a/drivers/gpu/drm/xe/xe_vsec.c
+++ b/drivers/gpu/drm/xe/xe_vsec.c
@@ -355,17 +355,110 @@ int xe_pmt_telem_read(struct device *dev, u32 guid, u64 *data, loff_t user_offse
 	return ret == 0 ? count : ret;
 }
 
-static struct pmt_callbacks xe_pmt_cb = {
+/**
+ * xe_pmt_read_reg() - read a crashlog register
+ * @dev: the xe device that registered the callback
+ * @guid: PMT guid of the crashlog instance
+ * @reg: data read from the PMT data structure
+ * @offset: which data to read from the PMT data structure
+ *
+ * Read the requested PMT register based on the pcie device and guid.  The
+ * supported struct is the Crashlog Type1 Version2.
+ *
+ * Currently this is for CRI only.
+ */
+static int xe_pmt_read_reg(struct device *dev, u32 guid, u32 *reg, u32 offset)
+{
+	struct xe_device *xe = kdev_to_xe_device(dev);
+	void __iomem *disc_addr = xe->mmio.regs + xe->pmt.base_offset;
+	int ret = 0;
+	u32 inst;
+	int idx;
+
+	if (!drm_dev_enter(&xe->drm, &idx))
+		return -ENODEV;
+
+	if (FIELD_GET(GUID_DEVICE_ID, guid) != CRI_DEVICE_ID ||
+	    FIELD_GET(GUID_CAP_TYPE, guid) != CRASHLOG) {
+		ret = -EINVAL;
+		goto dev_exit;
+	}
+
+	inst = FIELD_GET(GUID_RECORD_ID, guid) == PUNIT ?
+		CRI_CRASHLOG_PUNIT_DISC_OFFSET : CRI_CRASHLOG_OOBMSM_DISC_OFFSET;
+	disc_addr += inst + offset;
+
+	xe_pm_runtime_get(xe);
+	mutex_lock(&xe->pmt.lock);
+
+	xe->soc_remapper.set_telem_region(xe, CRI_IDX_TELEM_DISCOVERY);
+
+	memcpy_fromio(reg, disc_addr, sizeof(*reg));
+
+	mutex_unlock(&xe->pmt.lock);
+	xe_pm_runtime_put(xe);
+
+dev_exit:
+	drm_dev_exit(idx);
+
+	return ret;
+}
+
+static int xe_pmt_write_reg(struct device *dev, u32 guid, u32 reg, u32 offset)
+{
+	struct xe_device *xe = kdev_to_xe_device(dev);
+	void __iomem *disc_addr = xe->mmio.regs + xe->pmt.base_offset;
+	int ret = 0;
+	u32 inst;
+	int idx;
+
+	if (!drm_dev_enter(&xe->drm, &idx))
+		return -ENODEV;
+
+	if (FIELD_GET(GUID_DEVICE_ID, guid) != CRI_DEVICE_ID ||
+	    FIELD_GET(GUID_CAP_TYPE, guid) != CRASHLOG) {
+		ret = -EINVAL;
+		goto dev_exit;
+	}
+
+	inst = FIELD_GET(GUID_RECORD_ID, guid) == PUNIT ?
+		CRI_CRASHLOG_PUNIT_DISC_OFFSET : CRI_CRASHLOG_OOBMSM_DISC_OFFSET;
+	disc_addr += inst + offset;
+
+	xe_pm_runtime_get(xe);
+	mutex_lock(&xe->pmt.lock);
+
+	xe->soc_remapper.set_telem_region(xe, CRI_IDX_TELEM_DISCOVERY);
+
+	memcpy_toio(disc_addr, &reg, sizeof(reg));
+
+	mutex_unlock(&xe->pmt.lock);
+	xe_pm_runtime_put(xe);
+
+dev_exit:
+	drm_dev_exit(idx);
+
+	return ret;
+}
+
+static struct pmt_callbacks xe_bmg_pmt_cb = {
 	.read_telem = xe_pmt_telem_read,
 };
 
+static struct pmt_callbacks xe_cri_pmt_cb = {
+	.read_telem = xe_pmt_telem_read,
+	.read_reg = xe_pmt_read_reg,
+	.write_reg = xe_pmt_write_reg,
+};
+
 static const int vsec_platforms[] = {
 	[XE_BATTLEMAGE] = XE_VSEC_BMG,
+	[XE_CRESCENTISLAND] = XE_VSEC_CRI,
 };
 
 static enum xe_vsec get_platform_info(struct xe_device *xe)
 {
-	if (xe->info.platform > XE_BATTLEMAGE)
+	if (xe->info.platform > XE_CRESCENTISLAND)
 		return XE_VSEC_UNKNOWN;
 
 	return vsec_platforms[xe->info.platform];
@@ -395,8 +488,17 @@ void xe_vsec_init(struct xe_device *xe)
 		if (IS_SRIOV_VF(xe))
 			return;
 		xe->pmt.base_offset = BMG_TELEMETRY_OFFSET;
-		info->priv_data = &xe_pmt_cb;
+		info->priv_data = &xe_bmg_pmt_cb;
 		break;
+
+	case XE_VSEC_CRI:
+		if (IS_SRIOV_VF(xe))
+			return;
+		xe->pmt.base_offset = CRI_PMT_OFFSET;
+		info->priv_data = &xe_cri_pmt_cb;
+		xe->soc_remapper.set_telem_region(xe, CRI_IDX_TELEM_DISCOVERY);
+		break;
+
 	default:
 		break;
 	}
-- 
2.43.0

Re: [PATCH v5 14/18] drm/xe/vsec: Crescent Island PMT callbacks

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] Using memcpy_toio() and memcpy_fromio() for 32-bit MMIO control registers violates atomicity, lacks memory barriers, and breaks endianness on big-endian architectures.
--

commit dbad67bc0c83cc3b6a60d78b23589e00b666a0f0
Author: Michael J. Ruhl <michael.j.ruhl@intel.com>

drm/xe/vsec: Crescent Island PMT callbacks

This commit introduces callbacks for Crescent Island PMT support to access
the discovery status and control areas. It adds the glue logic and registers
the functionality for the XE_CRESCENTISLAND platform.

> diff --git a/drivers/gpu/drm/xe/xe_vsec.c b/drivers/gpu/drm/xe/xe_vsec.c
> index 0438810337dd3..336c4b7d284bc 100644
> --- a/drivers/gpu/drm/xe/xe_vsec.c
> +++ b/drivers/gpu/drm/xe/xe_vsec.c
[ ... ]
> +static int xe_pmt_read_reg(struct device *dev, u32 guid, u32 *reg, u32 offset)
> +{
[ ... ]
> +	xe_pm_runtime_get(xe);
> +	mutex_lock(&xe->pmt.lock);
> +
> +	xe->soc_remapper.set_telem_region(xe, CRI_IDX_TELEM_DISCOVERY);
> +
> +	memcpy_fromio(reg, disc_addr, sizeof(*reg));

[Severity: Medium]
Does using memcpy_fromio() here instead of readl() cause endianness issues
on big-endian platforms?

Since this is reading a 32-bit register, memcpy_fromio() omits endian
conversion and implicit memory barriers. It might also break atomicity or
cause PCIe target aborts if the underlying implementation falls back to 8-bit
byte accesses, which some hardware rejects for 32-bit registers.

> +
> +	mutex_unlock(&xe->pmt.lock);
> +	xe_pm_runtime_put(xe);
[ ... ]
> +static int xe_pmt_write_reg(struct device *dev, u32 guid, u32 reg, u32 offset)
> +{
[ ... ]
> +	xe_pm_runtime_get(xe);
> +	mutex_lock(&xe->pmt.lock);
> +
> +	xe->soc_remapper.set_telem_region(xe, CRI_IDX_TELEM_DISCOVERY);
> +
> +	memcpy_toio(disc_addr, &reg, sizeof(reg));

[Severity: Medium]
Could memcpy_toio() write these bytes in CPU endianness instead of the
device's expected little-endian format? 

Using writel() here would properly handle endianness conversion, ensure 
I/O memory barriers, and guarantee a single atomic 32-bit write operation.

> +
> +	mutex_unlock(&xe->pmt.lock);
> +	xe_pm_runtime_put(xe);
[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260903201002.1064889-20-michael.j.ruhl@intel.com?part=14

[PATCH v5 15/18] drm/xe/vsec: Support late bind fw information

[Michael J. Ruhl]

CRI FW is loaded on power on.  Because of this, access to the FW cannot
be done until it is running.

Update the XE PMT probe and access to check for late bind devices,
verify, and wait for the appropriate FW state before probe or access.

Signed-off-by: Michael J. Ruhl <michael.j.ruhl@intel.com>
---
 drivers/gpu/drm/xe/xe_device.c       |   4 +-
 drivers/gpu/drm/xe/xe_device_types.h |   5 +
 drivers/gpu/drm/xe/xe_vsec.c         | 134 +++++++++++++++++++++++++--
 drivers/gpu/drm/xe/xe_vsec.h         |   2 +-
 4 files changed, 137 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/xe/xe_device.c b/drivers/gpu/drm/xe/xe_device.c
index 8583b2e9ecf4..e51e3cc4ea62 100644
--- a/drivers/gpu/drm/xe/xe_device.c
+++ b/drivers/gpu/drm/xe/xe_device.c
@@ -1148,7 +1148,9 @@ int xe_device_probe(struct xe_device *xe)
 	for_each_gt(gt, xe, id)
 		xe_gt_sanitize_freq(gt);
 
-	xe_vsec_init(xe);
+	err = xe_vsec_init(xe);
+	if (err)
+		goto err_unregister_display;
 
 	err = xe_sriov_init_late(xe);
 	if (err)
diff --git a/drivers/gpu/drm/xe/xe_device_types.h b/drivers/gpu/drm/xe/xe_device_types.h
index 3f1a70813a99..69e052ac5a82 100644
--- a/drivers/gpu/drm/xe/xe_device_types.h
+++ b/drivers/gpu/drm/xe/xe_device_types.h
@@ -7,6 +7,7 @@
 #define _XE_DEVICE_TYPES_H_
 
 #include <linux/pci.h>
+#include <linux/workqueue.h>
 
 #include <drm/drm_device.h>
 #include <drm/drm_file.h>
@@ -468,6 +469,10 @@ struct xe_device {
 		struct mutex lock;
 		/** @pmt.base_offset: device specific base offset */
 		u64 base_offset;
+		/** @pmt.work: support late-bind probe */
+		struct delayed_work work;
+		/** @pmt.retry_count: late-bind probe retry */
+		u32 retry_count;
 	} pmt;
 
 	/** @soc_remapper: SoC remapper object */
diff --git a/drivers/gpu/drm/xe/xe_vsec.c b/drivers/gpu/drm/xe/xe_vsec.c
index 336c4b7d284b..48cacd23b071 100644
--- a/drivers/gpu/drm/xe/xe_vsec.c
+++ b/drivers/gpu/drm/xe/xe_vsec.c
@@ -3,6 +3,7 @@
 #include <linux/bitfield.h>
 #include <linux/bits.h>
 #include <linux/cleanup.h>
+#include <linux/delay.h>
 #include <linux/errno.h>
 #include <linux/intel_vsec.h>
 #include <linux/module.h>
@@ -18,6 +19,7 @@
 #include "xe_mmio.h"
 #include "xe_platform_types.h"
 #include "xe_pm.h"
+#include "xe_sysctrl.h"
 #include "xe_vsec.h"
 
 #include "regs/xe_pmt.h"
@@ -173,6 +175,14 @@ enum capability {
 	WATCHER,
 };
 
+/*
+ * Late bind will delay 100msec for up to 20 seconds
+ */
+#define VSEC_LATE_BIND_DELAY_MSEC	100
+#define VSEC_LATE_BIND_RETRY		200
+
+static void cri_late_bind_probe(struct xe_device *xe);
+
 static int bmg_guid_decode(u32 guid, int *index, u32 *offset)
 {
 	u32 record_id = FIELD_GET(GUID_RECORD_ID, guid);
@@ -281,6 +291,48 @@ static int xe_guid_decode(u32 guid, int *index, u32 *offset)
 	return -ENODEV;
 }
 
+static void cri_late_bind_probe_work(struct work_struct *work)
+{
+	struct xe_device *xe = container_of(work, struct xe_device, pmt.work.work);
+
+	if (xe_is_oobmsm_fw_ready(xe)) {
+		cri_late_bind_probe(xe);
+		xe_pm_runtime_put(xe);
+		return;
+	}
+
+	xe->pmt.retry_count++;
+
+	/* wait up to 20 seconds */
+	if (xe->pmt.retry_count == VSEC_LATE_BIND_RETRY) {
+		drm_warn(&xe->drm, "PMT probe: Late Binding failed to complete\n");
+		xe_pm_runtime_put(xe);
+		return;
+	}
+
+	if (!schedule_delayed_work(&xe->pmt.work, msecs_to_jiffies(VSEC_LATE_BIND_DELAY_MSEC)))
+		xe_pm_runtime_put(xe);
+}
+
+static bool wait_for_fw(struct xe_device *xe)
+{
+	int retries = VSEC_LATE_BIND_RETRY;  /* wait up to 20 secs */
+
+	if (xe->info.platform != XE_CRESCENTISLAND)
+		return true;
+
+	while (retries--) {
+		if (xe_is_oobmsm_fw_ready(xe))
+			return true;
+
+		msleep(VSEC_LATE_BIND_DELAY_MSEC);
+	}
+
+	drm_warn(&xe->drm, "Late Binding failed to complete\n");
+
+	return false;
+}
+
 /**
  * xe_pmt_telem_read - Given a device and a PMT GUID, read data into a buffer
  * @dev: valid Xe device
@@ -338,6 +390,11 @@ int xe_pmt_telem_read(struct device *dev, u32 guid, u64 *data, loff_t user_offse
 		goto dev_exit;
 	}
 
+	if (!wait_for_fw(xe)) {
+		ret = -ENODATA;
+		goto runtime_exit;
+	}
+
 	mutex_lock(&xe->pmt.lock);
 
 	/* set SoC re-mapper index register based on GUID memory region */
@@ -347,6 +404,7 @@ int xe_pmt_telem_read(struct device *dev, u32 guid, u64 *data, loff_t user_offse
 
 	mutex_unlock(&xe->pmt.lock);
 
+runtime_exit:
 	xe_pm_runtime_put(xe);
 
 dev_exit:
@@ -389,6 +447,10 @@ static int xe_pmt_read_reg(struct device *dev, u32 guid, u32 *reg, u32 offset)
 	disc_addr += inst + offset;
 
 	xe_pm_runtime_get(xe);
+	if (!wait_for_fw(xe)) {
+		ret = -ENODATA;
+		goto runtime_exit;
+	}
 	mutex_lock(&xe->pmt.lock);
 
 	xe->soc_remapper.set_telem_region(xe, CRI_IDX_TELEM_DISCOVERY);
@@ -396,6 +458,8 @@ static int xe_pmt_read_reg(struct device *dev, u32 guid, u32 *reg, u32 offset)
 	memcpy_fromio(reg, disc_addr, sizeof(*reg));
 
 	mutex_unlock(&xe->pmt.lock);
+
+runtime_exit:
 	xe_pm_runtime_put(xe);
 
 dev_exit:
@@ -426,6 +490,10 @@ static int xe_pmt_write_reg(struct device *dev, u32 guid, u32 reg, u32 offset)
 	disc_addr += inst + offset;
 
 	xe_pm_runtime_get(xe);
+	if (!wait_for_fw(xe)) {
+		ret = -ENODATA;
+		goto runtime_exit;
+	}
 	mutex_lock(&xe->pmt.lock);
 
 	xe->soc_remapper.set_telem_region(xe, CRI_IDX_TELEM_DISCOVERY);
@@ -433,6 +501,8 @@ static int xe_pmt_write_reg(struct device *dev, u32 guid, u32 reg, u32 offset)
 	memcpy_toio(disc_addr, &reg, sizeof(reg));
 
 	mutex_unlock(&xe->pmt.lock);
+
+runtime_exit:
 	xe_pm_runtime_put(xe);
 
 dev_exit:
@@ -464,12 +534,44 @@ static enum xe_vsec get_platform_info(struct xe_device *xe)
 	return vsec_platforms[xe->info.platform];
 }
 
+static void cri_late_bind_probe(struct xe_device *xe)
+{
+	struct intel_vsec_platform_info *info;
+	struct device *dev = xe->drm.dev;
+	enum xe_vsec platform;
+
+	platform = get_platform_info(xe);
+	if (platform != XE_VSEC_CRI)
+		return;
+
+	info = &xe_vsec_info[platform];
+	if (!info->headers)
+		return;
+
+	info->priv_data = &xe_cri_pmt_cb;
+	xe->soc_remapper.set_telem_region(xe, CRI_IDX_TELEM_DISCOVERY);
+
+	intel_vsec_register(dev, info);
+}
+
+static void vsec_disable_late_bind_work(void *arg)
+{
+	struct xe_device *xe = arg;
+
+	/*
+	 * If the queued work is canceled, the runtime reference needs to be
+	 * released here.
+	 */
+	if (disable_delayed_work_sync(&xe->pmt.work))
+		xe_pm_runtime_put(xe);
+}
+
 /**
  * xe_vsec_init - Initialize resources and add intel_vsec auxiliary
  * interface
  * @xe: valid xe instance
  */
-void xe_vsec_init(struct xe_device *xe)
+int xe_vsec_init(struct xe_device *xe)
 {
 	struct intel_vsec_platform_info *info;
 	struct device *dev = xe->drm.dev;
@@ -477,30 +579,45 @@ void xe_vsec_init(struct xe_device *xe)
 
 	platform = get_platform_info(xe);
 	if (platform == XE_VSEC_UNKNOWN)
-		return;
+		return 0;
 
 	info = &xe_vsec_info[platform];
 	if (!info->headers)
-		return;
+		return 0;
 
 	switch (platform) {
 	case XE_VSEC_BMG:
 		if (IS_SRIOV_VF(xe))
-			return;
+			return 0;
 		xe->pmt.base_offset = BMG_TELEMETRY_OFFSET;
 		info->priv_data = &xe_bmg_pmt_cb;
 		break;
 
 	case XE_VSEC_CRI:
 		if (IS_SRIOV_VF(xe))
-			return;
+			return 0;
+
 		xe->pmt.base_offset = CRI_PMT_OFFSET;
+
+		xe->pmt.retry_count = 0;
+		INIT_DELAYED_WORK(&xe->pmt.work, cri_late_bind_probe_work);
+
+		xe_pm_runtime_get_noresume(xe);
+		if (!xe_is_oobmsm_fw_ready(xe)) {
+			schedule_delayed_work(&xe->pmt.work,
+					      msecs_to_jiffies(VSEC_LATE_BIND_DELAY_MSEC));
+			return devm_add_action_or_reset(xe->drm.dev,
+							vsec_disable_late_bind_work,
+							xe);
+		}
+
 		info->priv_data = &xe_cri_pmt_cb;
 		xe->soc_remapper.set_telem_region(xe, CRI_IDX_TELEM_DISCOVERY);
 		break;
 
 	default:
-		break;
+		drm_err(&xe->drm, "Unsupported platform: %u\n", platform);
+		return 0;
 	}
 
 	/*
@@ -508,5 +625,10 @@ void xe_vsec_init(struct xe_device *xe)
 	 * resources.
 	 */
 	intel_vsec_register(dev, info);
+
+	if (platform == XE_VSEC_CRI)
+		xe_pm_runtime_put(xe);
+
+	return 0;
 }
 MODULE_IMPORT_NS("INTEL_VSEC");
diff --git a/drivers/gpu/drm/xe/xe_vsec.h b/drivers/gpu/drm/xe/xe_vsec.h
index a25b4e6e681b..c4a1e2fc67d8 100644
--- a/drivers/gpu/drm/xe/xe_vsec.h
+++ b/drivers/gpu/drm/xe/xe_vsec.h
@@ -9,7 +9,7 @@
 struct device;
 struct xe_device;
 
-void xe_vsec_init(struct xe_device *xe);
+int xe_vsec_init(struct xe_device *xe);
 int xe_pmt_telem_read(struct device *dev, u32 guid, u64 *data, loff_t user_offset, u32 count);
 
 #endif
-- 
2.43.0

Re: [PATCH v5 15/18] drm/xe/vsec: Support late bind fw information

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Asynchronous `devm` registration race causes memory leak and Use-After-Free of the auxiliary device.
- [High] Asynchronous runtime PM put during devm teardown leaves hardware in an incorrect power state.
- [High] Delayed work accesses hardware MMIO during system suspend.
--

commit 1b67a780c3fbc619f441f6dfdc210d11c50f6476
Author: Michael J. Ruhl <michael.j.ruhl@intel.com>

drm/xe/vsec: Support late bind fw information

CRI FW is loaded on power on. Because of this, access to the FW cannot
be done until it is running. Update the XE PMT probe and access to check
for late bind devices, verify, and wait for the appropriate FW state
before probe or access.

> diff --git a/drivers/gpu/drm/xe/xe_vsec.c b/drivers/gpu/drm/xe/xe_vsec.c
> index 336c4b7d284bc..48cacd23b071e 100644
> --- a/drivers/gpu/drm/xe/xe_vsec.c
> +++ b/drivers/gpu/drm/xe/xe_vsec.c
[ ... ]
> @@ -281,6 +291,48 @@ static int xe_guid_decode(u32 guid, int *index, u32 *offset)
>  	return -ENODEV;
>  }
>  
> +static void cri_late_bind_probe_work(struct work_struct *work)
> +{
> +	struct xe_device *xe = container_of(work, struct xe_device, pmt.work.work);
> +
> +	if (xe_is_oobmsm_fw_ready(xe)) {
[Severity: High]
Does this code access hardware MMIO during system suspend?

If a system suspend (e.g., S3) is initiated while the worker is actively
polling, the device is forced into a suspended state (D3) by the system PM
core, bypassing the runtime PM usage count held by the worker.

Because the work is on a non-freezable workqueue and not explicitly
cancelled in the driver's system suspend callbacks, the worker can wake up
while the hardware is powered down and call xe_is_oobmsm_fw_ready().
Reading MMIO from a device in the D3 state causes master aborts, unhandled
PCI errors, or silent 0xFFFFFFFF reads leading to undefined behavior.

> +		cri_late_bind_probe(xe);
> +		xe_pm_runtime_put(xe);
> +		return;
> +	}
[ ... ]
> @@ -464,12 +534,44 @@ static enum xe_vsec get_platform_info(struct xe_device *xe)
[ ... ]
> +static void cri_late_bind_probe(struct xe_device *xe)
> +{
[ ... ]
> +	intel_vsec_register(dev, info);
[Severity: High]
Can this asynchronous devm registration leak the auxiliary device?

Because intel_vsec_register() attaches its own devres actions to the parent
device (xe->drm.dev), calling it from a delayed worker can race with the
parent device teardown.

If the worker evaluates the firmware as ready and calls
intel_vsec_register() while the parent's devres list has already been
spliced during unbinding, these new devm actions are left behind on the
empty devres_head and are never executed. This causes a permanent memory
leak of the auxiliary device, and subsequent accesses can lead to a
use-after-free since the parent xe_device has been freed.

> +}
> +
> +static void vsec_disable_late_bind_work(void *arg)
> +{
> +	struct xe_device *xe = arg;
> +
> +	/*
> +	 * If the queued work is canceled, the runtime reference needs to be
> +	 * released here.
> +	 */
> +	if (disable_delayed_work_sync(&xe->pmt.work))
[Severity: High]
Does blocking to cancel the work here open the devres race window mentioned
above?

Since devres_release_all() has already spliced the devres list by the time
this devm action executes, any concurrent worker that bypassed the
cancellation check will attach its new devres actions to the already
spliced parent list.

> +		xe_pm_runtime_put(xe);
[Severity: High]
Could this asynchronous PM put leave the hardware active?

Using xe_pm_runtime_put() (which executes asynchronously) immediately before
device removal allows the pending async idle/suspend work to be cancelled
by the core teardown's pm_runtime_disable().

According to the PM subsystem guidelines, this requires a synchronous put
(like pm_runtime_put_sync()) to ensure the hardware is suspended before
teardown continues.

> +}
[ ... ]
> @@ -477,30 +579,45 @@ void xe_vsec_init(struct xe_device *xe)
[ ... ]
>  	case XE_VSEC_CRI:
>  		if (IS_SRIOV_VF(xe))
> -			return;
> +			return 0;
> +
>  		xe->pmt.base_offset = CRI_PMT_OFFSET;
> +
> +		xe->pmt.retry_count = 0;
> +		INIT_DELAYED_WORK(&xe->pmt.work, cri_late_bind_probe_work);
> +
> +		xe_pm_runtime_get_noresume(xe);
> +		if (!xe_is_oobmsm_fw_ready(xe)) {
> +			schedule_delayed_work(&xe->pmt.work,
> +					      msecs_to_jiffies(VSEC_LATE_BIND_DELAY_MSEC));
[Severity: High]
Is it safe to schedule this work on a non-freezable workqueue without
suspend hooks?

As noted earlier, scheduling this on the default workqueue allows the
worker to execute during system suspend. This permits hardware MMIO
accesses when the device is in a suspended state (D3).

> +			return devm_add_action_or_reset(xe->drm.dev,
> +							vsec_disable_late_bind_work,
> +							xe);
> +		}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260903201002.1064889-20-michael.j.ruhl@intel.com?part=15

RE: [PATCH v5 15/18] drm/xe/vsec: Support late bind fw information

[Ruhl, Michael J]

>-----Original Message-----
>From: sashiko-bot@kernel.org <sashiko-bot@kernel.org>
>Sent: Thursday, September 3, 2026 4:28 PM
>To: Ruhl, Michael J <michael.j.ruhl@intel.com>
>Cc: intel-xe@lists.freedesktop.org
>Subject: Re: [PATCH v5 15/18] drm/xe/vsec: Support late bind fw information
>
>Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to
>consider:
>- [High] Asynchronous `devm` registration race causes memory leak and Use-
>After-Free of the auxiliary device.
>- [High] Asynchronous runtime PM put during devm teardown leaves hardware
>in an incorrect power state.
>- [High] Delayed work accesses hardware MMIO during system suspend.
>--
>
>commit 1b67a780c3fbc619f441f6dfdc210d11c50f6476
>Author: Michael J. Ruhl <michael.j.ruhl@intel.com>
>
>drm/xe/vsec: Support late bind fw information
>
>CRI FW is loaded on power on. Because of this, access to the FW cannot
>be done until it is running. Update the XE PMT probe and access to check
>for late bind devices, verify, and wait for the appropriate FW state
>before probe or access.
>
>> diff --git a/drivers/gpu/drm/xe/xe_vsec.c b/drivers/gpu/drm/xe/xe_vsec.c
>> index 336c4b7d284bc..48cacd23b071e 100644
>> --- a/drivers/gpu/drm/xe/xe_vsec.c
>> +++ b/drivers/gpu/drm/xe/xe_vsec.c
>[ ... ]
>> @@ -281,6 +291,48 @@ static int xe_guid_decode(u32 guid, int *index, u32
>*offset)
>>  	return -ENODEV;
>>  }
>>
>> +static void cri_late_bind_probe_work(struct work_struct *work)
>> +{
>> +	struct xe_device *xe = container_of(work, struct xe_device,
>pmt.work.work);
>> +
>> +	if (xe_is_oobmsm_fw_ready(xe)) {
>[Severity: High]
>Does this code access hardware MMIO during system suspend?

No, a pm_runtime_get is done to ensure availability.

>If a system suspend (e.g., S3) is initiated while the worker is actively
>polling, the device is forced into a suspended state (D3) by the system PM
>core, bypassing the runtime PM usage count held by the worker.
>
>Because the work is on a non-freezable workqueue and not explicitly
>cancelled in the driver's system suspend callbacks, the worker can wake up
>while the hardware is powered down and call xe_is_oobmsm_fw_ready().
>Reading MMIO from a device in the D3 state causes master aborts, unhandled
>PCI errors, or silent 0xFFFFFFFF reads leading to undefined behavior.
>
>> +		cri_late_bind_probe(xe);
>> +		xe_pm_runtime_put(xe);
>> +		return;
>> +	}
>[ ... ]
>> @@ -464,12 +534,44 @@ static enum xe_vsec get_platform_info(struct
>xe_device *xe)
>[ ... ]
>> +static void cri_late_bind_probe(struct xe_device *xe)
>> +{
>[ ... ]
>> +	intel_vsec_register(dev, info);
>[Severity: High]
>Can this asynchronous devm registration leak the auxiliary device?
>
>Because intel_vsec_register() attaches its own devres actions to the parent
>device (xe->drm.dev), calling it from a delayed worker can race with the
>parent device teardown.
>
>If the worker evaluates the firmware as ready and calls
>intel_vsec_register() while the parent's devres list has already been
>spliced during unbinding, these new devm actions are left behind on the
>empty devres_head and are never executed. This causes a permanent memory
>leak of the auxiliary device, and subsequent accesses can lead to a
>use-after-free since the parent xe_device has been freed.
>
>> +}
>> +
>> +static void vsec_disable_late_bind_work(void *arg)
>> +{
>> +	struct xe_device *xe = arg;
>> +
>> +	/*
>> +	 * If the queued work is canceled, the runtime reference needs to be
>> +	 * released here.
>> +	 */
>> +	if (disable_delayed_work_sync(&xe->pmt.work))
>[Severity: High]
>Does blocking to cancel the work here open the devres race window mentioned
>above?
>
>Since devres_release_all() has already spliced the devres list by the time
>this devm action executes, any concurrent worker that bypassed the
>cancellation check will attach its new devres actions to the already
>spliced parent list.
>
>> +		xe_pm_runtime_put(xe);
>[Severity: High]
>Could this asynchronous PM put leave the hardware active?
>
>Using xe_pm_runtime_put() (which executes asynchronously) immediately
>before
>device removal allows the pending async idle/suspend work to be cancelled
>by the core teardown's pm_runtime_disable().
>
>According to the PM subsystem guidelines, this requires a synchronous put
>(like pm_runtime_put_sync()) to ensure the hardware is suspended before
>teardown continues.
>
>> +}
>[ ... ]
>> @@ -477,30 +579,45 @@ void xe_vsec_init(struct xe_device *xe)
>[ ... ]
>>  	case XE_VSEC_CRI:
>>  		if (IS_SRIOV_VF(xe))
>> -			return;
>> +			return 0;
>> +
>>  		xe->pmt.base_offset = CRI_PMT_OFFSET;
>> +
>> +		xe->pmt.retry_count = 0;
>> +		INIT_DELAYED_WORK(&xe->pmt.work,
>cri_late_bind_probe_work);
>> +
>> +		xe_pm_runtime_get_noresume(xe);
>> +		if (!xe_is_oobmsm_fw_ready(xe)) {
>> +			schedule_delayed_work(&xe->pmt.work,
>> +
>msecs_to_jiffies(VSEC_LATE_BIND_DELAY_MSEC));
>[Severity: High]
>Is it safe to schedule this work on a non-freezable workqueue without
>suspend hooks?
>
>As noted earlier, scheduling this on the default workqueue allows the
>worker to execute during system suspend. This permits hardware MMIO
>accesses when the device is in a suspended state (D3).
>
>> +			return devm_add_action_or_reset(xe->drm.dev,
>> +
>	vsec_disable_late_bind_work,
>> +							xe);
>> +		}
>
>--
>Sashiko AI review ·
>https://sashiko.dev/#/patchset/20260903201002.1064889-20-
>michael.j.ruhl@intel.com?part=15

[PATCH v5 16/18] drm/xe/vsec: Add PMT GUID internal access

[Michael J. Ruhl]

Xe accesses the PMT infrastructure directly. The current usage is
supported ONLY by BMG devices.

CRI has further requirements for access.

Add the platform id to the GUID register define and use that for the
register access.

Add an API to allow access the GUID based on the platform.

Signed-off-by: Michael J. Ruhl <michael.j.ruhl@intel.com>
---
 drivers/gpu/drm/xe/regs/xe_pmt.h     |  4 ++-
 drivers/gpu/drm/xe/xe_debugfs.c      |  3 +-
 drivers/gpu/drm/xe/xe_device_types.h |  2 ++
 drivers/gpu/drm/xe/xe_hwmon.c        |  2 +-
 drivers/gpu/drm/xe/xe_pcode.c        |  2 +-
 drivers/gpu/drm/xe/xe_vsec.c         | 44 ++++++++++++++++++++++++++++
 drivers/gpu/drm/xe/xe_vsec.h         |  1 +
 7 files changed, 54 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/xe/regs/xe_pmt.h b/drivers/gpu/drm/xe/regs/xe_pmt.h
index 2f6658395587..fb5b5a8648ba 100644
--- a/drivers/gpu/drm/xe/regs/xe_pmt.h
+++ b/drivers/gpu/drm/xe/regs/xe_pmt.h
@@ -10,7 +10,7 @@
 #define BMG_PMT_BASE_OFFSET		0xDB000
 #define BMG_DISCOVERY_OFFSET		(SOC_BASE + BMG_PMT_BASE_OFFSET)
 
-#define PUNIT_TELEMETRY_GUID		XE_REG(BMG_DISCOVERY_OFFSET + 0x4)
+#define BMG_PUNIT_TELEMETRY_GUID	XE_REG(BMG_DISCOVERY_OFFSET + 0x4)
 #define BMG_ENERGY_STATUS_PMT_OFFSET	(0x30)
 #define	ENERGY_PKG			REG_GENMASK64(31, 0)
 #define	ENERGY_CARD			REG_GENMASK64(63, 32)
@@ -24,6 +24,8 @@
 /* for CRI discovery and telemetry are in an indexed window */
 #define CRI_PMT_OFFSET			(SOC_BASE + XE_PMT_BASE_OFFSET)
 
+#define CRI_PUNIT_TELEMETRY_GUID	XE_REG(CRI_PMT_OFFSET + 0x4)
+
 #define BMG_MODS_RESIDENCY_OFFSET		(0x4D0)
 #define BMG_G2_RESIDENCY_OFFSET		(0x530)
 #define BMG_G6_RESIDENCY_OFFSET		(0x538)
diff --git a/drivers/gpu/drm/xe/xe_debugfs.c b/drivers/gpu/drm/xe/xe_debugfs.c
index 28135f84e286..b01f1e649b51 100644
--- a/drivers/gpu/drm/xe/xe_debugfs.c
+++ b/drivers/gpu/drm/xe/xe_debugfs.c
@@ -101,10 +101,11 @@ static void read_residency_counter(struct xe_device *xe, struct xe_mmio *mmio,
 				   u32 offset, const char *name, struct drm_printer *p)
 {
 	u64 residency = 0;
+
 	int ret;
 
 	ret = xe_pmt_telem_read(xe->drm.dev,
-				xe_mmio_read32(mmio, PUNIT_TELEMETRY_GUID),
+				xe_mmio_read32(mmio, BMG_PUNIT_TELEMETRY_GUID),
 				&residency, offset, sizeof(residency));
 	if (ret != sizeof(residency)) {
 		drm_warn(&xe->drm, "%s counter failed to read, ret %d\n", name, ret);
diff --git a/drivers/gpu/drm/xe/xe_device_types.h b/drivers/gpu/drm/xe/xe_device_types.h
index 69e052ac5a82..2a8c519e1639 100644
--- a/drivers/gpu/drm/xe/xe_device_types.h
+++ b/drivers/gpu/drm/xe/xe_device_types.h
@@ -473,6 +473,8 @@ struct xe_device {
 		struct delayed_work work;
 		/** @pmt.retry_count: late-bind probe retry */
 		u32 retry_count;
+		/** @pmt.punit_guid_cache: cache of the PUINT GUID */
+		u32 punit_guid_cache;
 	} pmt;
 
 	/** @soc_remapper: SoC remapper object */
diff --git a/drivers/gpu/drm/xe/xe_hwmon.c b/drivers/gpu/drm/xe/xe_hwmon.c
index 5284cab6703d..b9740224e07f 100644
--- a/drivers/gpu/drm/xe/xe_hwmon.c
+++ b/drivers/gpu/drm/xe/xe_hwmon.c
@@ -519,7 +519,7 @@ xe_hwmon_energy_get(struct xe_hwmon *hwmon, int channel, long *energy)
 		u64 pmt_val;
 
 		ret = xe_pmt_telem_read(hwmon->xe->drm.dev,
-					xe_mmio_read32(mmio, PUNIT_TELEMETRY_GUID),
+					xe_mmio_read32(mmio, BMG_PUNIT_TELEMETRY_GUID),
 					&pmt_val, BMG_ENERGY_STATUS_PMT_OFFSET,	sizeof(pmt_val));
 		if (ret != sizeof(pmt_val)) {
 			drm_warn(&hwmon->xe->drm, "energy read from pmt failed, ret %d\n", ret);
diff --git a/drivers/gpu/drm/xe/xe_pcode.c b/drivers/gpu/drm/xe/xe_pcode.c
index d502205bb72a..844d8c6face6 100644
--- a/drivers/gpu/drm/xe/xe_pcode.c
+++ b/drivers/gpu/drm/xe/xe_pcode.c
@@ -369,7 +369,7 @@ int xe_get_pcode_version(struct xe_device *xe, struct xe_pcode_version *version)
 	guard(xe_pm_runtime)(xe);
 
 	ret = xe_pmt_telem_read(xe->drm.dev,
-				xe_mmio_read32(xe_root_tile_mmio(xe), PUNIT_TELEMETRY_GUID),
+				xe_mmio_read32(xe_root_tile_mmio(xe), BMG_PUNIT_TELEMETRY_GUID),
 				(u64 *)version, PUNIT_VERSION_OFFSET, sizeof(*version));
 	if (ret != sizeof(*version)) {
 		xe_warn(xe, "pcode version read from PMT failed, ret %pe\n", ERR_PTR(ret));
diff --git a/drivers/gpu/drm/xe/xe_vsec.c b/drivers/gpu/drm/xe/xe_vsec.c
index 48cacd23b071..896dfb011baf 100644
--- a/drivers/gpu/drm/xe/xe_vsec.c
+++ b/drivers/gpu/drm/xe/xe_vsec.c
@@ -566,6 +566,50 @@ static void vsec_disable_late_bind_work(void *arg)
 		xe_pm_runtime_put(xe);
 }
 
+int xe_vsec_get_guid(struct xe_device *xe, u32 *guid)
+{
+	struct xe_mmio *mmio = xe_root_tile_mmio(xe);
+	int ret = 0;
+
+	if (IS_SRIOV_VF(xe))
+		return -EINVAL;
+
+	if (!xe_pm_runtime_get_if_active(xe))
+		return -ENODATA;
+
+	mutex_lock(&xe->pmt.lock);
+
+	if (xe->pmt.punit_guid_cache) {
+		*guid = xe->pmt.punit_guid_cache;
+		goto unlock;
+	}
+
+	switch (xe->info.platform) {
+	case XE_BATTLEMAGE:
+		*guid = xe_mmio_read32(mmio, BMG_PUNIT_TELEMETRY_GUID);
+		break;
+
+	case XE_CRESCENTISLAND:
+		xe->soc_remapper.set_telem_region(xe, CRI_IDX_TELEM_DISCOVERY);
+		*guid = xe_mmio_read32(mmio, CRI_PUNIT_TELEMETRY_GUID);
+		break;
+
+	default:
+		*guid = 0;
+		drm_err(&xe->drm, "Unsupported platform: %u\n", xe->info.platform);
+		ret = -EINVAL;
+		break;
+	}
+
+	xe->pmt.punit_guid_cache = *guid;
+
+unlock:
+	mutex_unlock(&xe->pmt.lock);
+	xe_pm_runtime_put(xe);
+
+	return ret;
+}
+
 /**
  * xe_vsec_init - Initialize resources and add intel_vsec auxiliary
  * interface
diff --git a/drivers/gpu/drm/xe/xe_vsec.h b/drivers/gpu/drm/xe/xe_vsec.h
index c4a1e2fc67d8..50187fdc1207 100644
--- a/drivers/gpu/drm/xe/xe_vsec.h
+++ b/drivers/gpu/drm/xe/xe_vsec.h
@@ -10,6 +10,7 @@ struct device;
 struct xe_device;
 
 int xe_vsec_init(struct xe_device *xe);
+int xe_vsec_get_guid(struct xe_device *xe, u32 *guid);
 int xe_pmt_telem_read(struct device *dev, u32 guid, u64 *data, loff_t user_offset, u32 count);
 
 #endif
-- 
2.43.0

Re: [PATCH v5 16/18] drm/xe/vsec: Add PMT GUID internal access

[Poosa, Karthik]

On 04-09-2026 01:40, Michael J. Ruhl wrote:
> Xe accesses the PMT infrastructure directly. The current usage is
> supported ONLY by BMG devices.
>
> CRI has further requirements for access.
>
> Add the platform id to the GUID register define and use that for the
> register access.
>
> Add an API to allow access the GUID based on the platform.
>
> Signed-off-by: Michael J. Ruhl <michael.j.ruhl@intel.com>
> ---
>   drivers/gpu/drm/xe/regs/xe_pmt.h     |  4 ++-
>   drivers/gpu/drm/xe/xe_debugfs.c      |  3 +-
>   drivers/gpu/drm/xe/xe_device_types.h |  2 ++
>   drivers/gpu/drm/xe/xe_hwmon.c        |  2 +-
>   drivers/gpu/drm/xe/xe_pcode.c        |  2 +-
>   drivers/gpu/drm/xe/xe_vsec.c         | 44 ++++++++++++++++++++++++++++
>   drivers/gpu/drm/xe/xe_vsec.h         |  1 +
>   7 files changed, 54 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/xe/regs/xe_pmt.h b/drivers/gpu/drm/xe/regs/xe_pmt.h
> index 2f6658395587..fb5b5a8648ba 100644
> --- a/drivers/gpu/drm/xe/regs/xe_pmt.h
> +++ b/drivers/gpu/drm/xe/regs/xe_pmt.h
> @@ -10,7 +10,7 @@
>   #define BMG_PMT_BASE_OFFSET		0xDB000
>   #define BMG_DISCOVERY_OFFSET		(SOC_BASE + BMG_PMT_BASE_OFFSET)
>   
> -#define PUNIT_TELEMETRY_GUID		XE_REG(BMG_DISCOVERY_OFFSET + 0x4)
> +#define BMG_PUNIT_TELEMETRY_GUID	XE_REG(BMG_DISCOVERY_OFFSET + 0x4)
>   #define BMG_ENERGY_STATUS_PMT_OFFSET	(0x30)
>   #define	ENERGY_PKG			REG_GENMASK64(31, 0)
>   #define	ENERGY_CARD			REG_GENMASK64(63, 32)
> @@ -24,6 +24,8 @@
>   /* for CRI discovery and telemetry are in an indexed window */
>   #define CRI_PMT_OFFSET			(SOC_BASE + XE_PMT_BASE_OFFSET)
>   
> +#define CRI_PUNIT_TELEMETRY_GUID	XE_REG(CRI_PMT_OFFSET + 0x4)
> +
>   #define BMG_MODS_RESIDENCY_OFFSET		(0x4D0)
>   #define BMG_G2_RESIDENCY_OFFSET		(0x530)
>   #define BMG_G6_RESIDENCY_OFFSET		(0x538)
> diff --git a/drivers/gpu/drm/xe/xe_debugfs.c b/drivers/gpu/drm/xe/xe_debugfs.c
> index 28135f84e286..b01f1e649b51 100644
> --- a/drivers/gpu/drm/xe/xe_debugfs.c
> +++ b/drivers/gpu/drm/xe/xe_debugfs.c
> @@ -101,10 +101,11 @@ static void read_residency_counter(struct xe_device *xe, struct xe_mmio *mmio,
>   				   u32 offset, const char *name, struct drm_printer *p)
>   {
>   	u64 residency = 0;
> +
this can be removed
>   	int ret;
>   
>   	ret = xe_pmt_telem_read(xe->drm.dev,
> -				xe_mmio_read32(mmio, PUNIT_TELEMETRY_GUID),
> +				xe_mmio_read32(mmio, BMG_PUNIT_TELEMETRY_GUID),
>   				&residency, offset, sizeof(residency));
>   	if (ret != sizeof(residency)) {
>   		drm_warn(&xe->drm, "%s counter failed to read, ret %d\n", name, ret);
> diff --git a/drivers/gpu/drm/xe/xe_device_types.h b/drivers/gpu/drm/xe/xe_device_types.h
> index 69e052ac5a82..2a8c519e1639 100644
> --- a/drivers/gpu/drm/xe/xe_device_types.h
> +++ b/drivers/gpu/drm/xe/xe_device_types.h
> @@ -473,6 +473,8 @@ struct xe_device {
>   		struct delayed_work work;
>   		/** @pmt.retry_count: late-bind probe retry */
>   		u32 retry_count;
> +		/** @pmt.punit_guid_cache: cache of the PUINT GUID */
> +		u32 punit_guid_cache;
>   	} pmt;
>   
>   	/** @soc_remapper: SoC remapper object */
> diff --git a/drivers/gpu/drm/xe/xe_hwmon.c b/drivers/gpu/drm/xe/xe_hwmon.c
> index 5284cab6703d..b9740224e07f 100644
> --- a/drivers/gpu/drm/xe/xe_hwmon.c
> +++ b/drivers/gpu/drm/xe/xe_hwmon.c
> @@ -519,7 +519,7 @@ xe_hwmon_energy_get(struct xe_hwmon *hwmon, int channel, long *energy)
>   		u64 pmt_val;
>   
>   		ret = xe_pmt_telem_read(hwmon->xe->drm.dev,
> -					xe_mmio_read32(mmio, PUNIT_TELEMETRY_GUID),
> +					xe_mmio_read32(mmio, BMG_PUNIT_TELEMETRY_GUID),
>   					&pmt_val, BMG_ENERGY_STATUS_PMT_OFFSET,	sizeof(pmt_val));
>   		if (ret != sizeof(pmt_val)) {
>   			drm_warn(&hwmon->xe->drm, "energy read from pmt failed, ret %d\n", ret);
> diff --git a/drivers/gpu/drm/xe/xe_pcode.c b/drivers/gpu/drm/xe/xe_pcode.c
> index d502205bb72a..844d8c6face6 100644
> --- a/drivers/gpu/drm/xe/xe_pcode.c
> +++ b/drivers/gpu/drm/xe/xe_pcode.c
> @@ -369,7 +369,7 @@ int xe_get_pcode_version(struct xe_device *xe, struct xe_pcode_version *version)
>   	guard(xe_pm_runtime)(xe);
>   
>   	ret = xe_pmt_telem_read(xe->drm.dev,
> -				xe_mmio_read32(xe_root_tile_mmio(xe), PUNIT_TELEMETRY_GUID),
> +				xe_mmio_read32(xe_root_tile_mmio(xe), BMG_PUNIT_TELEMETRY_GUID),
>   				(u64 *)version, PUNIT_VERSION_OFFSET, sizeof(*version));
>   	if (ret != sizeof(*version)) {
>   		xe_warn(xe, "pcode version read from PMT failed, ret %pe\n", ERR_PTR(ret));
> diff --git a/drivers/gpu/drm/xe/xe_vsec.c b/drivers/gpu/drm/xe/xe_vsec.c
> index 48cacd23b071..896dfb011baf 100644
> --- a/drivers/gpu/drm/xe/xe_vsec.c
> +++ b/drivers/gpu/drm/xe/xe_vsec.c
> @@ -566,6 +566,50 @@ static void vsec_disable_late_bind_work(void *arg)
>   		xe_pm_runtime_put(xe);
>   }
>   
> +int xe_vsec_get_guid(struct xe_device *xe, u32 *guid)
> +{
> +	struct xe_mmio *mmio = xe_root_tile_mmio(xe);
> +	int ret = 0;
> +
> +	if (IS_SRIOV_VF(xe))
> +		return -EINVAL;
> +
> +	if (!xe_pm_runtime_get_if_active(xe))
> +		return -ENODATA;
> +
> +	mutex_lock(&xe->pmt.lock);
> +
> +	if (xe->pmt.punit_guid_cache) {
> +		*guid = xe->pmt.punit_guid_cache;
> +		goto unlock;
> +	}
can you add a comment here that guid doesn't change after first read.
> +
> +	switch (xe->info.platform) {
> +	case XE_BATTLEMAGE:
> +		*guid = xe_mmio_read32(mmio, BMG_PUNIT_TELEMETRY_GUID);
> +		break;
> +
> +	case XE_CRESCENTISLAND:
> +		xe->soc_remapper.set_telem_region(xe, CRI_IDX_TELEM_DISCOVERY);
> +		*guid = xe_mmio_read32(mmio, CRI_PUNIT_TELEMETRY_GUID);
> +		break;
> +
> +	default:
> +		*guid = 0;
> +		drm_err(&xe->drm, "Unsupported platform: %u\n", xe->info.platform);
> +		ret = -EINVAL;
> +		break;
> +	}
> +
> +	xe->pmt.punit_guid_cache = *guid;
you can set cache only when guid is valid

if(*guid)

    xe->pmt.punit_guid_cache = *guid;

> +
> +unlock:
> +	mutex_unlock(&xe->pmt.lock);
> +	xe_pm_runtime_put(xe);
> +
> +	return ret;
> +}
> +
>   /**
>    * xe_vsec_init - Initialize resources and add intel_vsec auxiliary
>    * interface
> diff --git a/drivers/gpu/drm/xe/xe_vsec.h b/drivers/gpu/drm/xe/xe_vsec.h
> index c4a1e2fc67d8..50187fdc1207 100644
> --- a/drivers/gpu/drm/xe/xe_vsec.h
> +++ b/drivers/gpu/drm/xe/xe_vsec.h
> @@ -10,6 +10,7 @@ struct device;
>   struct xe_device;
>   
>   int xe_vsec_init(struct xe_device *xe);
> +int xe_vsec_get_guid(struct xe_device *xe, u32 *guid);
>   int xe_pmt_telem_read(struct device *dev, u32 guid, u64 *data, loff_t user_offset, u32 count);
>   
>   #endif

RE: [PATCH v5 16/18] drm/xe/vsec: Add PMT GUID internal access

[Ruhl, Michael J]

>-----Original Message-----
>From: Poosa, Karthik <karthik.poosa@intel.com>
>Sent: Friday, September 4, 2026 11:15 AM
>To: Ruhl, Michael J <michael.j.ruhl@intel.com>; platform-driver-
>x86@vger.kernel.org; intel-xe@lists.freedesktop.org; hansg@kernel.org;
>ilpo.jarvinen@linux.intel.com; Brost, Matthew <matthew.brost@intel.com>;
>Vivi, Rodrigo <rodrigo.vivi@intel.com>; thomas.hellstrom@linux.intel.com;
>airlied@gmail.com; simona@ffwll.ch; david.e.box@linux.intel.com; Vijay,
>Anoop C <anoop.c.vijay@intel.com>; Nilawar, Badal
><badal.nilawar@intel.com>; Roper, Matthew D <matthew.d.roper@intel.com>;
>Ausmus, James <james.ausmus@intel.com>
>Subject: Re: [PATCH v5 16/18] drm/xe/vsec: Add PMT GUID internal access
>
>
>On 04-09-2026 01:40, Michael J. Ruhl wrote:
>> Xe accesses the PMT infrastructure directly. The current usage is
>> supported ONLY by BMG devices.
>>
>> CRI has further requirements for access.
>>
>> Add the platform id to the GUID register define and use that for the
>> register access.
>>
>> Add an API to allow access the GUID based on the platform.
>>
>> Signed-off-by: Michael J. Ruhl <michael.j.ruhl@intel.com>
>> ---
>>   drivers/gpu/drm/xe/regs/xe_pmt.h     |  4 ++-
>>   drivers/gpu/drm/xe/xe_debugfs.c      |  3 +-
>>   drivers/gpu/drm/xe/xe_device_types.h |  2 ++
>>   drivers/gpu/drm/xe/xe_hwmon.c        |  2 +-
>>   drivers/gpu/drm/xe/xe_pcode.c        |  2 +-
>>   drivers/gpu/drm/xe/xe_vsec.c         | 44
>++++++++++++++++++++++++++++
>>   drivers/gpu/drm/xe/xe_vsec.h         |  1 +
>>   7 files changed, 54 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/xe/regs/xe_pmt.h
>b/drivers/gpu/drm/xe/regs/xe_pmt.h
>> index 2f6658395587..fb5b5a8648ba 100644
>> --- a/drivers/gpu/drm/xe/regs/xe_pmt.h
>> +++ b/drivers/gpu/drm/xe/regs/xe_pmt.h
>> @@ -10,7 +10,7 @@
>>   #define BMG_PMT_BASE_OFFSET		0xDB000
>>   #define BMG_DISCOVERY_OFFSET		(SOC_BASE +
>BMG_PMT_BASE_OFFSET)
>>
>> -#define PUNIT_TELEMETRY_GUID
>	XE_REG(BMG_DISCOVERY_OFFSET + 0x4)
>> +#define BMG_PUNIT_TELEMETRY_GUID
>	XE_REG(BMG_DISCOVERY_OFFSET + 0x4)
>>   #define BMG_ENERGY_STATUS_PMT_OFFSET	(0x30)
>>   #define	ENERGY_PKG			REG_GENMASK64(31, 0)
>>   #define	ENERGY_CARD			REG_GENMASK64(63, 32)
>> @@ -24,6 +24,8 @@
>>   /* for CRI discovery and telemetry are in an indexed window */
>>   #define CRI_PMT_OFFSET			(SOC_BASE +
>XE_PMT_BASE_OFFSET)
>>
>> +#define CRI_PUNIT_TELEMETRY_GUID	XE_REG(CRI_PMT_OFFSET +
>0x4)
>> +
>>   #define BMG_MODS_RESIDENCY_OFFSET		(0x4D0)
>>   #define BMG_G2_RESIDENCY_OFFSET		(0x530)
>>   #define BMG_G6_RESIDENCY_OFFSET		(0x538)
>> diff --git a/drivers/gpu/drm/xe/xe_debugfs.c
>b/drivers/gpu/drm/xe/xe_debugfs.c
>> index 28135f84e286..b01f1e649b51 100644
>> --- a/drivers/gpu/drm/xe/xe_debugfs.c
>> +++ b/drivers/gpu/drm/xe/xe_debugfs.c
>> @@ -101,10 +101,11 @@ static void read_residency_counter(struct
>xe_device *xe, struct xe_mmio *mmio,
>>   				   u32 offset, const char *name, struct
>drm_printer *p)
>>   {
>>   	u64 residency = 0;
>> +
>this can be removed

will fix.


>>   	int ret;
>>
>>   	ret = xe_pmt_telem_read(xe->drm.dev,
>> -				xe_mmio_read32(mmio,
>PUNIT_TELEMETRY_GUID),
>> +				xe_mmio_read32(mmio,
>BMG_PUNIT_TELEMETRY_GUID),
>>   				&residency, offset, sizeof(residency));
>>   	if (ret != sizeof(residency)) {
>>   		drm_warn(&xe->drm, "%s counter failed to read, ret %d\n",
>name, ret);
>> diff --git a/drivers/gpu/drm/xe/xe_device_types.h
>b/drivers/gpu/drm/xe/xe_device_types.h
>> index 69e052ac5a82..2a8c519e1639 100644
>> --- a/drivers/gpu/drm/xe/xe_device_types.h
>> +++ b/drivers/gpu/drm/xe/xe_device_types.h
>> @@ -473,6 +473,8 @@ struct xe_device {
>>   		struct delayed_work work;
>>   		/** @pmt.retry_count: late-bind probe retry */
>>   		u32 retry_count;
>> +		/** @pmt.punit_guid_cache: cache of the PUINT GUID */
>> +		u32 punit_guid_cache;
>>   	} pmt;
>>
>>   	/** @soc_remapper: SoC remapper object */
>> diff --git a/drivers/gpu/drm/xe/xe_hwmon.c
>b/drivers/gpu/drm/xe/xe_hwmon.c
>> index 5284cab6703d..b9740224e07f 100644
>> --- a/drivers/gpu/drm/xe/xe_hwmon.c
>> +++ b/drivers/gpu/drm/xe/xe_hwmon.c
>> @@ -519,7 +519,7 @@ xe_hwmon_energy_get(struct xe_hwmon *hwmon,
>int channel, long *energy)
>>   		u64 pmt_val;
>>
>>   		ret = xe_pmt_telem_read(hwmon->xe->drm.dev,
>> -					xe_mmio_read32(mmio,
>PUNIT_TELEMETRY_GUID),
>> +					xe_mmio_read32(mmio,
>BMG_PUNIT_TELEMETRY_GUID),
>>   					&pmt_val,
>BMG_ENERGY_STATUS_PMT_OFFSET,	sizeof(pmt_val));
>>   		if (ret != sizeof(pmt_val)) {
>>   			drm_warn(&hwmon->xe->drm, "energy read from pmt
>failed, ret %d\n", ret);
>> diff --git a/drivers/gpu/drm/xe/xe_pcode.c
>b/drivers/gpu/drm/xe/xe_pcode.c
>> index d502205bb72a..844d8c6face6 100644
>> --- a/drivers/gpu/drm/xe/xe_pcode.c
>> +++ b/drivers/gpu/drm/xe/xe_pcode.c
>> @@ -369,7 +369,7 @@ int xe_get_pcode_version(struct xe_device *xe,
>struct xe_pcode_version *version)
>>   	guard(xe_pm_runtime)(xe);
>>
>>   	ret = xe_pmt_telem_read(xe->drm.dev,
>> -				xe_mmio_read32(xe_root_tile_mmio(xe),
>PUNIT_TELEMETRY_GUID),
>> +				xe_mmio_read32(xe_root_tile_mmio(xe),
>BMG_PUNIT_TELEMETRY_GUID),
>>   				(u64 *)version, PUNIT_VERSION_OFFSET,
>sizeof(*version));
>>   	if (ret != sizeof(*version)) {
>>   		xe_warn(xe, "pcode version read from PMT failed, ret %pe\n",
>ERR_PTR(ret));
>> diff --git a/drivers/gpu/drm/xe/xe_vsec.c b/drivers/gpu/drm/xe/xe_vsec.c
>> index 48cacd23b071..896dfb011baf 100644
>> --- a/drivers/gpu/drm/xe/xe_vsec.c
>> +++ b/drivers/gpu/drm/xe/xe_vsec.c
>> @@ -566,6 +566,50 @@ static void vsec_disable_late_bind_work(void *arg)
>>   		xe_pm_runtime_put(xe);
>>   }
>>
>> +int xe_vsec_get_guid(struct xe_device *xe, u32 *guid)
>> +{
>> +	struct xe_mmio *mmio = xe_root_tile_mmio(xe);
>> +	int ret = 0;
>> +
>> +	if (IS_SRIOV_VF(xe))
>> +		return -EINVAL;
>> +
>> +	if (!xe_pm_runtime_get_if_active(xe))
>> +		return -ENODATA;
>> +
>> +	mutex_lock(&xe->pmt.lock);
>> +
>> +	if (xe->pmt.punit_guid_cache) {
>> +		*guid = xe->pmt.punit_guid_cache;
>> +		goto unlock;
>> +	}
>can you add a comment here that guid doesn't change after first read.
>> +
>> +	switch (xe->info.platform) {
>> +	case XE_BATTLEMAGE:
>> +		*guid = xe_mmio_read32(mmio,
>BMG_PUNIT_TELEMETRY_GUID);
>> +		break;
>> +
>> +	case XE_CRESCENTISLAND:
>> +		xe->soc_remapper.set_telem_region(xe,
>CRI_IDX_TELEM_DISCOVERY);
>> +		*guid = xe_mmio_read32(mmio,
>CRI_PUNIT_TELEMETRY_GUID);
>> +		break;
>> +
>> +	default:
>> +		*guid = 0;
>> +		drm_err(&xe->drm, "Unsupported platform: %u\n", xe-
>>info.platform);
>> +		ret = -EINVAL;
>> +		break;
>> +	}
>> +
>> +	xe->pmt.punit_guid_cache = *guid;
>you can set cache only when guid is valid
>
>if(*guid)
>
>    xe->pmt.punit_guid_cache = *guid;

I did ponder that... and decided that setting the cache with "invalid" was reasonable...
(i.e. always set the cache)...

I will do the above.

m


>> +
>> +unlock:
>> +	mutex_unlock(&xe->pmt.lock);
>> +	xe_pm_runtime_put(xe);
>> +
>> +	return ret;
>> +}
>> +
>>   /**
>>    * xe_vsec_init - Initialize resources and add intel_vsec auxiliary
>>    * interface
>> diff --git a/drivers/gpu/drm/xe/xe_vsec.h b/drivers/gpu/drm/xe/xe_vsec.h
>> index c4a1e2fc67d8..50187fdc1207 100644
>> --- a/drivers/gpu/drm/xe/xe_vsec.h
>> +++ b/drivers/gpu/drm/xe/xe_vsec.h
>> @@ -10,6 +10,7 @@ struct device;
>>   struct xe_device;
>>
>>   int xe_vsec_init(struct xe_device *xe);
>> +int xe_vsec_get_guid(struct xe_device *xe, u32 *guid);
>>   int xe_pmt_telem_read(struct device *dev, u32 guid, u64 *data, loff_t
>user_offset, u32 count);
>>
>>   #endif

[PATCH v5 17/18] drm/xe/vsec: Update PMT internal access

[Michael J. Ruhl]

Current PMT internal access assumes BMG devices.

Use the new PMT GUID API to read the device specific guid.

Minor cleanup for a newly unused parameter (mmio).

Reviewed-by: Karthik Poosa <karthik.poosa@intel.com>
Signed-off-by: Michael J. Ruhl <michael.j.ruhl@intel.com>
---
 drivers/gpu/drm/xe/xe_debugfs.c | 27 ++++++++++++++-------------
 drivers/gpu/drm/xe/xe_hwmon.c   | 10 ++++++++--
 drivers/gpu/drm/xe/xe_pcode.c   | 10 ++++++++--
 3 files changed, 30 insertions(+), 17 deletions(-)

diff --git a/drivers/gpu/drm/xe/xe_debugfs.c b/drivers/gpu/drm/xe/xe_debugfs.c
index b01f1e649b51..f17b993755f3 100644
--- a/drivers/gpu/drm/xe/xe_debugfs.c
+++ b/drivers/gpu/drm/xe/xe_debugfs.c
@@ -5,6 +5,7 @@
 
 #include "xe_debugfs.h"
 
+#include <linux/array_size.h>
 #include <linux/bits.h>
 #include <linux/debugfs.h>
 #include <linux/fault-inject.h>
@@ -21,7 +22,6 @@
 #include "xe_gt_printk.h"
 #include "xe_guc_ads.h"
 #include "xe_hw_engine.h"
-#include "xe_mmio.h"
 #include "xe_pagefault.h"
 #include "xe_pcode.h"
 #include "xe_pm.h"
@@ -97,16 +97,20 @@ static void xe_fault_inject_debugfs_register(struct xe_device *xe,
 	}
 }
 
-static void read_residency_counter(struct xe_device *xe, struct xe_mmio *mmio,
-				   u32 offset, const char *name, struct drm_printer *p)
+static void read_residency_counter(struct xe_device *xe, u32 offset, const char *name,
+				   struct drm_printer *p)
 {
 	u64 residency = 0;
-
+	u32 guid;
 	int ret;
 
-	ret = xe_pmt_telem_read(xe->drm.dev,
-				xe_mmio_read32(mmio, BMG_PUNIT_TELEMETRY_GUID),
-				&residency, offset, sizeof(residency));
+	ret = xe_vsec_get_guid(xe, &guid);
+	if (ret) {
+		drm_warn(&xe->drm, "PMT device is not available: %d\n", ret);
+		return;
+	}
+
+	ret = xe_pmt_telem_read(xe->drm.dev, guid, &residency, offset, sizeof(residency));
 	if (ret != sizeof(residency)) {
 		drm_warn(&xe->drm, "%s counter failed to read, ret %d\n", name, ret);
 		return;
@@ -245,13 +249,12 @@ static int pcode_info(struct seq_file *m, void *data)
 static int dgfx_pkg_residencies_show(struct seq_file *m, void *data)
 {
 	struct xe_device *xe;
-	struct xe_mmio *mmio;
 	struct drm_printer p;
 
 	xe = node_to_xe(m->private);
 	p = drm_seq_file_printer(m);
 	guard(xe_pm_runtime)(xe);
-	mmio = xe_root_tile_mmio(xe);
+
 	static const struct {
 		u32 offset;
 		const char *name;
@@ -265,7 +268,7 @@ static int dgfx_pkg_residencies_show(struct seq_file *m, void *data)
 	};
 
 	for (int i = 0; i < ARRAY_SIZE(residencies); i++)
-		read_residency_counter(xe, mmio, residencies[i].offset, residencies[i].name, &p);
+		read_residency_counter(xe, residencies[i].offset, residencies[i].name, &p);
 
 	return 0;
 }
@@ -273,13 +276,11 @@ static int dgfx_pkg_residencies_show(struct seq_file *m, void *data)
 static int dgfx_pcie_link_residencies_show(struct seq_file *m, void *data)
 {
 	struct xe_device *xe;
-	struct xe_mmio *mmio;
 	struct drm_printer p;
 
 	xe = node_to_xe(m->private);
 	p = drm_seq_file_printer(m);
 	guard(xe_pm_runtime)(xe);
-	mmio = xe_root_tile_mmio(xe);
 
 	static const struct {
 		u32 offset;
@@ -291,7 +292,7 @@ static int dgfx_pcie_link_residencies_show(struct seq_file *m, void *data)
 	};
 
 	for (int i = 0; i < ARRAY_SIZE(residencies); i++)
-		read_residency_counter(xe, mmio, residencies[i].offset, residencies[i].name, &p);
+		read_residency_counter(xe, residencies[i].offset, residencies[i].name, &p);
 
 	return 0;
 }
diff --git a/drivers/gpu/drm/xe/xe_hwmon.c b/drivers/gpu/drm/xe/xe_hwmon.c
index b9740224e07f..c44a7d24816d 100644
--- a/drivers/gpu/drm/xe/xe_hwmon.c
+++ b/drivers/gpu/drm/xe/xe_hwmon.c
@@ -517,9 +517,15 @@ xe_hwmon_energy_get(struct xe_hwmon *hwmon, int channel, long *energy)
 
 	if (hwmon->xe->info.platform == XE_BATTLEMAGE) {
 		u64 pmt_val;
+		u32 guid;
 
-		ret = xe_pmt_telem_read(hwmon->xe->drm.dev,
-					xe_mmio_read32(mmio, BMG_PUNIT_TELEMETRY_GUID),
+		ret = xe_vsec_get_guid(hwmon->xe, &guid);
+		if (ret) {
+			drm_warn(&hwmon->xe->drm, "PMT device is not available: %d\n", ret);
+			*energy = 0;
+			return;
+		}
+		ret = xe_pmt_telem_read(hwmon->xe->drm.dev, guid,
 					&pmt_val, BMG_ENERGY_STATUS_PMT_OFFSET,	sizeof(pmt_val));
 		if (ret != sizeof(pmt_val)) {
 			drm_warn(&hwmon->xe->drm, "energy read from pmt failed, ret %d\n", ret);
diff --git a/drivers/gpu/drm/xe/xe_pcode.c b/drivers/gpu/drm/xe/xe_pcode.c
index 844d8c6face6..e534d64834e3 100644
--- a/drivers/gpu/drm/xe/xe_pcode.c
+++ b/drivers/gpu/drm/xe/xe_pcode.c
@@ -365,11 +365,17 @@ ALLOW_ERROR_INJECTION(xe_pcode_probe_early, ERRNO); /* See xe_pci_probe */
 int xe_get_pcode_version(struct xe_device *xe, struct xe_pcode_version *version)
 {
 	int ret = 0;
+	u32 guid;
 
 	guard(xe_pm_runtime)(xe);
 
-	ret = xe_pmt_telem_read(xe->drm.dev,
-				xe_mmio_read32(xe_root_tile_mmio(xe), BMG_PUNIT_TELEMETRY_GUID),
+	ret = xe_vsec_get_guid(xe, &guid);
+	if (ret) {
+		xe_warn(xe, "PMT device is not available: %d\n", ret);
+		return ret;
+	}
+
+	ret = xe_pmt_telem_read(xe->drm.dev, guid,
 				(u64 *)version, PUNIT_VERSION_OFFSET, sizeof(*version));
 	if (ret != sizeof(*version)) {
 		xe_warn(xe, "pcode version read from PMT failed, ret %pe\n", ERR_PTR(ret));
-- 
2.43.0

[PATCH v5 18/18] drm/xe/vsec: Refactor platform check

[Michael J. Ruhl]

For debugfs, residencies and pcode_info are available on the BMG and
CRI (!SRIOV_VF) platforms.

Refactor to have one code path for adding the debugfs entries.

Reviewed-by: Karthik Poosa <karthik.poosa@intel.com>
Signed-off-by: Michael J. Ruhl <michael.j.ruhl@intel.com>
---
 drivers/gpu/drm/xe/xe_debugfs.c | 19 +++++++++----------
 1 file changed, 9 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/xe/xe_debugfs.c b/drivers/gpu/drm/xe/xe_debugfs.c
index f17b993755f3..9e32a5e230b9 100644
--- a/drivers/gpu/drm/xe/xe_debugfs.c
+++ b/drivers/gpu/drm/xe/xe_debugfs.c
@@ -712,23 +712,22 @@ void xe_debugfs_register(struct xe_device *xe)
 				 ARRAY_SIZE(debugfs_list),
 				 root, minor);
 
-	if (xe->info.platform == XE_BATTLEMAGE && !IS_SRIOV_VF(xe)) {
-		drm_debugfs_create_files(debugfs_residencies,
-					 ARRAY_SIZE(debugfs_residencies),
-					 root, minor);
-	}
-
 	/*
-	 * Pcode version read from PMT is currently only supported on CRI and BMG platforms in PF
-	 * mode, as both platforms support the necessary telemetry read mechanism and have a fixed
-	 * PUNIT_VERSION_OFFSET.
+	 * Residencies and Pcode version read from PMT is currently only supported on CRI and BMG
+	 * platforms in PF mode.  Both platforms support the necessary telemetry read mechanism
+	 * and have a fixed offsets for the required data.
 	 * Attempting this access on other platforms must be verified before enabling support.
 	 */
 	if (!IS_SRIOV_VF(xe) &&
-	    (xe->info.platform == XE_CRESCENTISLAND || xe->info.platform == XE_BATTLEMAGE))
+	    (xe->info.platform == XE_CRESCENTISLAND || xe->info.platform == XE_BATTLEMAGE)) {
+		drm_debugfs_create_files(debugfs_residencies,
+					 ARRAY_SIZE(debugfs_residencies),
+					 root, minor);
+
 		drm_debugfs_create_files(pcode_info_debugfs,
 					 ARRAY_SIZE(pcode_info_debugfs),
 					 root, minor);
+	}
 
 	debugfs_create_file("forcewake_all", 0400, root, xe,
 			    &forcewake_all_fops);
-- 
2.43.0

✗ CI.checkpatch: warning for Crescent Island PMT support (rev7)

[Patchwork]

== Series Details ==

Series: Crescent Island PMT support (rev7)
URL   : https://patchwork.freedesktop.org/series/160717/
State : warning

== Summary ==

+ KERNEL=/kernel
+ git clone https://gitlab.freedesktop.org/drm/maintainer-tools mt
Cloning into 'mt'...
warning: redirecting to https://gitlab.freedesktop.org/drm/maintainer-tools.git/
+ git -C mt rev-list -n1 origin/master
d875049d2b299159a272bd5151994970cdcd1e31
+ cd /kernel
+ git config --global --add safe.directory /kernel
+ git log -n1
commit 8278f4900eac36efc2d5a60149ed7e72cd4061d7
Author: Michael J. Ruhl <michael.j.ruhl@intel.com>
Date:   Thu Sep 3 13:10:20 2026 -0700

    drm/xe/vsec: Refactor platform check
    
    For debugfs, residencies and pcode_info are available on the BMG and
    CRI (!SRIOV_VF) platforms.
    
    Refactor to have one code path for adding the debugfs entries.
    
    Reviewed-by: Karthik Poosa <karthik.poosa@intel.com>
    Signed-off-by: Michael J. Ruhl <michael.j.ruhl@intel.com>
+ /mt/dim checkpatch 96ca9b9870504c4a80addae1da9454c8c704a826 drm-intel
f37635015519 platform/x86/intel/pmt: complete pcidev to device update
c2b97bbedf27 platform/x86/intel/pmt: refactor rmw with a return value
eee92e0e4417 platform/x86/intel/pmt: refactor rc with a return value
cc5aa3b45f06 platform/x86/intel/pmt: Add register access callbacks
82c143c7acab platform/x86/intel/pmt: Do not remap when using callbacks
aace8bd9583b drm/xe/vsec: Do not register BMG PMT for VF
24cf215e9fb7 drm/xe/vsec: Correct locking order
f5278c7a0f26 drm/xe/vsec: Use correct pm state get
f65ba9872e9c drm/xe/vsec: Add DOC text for VSEC
-:11: WARNING:FILE_PATH_CHANGES: added, moved or deleted file(s), does MAINTAINERS need updating?
#11: 
new file mode 100644

total: 0 errors, 1 warnings, 0 checks, 48 lines checked
944be78c1a0e drm/xe/vsec: Support possible hotplug exit
520d52cab183 drm/xe/vsec: Support Crescent Island PMT
e7c44bc88fec drm/xe/vsec: Refactor BattleMage PMT defines
2fab9b493302 drm/xe/vsec: Crescent Island PMT decode
bbf9a637b184 drm/xe/vsec: Crescent Island PMT callbacks
4e1d2d5cb911 drm/xe/vsec: Support late bind fw information
fdfd0e06a8cc drm/xe/vsec: Add PMT GUID internal access
dd36a0b2646b drm/xe/vsec: Update PMT internal access
8278f4900eac drm/xe/vsec: Refactor platform check

✗ CI.KUnit: failure for Crescent Island PMT support (rev7)

[Patchwork]

== Series Details ==

Series: Crescent Island PMT support (rev7)
URL   : https://patchwork.freedesktop.org/series/160717/
State : failure

== Summary ==

+ trap cleanup EXIT
+ kunitconfigs=('/kernel/drivers/gpu/tests/.kunitconfig' '/kernel/drivers/gpu/drm/xe/.kunitconfig' '/kernel/drivers/gpu/drm/tests/.kunitconfig' '/kernel/drivers/gpu/drm/ttm/tests/.kunitconfig' '/kernel/drivers/dma-buf/.kunitconfig')
+ for kcfg in "${kunitconfigs[@]}"
+ [[ ! -f /kernel/drivers/gpu/tests/.kunitconfig ]]
+ /kernel/tools/testing/kunit/kunit.py run --kunitconfig /kernel/drivers/gpu/tests/.kunitconfig
[20:17:12] Configuring KUnit Kernel ...
Generating .config ...
Populating config with:
$ make ARCH=um O=.kunit olddefconfig
[20:17:16] Building KUnit Kernel ...
Populating config with:
$ make ARCH=um O=.kunit olddefconfig
Building with:
$ make all compile_commands.json scripts_gdb ARCH=um O=.kunit --jobs=48
[20:17:36] Starting KUnit Kernel (1/1)...
[20:17:36] ============================================================
Running tests with:
$ .kunit/linux kunit.enable=1 mem=1G console=tty kunit_shutdown=halt
[20:17:36] ============= refcount_interrupt (4 subtests) ==============
[20:17:36] [PASSED] test_single_irq_change
[20:17:36] [PASSED] test_nested_irq_change
[20:17:36] [PASSED] test_multiple_irq_change
[20:17:36] [PASSED] test_irq_save
[20:17:36] =============== [PASSED] refcount_interrupt ================
[20:17:36] ================= gpu_buddy (14 subtests) ==================
[20:17:36] [PASSED] gpu_test_buddy_alloc_limit
[20:17:36] [PASSED] gpu_test_buddy_alloc_optimistic
[20:17:36] [PASSED] gpu_test_buddy_alloc_pessimistic
[20:17:36] [PASSED] gpu_test_buddy_alloc_pathological
[20:17:36] [PASSED] gpu_test_buddy_alloc_contiguous
[20:17:36] [PASSED] gpu_test_buddy_alloc_clear
[20:17:36] [PASSED] gpu_test_buddy_alloc_range
[20:17:37] [PASSED] gpu_test_buddy_alloc_range_bias
[20:17:38] [PASSED] gpu_test_buddy_fragmentation_performance
[20:17:38] [PASSED] gpu_test_buddy_dirty_tracker_performance
[20:17:38] [PASSED] gpu_test_buddy_alloc_exceeds_max_order
[20:17:38] [PASSED] gpu_test_buddy_offset_aligned_allocation
[20:17:38] [PASSED] gpu_test_buddy_subtree_offset_alignment_stress
[20:17:38] [PASSED] gpu_test_buddy_addr_to_block
[20:17:38] ==================== [PASSED] gpu_buddy ====================
[20:17:38] ============================================================
[20:17:38] Testing complete. Ran 18 tests: passed: 18
[20:17:38] Elapsed time: 26.518s total, 4.402s configuring, 20.199s building, 1.844s running

+ for kcfg in "${kunitconfigs[@]}"
+ [[ ! -f /kernel/drivers/gpu/drm/xe/.kunitconfig ]]
+ /kernel/tools/testing/kunit/kunit.py run --kunitconfig /kernel/drivers/gpu/drm/xe/.kunitconfig
ERROR:root:../drivers/gpu/drm/xe/xe_vsec.c: In function ‘cri_late_bind_probe_work’:
../drivers/gpu/drm/xe/xe_vsec.c:298:13: error: implicit declaration of function ‘xe_is_oobmsm_fw_ready’ [-Werror=implicit-function-declaration]
  298 |         if (xe_is_oobmsm_fw_ready(xe)) {
      |             ^~~~~~~~~~~~~~~~~~~~~
cc1: some warnings being treated as errors
make[7]: *** [../scripts/Makefile.build:290: drivers/gpu/drm/xe/xe_vsec.o] Error 1
make[7]: *** Waiting for unfinished jobs....
make[6]: *** [../scripts/Makefile.build:551: drivers/gpu/drm/xe] Error 2
make[5]: *** [../scripts/Makefile.build:551: drivers/gpu/drm] Error 2
make[4]: *** [../scripts/Makefile.build:551: drivers/gpu] Error 2
make[3]: *** [../scripts/Makefile.build:551: drivers] Error 2
make[2]: *** [/kernel/Makefile:2229: .] Error 2
make[1]: *** [/kernel/Makefile:248: __sub-make] Error 2
make: *** [Makefile:248: __sub-make] Error 2

[20:17:38] Configuring KUnit Kernel ...
Regenerating .config ...
Populating config with:
$ make ARCH=um O=.kunit olddefconfig
[20:17:40] Building KUnit Kernel ...
Populating config with:
$ make ARCH=um O=.kunit olddefconfig
Building with:
$ make all compile_commands.json scripts_gdb ARCH=um O=.kunit --jobs=48
+ cleanup
++ stat -c %u:%g /kernel
+ chown -R 1003:1003 /kernel