From mboxrd@z Thu Jan 1 00:00:00 1970 From: Claude Code Review Bot To: dri-devel-reviews@example.com Subject: Claude review: misc: fastrpc: Allocate entire reserved memory for Audio PD in probe Date: Thu, 04 Jun 2026 13:11:16 +1000 Message-ID: In-Reply-To: <20260602071750.526-5-jianping.li@oss.qualcomm.com> References: <20260602071750.526-1-jianping.li@oss.qualcomm.com> <20260602071750.526-5-jianping.li@oss.qualcomm.com> X-Mailer: Claude Code Patch Reviewer Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Patch Review **Purpose:** Move Audio PD reserved memory allocation from lazy (on first s= tatic process creation) to probe time, tying its lifetime to the rpmsg chan= nel. **Analysis:** This is the most significant patch in the series. **Probe changes:** ```c - if (domain_id =3D=3D SDSP_DOMAIN_ID) { + if (domain_id =3D=3D SDSP_DOMAIN_ID || domain_id =3D=3D ADSP_DOMAIN_ID) { struct resource res; u64 src_perms; =20 err =3D of_reserved_mem_region_to_resource(rdev->of_node, 0, &res); if (!err) { + if (domain_id =3D=3D ADSP_DOMAIN_ID) { + data->remote_heap =3D + kzalloc_obj(*data->remote_heap, GFP_KERNEL); + if (!data->remote_heap) + return -ENOMEM; + + data->remote_heap->dma_addr =3D res.start; + data->remote_heap->size =3D resource_size(&res); + } src_perms =3D BIT(QCOM_SCM_VMID_HLOS); ``` **Issue 1 - Memory leak on SCM failure:** If `kzalloc_obj` succeeds but the= subsequent `qcom_scm_assign_mem` fails, code jumps to `err_free_data` whic= h does not free `data->remote_heap`. This is a leak. **Issue 2 - ADSP without reserved memory:** If `of_reserved_mem_region_to_r= esource` returns an error for ADSP, the code skips the entire block =E2=80= =94 `remote_heap` remains NULL. Combined with patch 3, this means Audio PD = init will always fail on platforms without reserved memory, which is the in= tended behavior per the cover letter. Good. **Issue 3 - Missing return value check:** The `return -ENOMEM` in probe ski= ps the error cleanup path. The probe function has allocated other resources= before this point (potentially `data` itself, sessions, etc.) that won't b= e cleaned up. Should use `goto err_free_data` instead of `return -ENOMEM`. **Static process creation changes:** The lazy allocation and SCM assignment= logic is removed. Instead, a new `audio_init_mem` flag protected by `cctx-= >lock` (with `spin_lock_irqsave`) gates whether pages info is sent: ```c + spin_lock_irqsave(&cctx->lock, flags); + if (!fl->cctx->audio_init_mem) { + pages[0].addr =3D fl->cctx->remote_heap->dma_addr; + pages[0].size =3D fl->cctx->remote_heap->size; + fl->cctx->audio_init_mem =3D true; + inbuf.pageslen =3D 1; + } else { + pages[0].addr =3D 0; + pages[0].size =3D 0; + } + spin_unlock_irqrestore(&cctx->lock, flags); ``` **Issue 4 - Inconsistent use of cctx vs fl->cctx:** The code introduces `st= ruct fastrpc_channel_ctx *cctx =3D fl->cctx;` but then uses `fl->cctx->` in= several places within the spinlock section. Should consistently use `cctx-= >` for clarity. **Error path changes:** ```c err_invoke: - if (fl->cctx->vmcount && scm_done) { - ...scm_assign_mem reversal... - } -err_map: - fastrpc_buf_free(fl->cctx->remote_heap); - fl->cctx->remote_heap =3D NULL; -err_name: + fl->cctx->audio_init_mem =3D false; ``` This correctly resets the flag so a retry can re-send pages, and no longer = frees the remote_heap since its lifetime is now tied to the rpmsg channel. **Remove path changes:** ```c + if (cctx->remote_heap && cctx->vmcount) { + ...reverse SCM assignment... + err =3D qcom_scm_assign_mem(...); + if (!err) + kfree(cctx->remote_heap); + } ``` **Issue 5 - Leak when SCM reassign fails:** If `qcom_scm_assign_mem` fails = in the remove path, the `remote_heap` struct is leaked (never freed). The o= ld code called `fastrpc_buf_free` unconditionally. This is a deliberate cho= ice (can't free memory the DSP still has access to), but the error should a= t least be logged. **Issue 6 - SDSP remote_heap handling regression:** The removal path previo= usly called `fastrpc_buf_free(cctx->remote_heap)` unconditionally for any d= omain with a remote_heap. Now it only does `kfree` and only when `vmcount >= 0`. For SDSP (which also sets `remote_heap` via `fastrpc_remote_heap_alloc= `), the old `fastrpc_buf_free` call freed DMA memory. The new code uses `kf= ree` only, which would leak DMA memory for SDSP. Wait =E2=80=94 looking mor= e carefully, the SDSP path doesn't set `remote_heap` in the current upstrea= m; `remote_heap` is only set in `fastrpc_init_create_static_process`. So SD= SP would still go through the static process creation path and set it that = way. But with patch 4, the `fastrpc_init_create_static_process` no longer a= llocates `remote_heap`. This means SDSP `remote_heap` allocation is now bro= ken =E2=80=94 there's no code path left that allocates it for SDSP. Actually, re-reading more carefully: the SDSP probe path does SCM assign bu= t does NOT set `remote_heap`. The SDSP `remote_heap` would still be allocat= ed in `fastrpc_init_create_static_process`... but patch 4 removes that allo= cation code. This seems like a regression for SDSP. **Verdict:** Several issues need fixing: the probe error path leak, inconsi= stent variable usage, and the potential SDSP regression. --- --- Generated by Claude Code Patch Reviewer