RE: [PATCH v2 08/10] drm/xe/vsec: Crescent Island PMT callbacks

>-----Original Message-----
>From: [email protected] <[email protected]>
>Sent: Wednesday, August 12, 2026 3:57 PM
>To: Ruhl, Michael J <[email protected]>
>Cc: [email protected]
>Subject: Re: [PATCH v2 08/10] drm/xe/vsec: Crescent Island PMT callbacks
>
>Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to
>consider:
>- [High] Improper MMIO access for 32-bit control registers using byte-wise
>copies (memcpy_toio/memcpy_fromio).
>--
>
>--- Patch [8]: [PATCH v2 08/10] drm/xe/vsec: Crescent Island PMT callbacks ---
>commit 664ab77147de6f456af0ceaeae6a996ddd3d6a40
>Author: Michael J. Ruhl <[email protected]>
>drm/xe/vsec: Crescent Island PMT callbacks
>
>This commit introduces callbacks to get the status and control information
>for CRI PMT usage. It adds the glue logic to register the CRI PMT
>functionality in the xe driver.
>
>> diff --git a/drivers/gpu/drm/xe/xe_vsec.c b/drivers/gpu/drm/xe/xe_vsec.c
>> index dc42b9492428a..578d59048b39c 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: High]
>Will this cause unsupported requests or return garbage on some platforms?
>
>Using memcpy_fromio() for a 32-bit register read often falls back to
>byte-wise transactions (like rep movsb) instead of a single 32-bit atomic
>transaction. For MMIO control registers, issuing four 1-byte PCIe
>transactions can cause the hardware to drop the request.
>
>Should this use readl() or the driver's xe_mmio_read32() wrapper instead
>to guarantee the correct 32-bit transaction width?


xe_mmio_read32 makes sense.  I will update.

M

>> +
>> +	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)
>> +{
>
>[ ... ]
>
>> +	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: High]
>Does this have the same byte-wise transaction issue?
>
>Similar to the read path above, using memcpy_toio() for a single 32-bit
>register write can result in 4 byte-sized MMIO writes. Should this use
>writel() or xe_mmio_write32() to ensure a single 32-bit atomic write?

will update to xe_mmio_write32().

M

>> +
>> +	mutex_unlock(&xe->pmt.lock);
>> +	xe_pm_runtime_put(xe);
>> +
>> +dev_exit:
>> +	drm_dev_exit(idx);
>> +
>> +	return ret;
>> +}
>
>[ ... ]
>
>--
>Sashiko AI review ·
>https://sashiko.dev/#/patchset/20260812193742.2170188-12-
>[email protected]?part=8