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?

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

> +
> +	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/[email protected]?part=8