Re: [PATCH v11 16/20] drm/xe/vsec: Crescent Island PMT callbacks

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] goto mixing: goto-based error handling mixed with scoped_guard()
--

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

drm/xe/vsec: Crescent Island PMT callbacks

This commit introduces the necessary callbacks to get the status and control
information for Crescent Island (CRI) PMT usage. It adds xe_pmt_read_reg() and
xe_pmt_write_reg() to access the discovery status and control areas, and
registers the CRI PMT functionality.

diff --git a/drivers/gpu/drm/xe/xe_vsec.c b/drivers/gpu/drm/xe/xe_vsec.c
index a814d7151eeae..0f6e3be520c09 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)
> +{
> +	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);
> +
> +	scoped_guard(mutex, &xe->pmt.lock) {
> +		xe->soc_remapper.set_telem_region(xe, CRI_IDX_TELEM_DISCOVERY);
> +
> +		*reg = readl(disc_addr);
> +	}
> +
> +	xe_pm_runtime_put(xe);
> +
> +dev_exit:
> +	drm_dev_exit(idx);
> +
> +	return ret;
> +}

[Severity: Low]
Does this violate the cleanup API contract by mixing manual goto-based cleanup
in xe_pmt_read_reg() with scope-based cleanup via scoped_guard()?

According to the subsystem guidelines in include/linux/cleanup.h, we should
either convert all resources that need a goto cleanup to scope-based cleanup,
or convert none of them. While this specific code path in
drivers/gpu/drm/xe/xe_vsec.c avoids immediate resource leaks because the
scope exits cleanly before the goto labels are reached, this architectural
pattern creates confusing ownership semantics and increases the risk of
double-free or resource leaks during future refactoring.

> +
> +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);
> +
> +	scoped_guard(mutex, &xe->pmt.lock) {
> +		xe->soc_remapper.set_telem_region(xe, CRI_IDX_TELEM_DISCOVERY);
> +
> +		writel(reg, disc_addr);
> +	}
> +
> +	xe_pm_runtime_put(xe);
> +
> +dev_exit:
> +	drm_dev_exit(idx);
> +
> +	return ret;
> +}

[Severity: Low]
Similarly here in xe_pmt_write_reg(), is it appropriate to mix the goto dev_exit
error handling with the scoped_guard() block?

As noted above, mixing these two cleanup styles in the same function is
discouraged to prevent confusing semantics in the execution paths.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260923181115.2514193-22-michael.j.ruhl@intel.com?part=16