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