Re: [PATCH 4/4] drm/msm/a6xx: Import a680 hwcg sequence from kgsl

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] drm/msm/a6xx: out-of-bounds gpu_write() on GMU registers in a680_hwcg
--

commit f29e5fbd58a4870ae3b815aaaebc1ea0d6ff115a
Author: Val Packett <val@packett.cool>

drm/msm/a6xx: Import a680 hwcg sequence from kgsl

This commit adds an a680_hwcg register table imported from downstream kgsl
and wires it up to the Adreno 680 GPU catalog entry in place of a640_hwcg.

> diff --git a/drivers/gpu/drm/msm/adreno/a6xx_catalog.c b/drivers/gpu/drm/msm/adreno/a6xx_catalog.c
> index 6c154792cb077..48197a54984cf 100644
> --- a/drivers/gpu/drm/msm/adreno/a6xx_catalog.c
> +++ b/drivers/gpu/drm/msm/adreno/a6xx_catalog.c
> @@ -9,6 +9,7 @@
>  #include "adreno_gpu.h"
>  #include "a6xx_gpu.h"
>  #include "a6xx.xml.h"
> +#include "a6xx_gmu.xml.h"
>  
>  static const struct adreno_reglist a612_hwcg[] = {
>  	{REG_A6XX_RBBM_CLOCK_CNTL_SP0, 0x22222222},
> @@ -449,6 +450,59 @@ static const struct adreno_reglist a660_hwcg[] = {
>  	{},
>  };
>  
> +static const struct adreno_reglist a680_hwcg[] = {

[ ... ]

> +	{REG_A6XX_RBBM_CLOCK_CNTL_GMU_GX, 0x00000222},
> +	{REG_A6XX_RBBM_CLOCK_DELAY_GMU_GX, 0x00000111},
> +	{REG_A6XX_RBBM_CLOCK_HYST_GMU_GX, 0x00000555},
> +	{REG_A6XX_GMU_GX_GMU_SP_RF_CONTROL_0, 0x00000001},
> +	{REG_A6XX_GMU_GX_GMU_SP_RF_CONTROL_1, 0x00000001},

[Severity: High]
Does placing REG_A6XX_GMU_GX_GMU_SP_RF_CONTROL_0 and
REG_A6XX_GMU_GX_GMU_SP_RF_CONTROL_1 into a680_hwcg cause an out-of-bounds
MMIO write past the end of gpu->mmio?

During GPU initialization and suspend (via hw_init() -> a6xx_set_hwcg() and
a6xx_pm_suspend() -> a6xx_set_hwcg()), every entry in the hwcg table is
programmed using gpu_write():

drivers/gpu/drm/msm/adreno/a6xx_gpu.c:a6xx_set_hwcg() {
	...
	for (i = 0; (reg = &adreno_gpu->info->a6xx->hwcg[i], reg->offset); i++)
		gpu_write(gpu, reg->offset, state ? reg->value : 0);
	...
}

In drivers/gpu/drm/msm/registers/adreno/a6xx_gmu.xml, these two registers are
GMU registers at dword offsets 0x1a883 and 0x1a884 (byte offsets 0x6a20c and
0x6a210 from the GPU base address).

Unlike downstream kgsl, upstream maps the GPU and GMU register spaces
separately. In arch/arm64/boot/dts/qcom/sc8180x.dtsi, the gpu@2c00000 node's
kgsl_3d0_reg_memory region mapped into gpu->mmio is only 0x40000 bytes:

	gpu@2c00000 {
		...
		reg = <0 0x02c00000 0 0x40000>;
		reg-names = "kgsl_3d0_reg_memory";

while the GMU registers at 0x02c6a000 are mapped separately into gmu->mmio by
gmu@2c6a000 and accessed via gmu_write(), which subtracts gmu->mmio_offset
(0x6a000):

drivers/gpu/drm/msm/adreno/a6xx_gmu.h:
#define GMU_BYTE_OFFSET(gmu, offset) (((offset) << 2) - (gmu)->mmio_offset)
...
static inline void gmu_write(struct a6xx_gmu *gmu, u32 offset, u32 value)
{
	writel(value, gmu->mmio + GMU_BYTE_OFFSET(gmu, offset));
}

When a6xx_set_hwcg() calls gpu_write() with offsets 0x1a883 and 0x1a884, it
writes to gpu->mmio + 0x6a20c and gpu->mmio + 0x6a210, which is 0x2a20c bytes
past the end of the 0x40000-byte gpu->mmio mapping. Should these GMU register
writes be routed through gmu_write() instead of gpu_write()?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261103011358.65398-1-val@packett.cool?part=4