Re: [PATCH 2/8] drm/msm/adreno: migrate zap shader loading to qcom_mdt_pas_load() and qcom_pas_prepare_and_auth_reset()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] drm/msm/adreno: memory leak of PAS metadata buffer
- [High] drm/msm/adreno: shmbridge preparation step silently skipped
- [Medium] drm/msm/adreno: struct qcom_pas_context leaked via devres on post-probe retries
--

commit 4d137953382eb8839541fc9229948a9472d95133
Author: Mukesh Ojha <mukesh.ojha@oss.qualcomm.com>

drm/msm/adreno: migrate zap shader loading to qcom_mdt_pas_load() and qcom_pas_prepare_and_auth_reset()

This commit replaces the open-coded memremap/mdt_load sequence for zap shader
loading with devm_qcom_pas_context_alloc and qcom_mdt_pas_load. It then passes
the context to qcom_pas_prepare_and_auth_reset to ensure the shmbridge
preparation step is executed before TrustZone authentication.

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

[ ... ]

> @@ -122,9 +122,9 @@ static int zap_shader_load_mdt(struct msm_gpu *gpu, const char *fwname,
>  	}
>  
>  	/* Allocate memory for the firmware image */
> -	mem_region = memremap(mem_phys, mem_size,  MEMREMAP_WC);
> -	if (!mem_region) {
> -		ret = -ENOMEM;
> +	ctx = devm_qcom_pas_context_alloc(dev, pasid, mem_phys, resource_size(&r));
> +	if (IS_ERR(ctx)) {
> +		ret = PTR_ERR(ctx);

[Severity: High]
Is the shmbridge preparation step actually skipped here?

The commit message mentions fixing the omission of the shmbridge prepare step.
However, devm_qcom_pas_context_alloc() uses devm_kzalloc() which zeroes the
context, and ctx->use_tzmem is never explicitly set to true in
zap_shader_load_mdt().

Because of this, it looks like qcom_pas_prepare_and_auth_reset() will hit
the early return (if (!ctx->use_tzmem)) downstream and bypass the shmbridge
creation entirely, leaving the behaviour unchanged.

[Severity: Medium]
Can this devm allocation cause an accumulating memory leak during post-probe 
retries?

Since devm_qcom_pas_context_alloc() ties the allocation to the device
lifetime, if zap_shader_load_mdt() fails and is called multiple times during
runtime PM resume or GPU recovery sequences, the context allocations might
accumulate without being freed since the device remains bound.

>  		goto out;
>  	}
>  

[ ... ]

> @@ -165,9 +163,6 @@ static int zap_shader_load_mdt(struct msm_gpu *gpu, const char *fwname,
>  		DRM_DEV_ERROR(dev, "Unable to authorize the image\n");
>  
>  out:
> -	if (mem_region)
> -		memunmap(mem_region);
> -
>  	release_firmware(fw);
>  
>  	return ret;

[Severity: High]
Does this exit path leak the PAS metadata buffer?

When qcom_mdt_pas_load() executes, it internally calls qcom_pas_init_image(),
which allocates a metadata buffer and stores it in ctx->ptr. Unlike the
previous flow with a NULL context where the buffer was immediately freed
internally, a valid context tracks it so the caller can free it.

Should there be a call to qcom_pas_metadata_release(ctx) in this exit path
to prevent leaking this physical memory on every execution?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260925155737.4161358-1-mukesh.ojha@oss.qualcomm.com?part=2