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

>-----Original Message-----
>From: [email protected] <[email protected]>
>Sent: Wednesday, August 12, 2026 4:05 PM
>To: Ruhl, Michael J <[email protected]>
>Cc: [email protected]
>Subject: 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.

Agreed.  I will remove the mmio argument.

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

Agreed, the extra _put will be removed.

m

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