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

[sashiko-bot@kernel.org]

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