From mboxrd@z Thu Jan  1 00:00:00 1970
From: Claude Code Review Bot <claude-review@example.com>
To: dri-devel-reviews@example.com
Subject: Claude review: misc: fastrpc: Allocate entire reserved memory for Audio PD in probe
Date: Sat, 16 May 2026 09:09:54 +1000
Message-ID: <review-patch4-20260515124217.20723-5-jianping.li@oss.qualcomm.com>
In-Reply-To: <20260515124217.20723-5-jianping.li@oss.qualcomm.com>
References: <20260515124217.20723-1-jianping.li@oss.qualcomm.com> <20260515124217.20723-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

**Verdict: Needs fixes =E2=80=94 two memory leaks in probe error paths, and=
 a logic issue with `audio_init_mem` reset.**

**Bug 1: Memory leak on kzalloc failure in probe**

```c
+			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;
```

At this point, `data` has already been allocated (line ~2373). The bare `re=
turn -ENOMEM` leaks `data`. This should be `goto err_free_data` or equivale=
nt. Also, note that `kzalloc_obj(*data->remote_heap)` without explicit `GFP=
_KERNEL` would be equivalent since `default_gfp()` defaults to `GFP_KERNEL`.

**Bug 2: Memory leak on SCM failure in probe**

If `qcom_scm_assign_mem()` fails after `data->remote_heap` was allocated:

```c
			err =3D qcom_scm_assign_mem(res.start, resource_size(&res), &src_perms,
				    data->vmperms, data->vmcount);
			if (err)
				goto err_free_data;
```

The `err_free_data` label only does `kfree(data)` =E2=80=94 it does not fre=
e `data->remote_heap`. This leaks the remote_heap allocation.

**Issue 3: Unconditional `audio_init_mem =3D false` in err_invoke**

```c
 err_invoke:
-	if (fl->cctx->vmcount && scm_done) { ... }
-err_map:
-	fastrpc_buf_free(fl->cctx->remote_heap);
-	fl->cctx->remote_heap =3D NULL;
-err_name:
+	fl->cctx->audio_init_mem =3D false;
```

If two concurrent callers enter `fastrpc_init_create_static_process()`, the=
 first one sets `audio_init_mem =3D true` and succeeds. The second caller t=
akes the `else` branch (pageslen=3D0, pages=3D0). If the second caller's in=
voke fails, `audio_init_mem` is unconditionally reset to `false`. A subsequ=
ent third caller would then re-send the memory information to the DSP, whic=
h the DSP may have already received from the first call. The reset should b=
e conditional:

```c
err_invoke:
	if (inbuf.pageslen)
		fl->cctx->audio_init_mem =3D false;
```

**Style nit: Inconsistent cctx usage**

The patch adds `struct fastrpc_channel_ctx *cctx =3D fl->cctx;` but then mi=
xes `fl->cctx->` and `cctx->` in the same code block:

```c
+	spin_lock_irqsave(&cctx->lock, flags);
+	if (!fl->cctx->audio_init_mem) {
+		if (!fl->cctx->remote_heap ||
```

Should consistently use `cctx->` throughout.

**Observation on remove path**

The remove path correctly uses `kfree()` instead of `fastrpc_buf_free()` si=
nce the buffer was allocated with `kzalloc_obj` (not DMA coherent allocatio=
n). However, if `qcom_scm_assign_mem` fails during remove, the remote_heap =
struct is never freed:

```c
+		err =3D qcom_scm_assign_mem(cctx->remote_heap->dma_addr,
+					  cctx->remote_heap->size, &src_perms,
+					  &dst_perms, 1);
+		if (!err)
+			kfree(cctx->remote_heap);
```

If the SCM call fails during teardown, there's not much that can be done, b=
ut a `dev_err()` log would be helpful for debugging, and the memory should =
arguably be freed anyway since the channel is shutting down (the SCM failur=
e means the memory ownership is in a bad state, but leaking the tracking st=
ruct doesn't help).

Also: if `cctx->vmcount` is 0 but `cctx->remote_heap` is non-NULL, the enti=
re block is skipped and `remote_heap` is leaked. This could happen if the A=
DSP domain has reserved-memory in DT but no `qcom,vmids`.

---

---
Generated by Claude Code Patch Reviewer