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

>-----Original Message-----
>From: Intel-xe <[email protected]> On Behalf Of Ruhl,
>Michael J
>Sent: Thursday, August 13, 2026 12:51 PM
>To: [email protected]
>Cc: [email protected]
>Subject: 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.

Upon further reflection, xe_mmio_read32() is used for using device registers, and has
code to address specific HW requirements.

For this context the "register" is part of the PMT discovery stuct, so doing a memcpy
is reasonable.

So keeping this the way it is.

m

>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