Re: [PATCH v2 10/10] drm/xe/vsec: Update PMT internal access for CRI

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] The function `xe_pmt_write_reg` executes a double put on the runtime PM usage counter on its success path.
- [Low] Unused `mmio` parameter in `read_residency_counter`.
--

--- Patch [10]: [PATCH v2 10/10] drm/xe/vsec: Update PMT internal access for CRI ---
commit dad9ae19b6a680a51c5ad534968234e3d02be8a6
Author: Michael J. Ruhl <[email protected]>
Subject: drm/xe/vsec: Update PMT internal access for CRI

This commit adds a new API to access the PMT telemetry GUID based on the
platform. It updates the debugfs, hwmon, and pcode components to use this
new xe_vsec_get_guid() function instead of reading the GUID directly.

> diff --git a/drivers/gpu/drm/xe/xe_debugfs.c b/drivers/gpu/drm/xe/xe_debugfs.c
> index 8de78cd0aa03b..0ef62b1e0e91f 100644
> --- a/drivers/gpu/drm/xe/xe_debugfs.c
> +++ b/drivers/gpu/drm/xe/xe_debugfs.c
> @@ -96,11 +96,16 @@ 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;
> +	u32 guid;
>  	int ret;
>  
> -	ret = xe_pmt_telem_read(xe->drm.dev,
> -				xe_mmio_read32(mmio, PUNIT_TELEMETRY_GUID),
> -				&residency, offset, sizeof(residency));
> +	guid = xe_vsec_get_guid(xe);

[Severity: Low]
Is the mmio parameter still needed in read_residency_counter()?

With the switch to xe_vsec_get_guid(), it looks like the mmio argument is no
longer accessed in the function body and can probably be removed from the
signature.

> +	if (!guid) {
> +		drm_warn(&xe->drm, "PMT device is not powered\n");
> +		return;
> +	}
> +
> +	ret = xe_pmt_telem_read(xe->drm.dev, guid, &residency, offset, sizeof(residency));

[ ... ]

While reviewing this, I also noticed a pre-existing issue in xe_pmt_write_reg():

drivers/gpu/drm/xe/xe_vsec.c:xe_pmt_write_reg() {
    ...
	memcpy_toio(disc_addr, &reg, sizeof(reg));

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

runtime_exit:
	xe_pm_runtime_put(xe);
    ...
}

[Severity: High]
This is a pre-existing issue, but does xe_pmt_write_reg() execute a double put
on the runtime PM usage counter on its success path?

When the register write completes successfully, it unlocks the mutex and calls
xe_pm_runtime_put(xe), but then falls through to the runtime_exit label which
calls xe_pm_runtime_put(xe) a second time. Can this lead to an underflow and
cause the device to enter runtime suspend prematurely while still in use?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=10