Re: [PATCH RFC 10/13] drm/msm: initialize LPAC ring

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [Critical] Unconditional execution of LPAC initialization on all A6xx/A7xx GPUs will crash hardware without LPAC support.
- [High] The error check for the LPAC ringbuffer allocation evaluates the wrong variable, ignoring allocation failures and reading out-of-bounds.
- [Medium] The GPU hangcheck timer does not monitor the LPAC ring, causing LPAC-only hangs to go undetected.
- [Medium] Missing cleanup of `gpu->lpac_rb` in the error path of `msm_gpu_init()`.
- [Low] Unconditional allocation of the LPAC ringbuffer wastes resources for all MSM GPUs.
--

commit 35c6bd5d02bb9ed8858bdd2395719b06651d77e8
Author: Anna Maniscalco <anna.maniscalco2000@gmail.com>

drm/msm: initialize LPAC ring

Program all the register needed to start up LPAC and send initialization
sequence. Additionally, now that the LPAC ring is live, make the code aware
of it.

> diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
> --- a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
> +++ b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c

[ ... ]

> @@ -1481,6 +1567,10 @@ static int hw_init(struct msm_gpu *gpu)
>  	if (ret)
>  		goto out;
>  
> +	ret = lpac_cp_init(gpu);
> +	if (ret)
> +		goto out;
> +

[Severity: Critical]
Will this unconditional execution of LPAC initialization cause a regression on
hardware without LPAC support?

During hw_init(), the driver unconditionally writes to LPAC-specific registers
and calls lpac_cp_init(). This function sends a CP_ME_INIT packet to the LPAC
ring and waits for it to idle using a6xx_idle().

On older GPUs lacking LPAC hardware (e.g., A630), the CP engine ignores this
ring, causing the idle wait to time out and hw_init() to return -EINVAL.

>  	/*
>  	 * Try to load a zap shader into the secure world. If successful

[ ... ]

> diff --git a/drivers/gpu/drm/msm/msm_gpu.c b/drivers/gpu/drm/msm/msm_gpu.c
> --- a/drivers/gpu/drm/msm/msm_gpu.c
> +++ b/drivers/gpu/drm/msm/msm_gpu.c

[ ... ]

> @@ -1006,6 +1009,15 @@ int msm_gpu_init(struct drm_device *drm, struct platform_device *pdev,
>  		memptrs_iova += sizeof(struct msm_rbmemptrs);
>  	}
>  
> +	gpu->lpac_rb = msm_ringbuffer_new(gpu, nr_rings, memptrs, memptrs_iova);
> +

[Severity: Low]
Does this unconditional allocation of the LPAC ringbuffer waste resources for
legacy non-LPAC GPUs?

A GEM object, DRM scheduler kthread, and fence context are allocated for every
MSM GPU even if they do not support LPAC.

> +	if (IS_ERR(gpu->rb[i])) {
> +		ret = PTR_ERR(gpu->rb[i]);
> +		DRM_DEV_ERROR(drm->dev,
> +					  "could not create lpac ringbuffer %d\n", ret);
> +		goto fail;
> +	}
> +

[Severity: High]
Does this check the wrong variable for the LPAC allocation failure?

The error check evaluates gpu->rb[i] instead of gpu->lpac_rb. If the LPAC
allocation fails, gpu->lpac_rb holds an error pointer but the initialization
succeeds.

This later causes a kernel panic when the error pointer is dereferenced during
device setup. Additionally, because the loop counter i equals nr_rings here,
checking gpu->rb[i] results in an out-of-bounds array read if nr_rings equals
MSM_GPU_MAX_RINGS.

[Severity: Medium]
If GPU initialization fails and we take this goto fail path, is there a memory
leak for the LPAC ringbuffer?

The fail path in msm_gpu_init() iterates over gpu->rb and destroys those
ringbuffers, but it lacks a call to msm_ringbuffer_destroy(gpu->lpac_rb).

>  	gpu->nr_rings = nr_rings;
>  
>  	refcount_set(&gpu->sysprof_active, 1);

[ ... ]

> diff --git a/drivers/gpu/drm/msm/msm_gpu.h b/drivers/gpu/drm/msm/msm_gpu.h
> --- a/drivers/gpu/drm/msm/msm_gpu.h
> +++ b/drivers/gpu/drm/msm/msm_gpu.h

[ ... ]

> @@ -181,6 +181,8 @@ struct msm_gpu {
>  	struct msm_ringbuffer *rb[MSM_GPU_MAX_RINGS];
>  	int nr_rings;
>  
> +	struct msm_ringbuffer *lpac_rb;
> +

[Severity: Medium]
Is the GPU hangcheck timer missing monitoring for the LPAC ring?

In msm_gpu.c:hangcheck_handler(), the code only monitors the primary active
ring:

	struct msm_ringbuffer *ring = gpu->funcs->active_ring(gpu);
	...
	if (fence_after(ring->fctx->last_fence, ring->hangcheck_fence))
		hangcheck_timer_reset(gpu);

If a task submitted to the LPAC ring hangs and the primary ring is idle,
fence_after() evaluates to false and the timer is not reset. The stall goes
undetected, leaving the task hanging indefinitely without GPU recovery.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260705-descriptive-name-lpac-upstream-v1-0-01d50c3e0c99@gmail.com?part=10