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: Sun, 12 Apr 2026 11:44:34 +1000
Message-ID: <review-patch3-20260409062617.1182-4-jianping.li@oss.qualcomm.com>
In-Reply-To: <20260409062617.1182-4-jianping.li@oss.qualcomm.com>
References: <20260409062617.1182-1-jianping.li@oss.qualcomm.com> <20260409062617.1182-4-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: Has critical bugs. Needs rework.**

The design intent is sound: allocate the Audio PD reserved memory once in `=
fastrpc_rpmsg_probe()` and tie its lifetime to the rpmsg channel, rather th=
an allowing unsafe userspace-controlled alloc/free. However, the implementa=
tion has several issues:

**Bug 1 (Critical): `fastrpc_buf_free()` will crash on the probe-allocated =
`remote_heap`.**

In `fastrpc_rpmsg_probe`, the buffer is allocated with bare `kzalloc`:

```c
data->remote_heap =3D
    kzalloc_obj(*data->remote_heap, GFP_KERNEL);
// ...
data->remote_heap->dma_addr =3D res.start;
data->remote_heap->size =3D resource_size(&res);
```

This leaves `buf->fl` and `buf->dev` as `NULL`. But `fastrpc_buf_free()` de=
references both:

```c
static void fastrpc_buf_free(struct fastrpc_buf *buf)
{
    dma_free_coherent(buf->dev, buf->size, buf->virt,           // buf->dev=
 is NULL
                      fastrpc_ipa_to_dma_addr(buf->fl->cctx,    // buf->fl =
is NULL =E2=86=92 crash
                                              buf->dma_addr));
    kfree(buf);
}
```

In `fastrpc_rpmsg_remove`, `fastrpc_buf_free(cctx->remote_heap)` is called,=
 which will NULL-dereference `buf->fl->cctx`. Additionally, `dma_free_coher=
ent(NULL, ...)` would also crash. This buffer is a tracking struct for rese=
rved DT memory, not a DMA-coherent allocation =E2=80=94 it should be freed =
with `kfree()`, not `fastrpc_buf_free()`.

**Bug 2: Uninitialized `res` used for ADSP when reserved memory is absent.**

The code structure is:

```c
if (domain_id =3D=3D SDSP_DOMAIN_ID || domain_id =3D=3D ADSP_DOMAIN_ID) {
    struct resource res;
    err =3D of_reserved_mem_region_to_resource(rdev->of_node, 0, &res);
    if (!err) {
        // SCM assign using res...
    }
    if (domain_id =3D=3D ADSP_DOMAIN_ID) {
        // ...
        data->remote_heap->dma_addr =3D res.start;       // used unconditio=
nally
        data->remote_heap->size =3D resource_size(&res);  // used unconditi=
onally
    }
}
```

If `of_reserved_mem_region_to_resource()` fails (no reserved memory in DT),=
 `res` is uninitialized stack data. The ADSP block still runs and stores ga=
rbage into `dma_addr` and `size`. The check in `fastrpc_init_create_static_=
process` (`!fl->cctx->remote_heap->dma_addr || !fl->cctx->remote_heap->size=
`) only catches zero values, not arbitrary garbage. The ADSP `kzalloc` bloc=
k must be inside the `if (!err)` guard, or separately check `err`.

**Bug 3: `return -ENOMEM` leaks `data`.**

```c
if (!data->remote_heap)
    return -ENOMEM;
```

At this point in probe, `data` has been allocated via `kzalloc_obj`. A bare=
 `return` skips `err_free_data` cleanup. Should be:

```c
if (!data->remote_heap) {
    err =3D -ENOMEM;
    goto err_free_data;
}
```

**Bug 4: Redundant condition and leak in remove.**

```c
if (cctx->remote_heap && cctx->vmcount) {
    if (cctx->vmcount) {           // always true =E2=80=94 redundant
```

Also, if `qcom_scm_assign_mem` fails, `cctx->remote_heap` is never freed (n=
either the struct nor the reserved memory is reclaimed). And if `cctx->remo=
te_heap` is set but `!cctx->vmcount`, the struct leaks entirely.

**Issue 5: `audio_init_mem =3D false` on error without locking.**

```c
err_invoke:
    fl->cctx->audio_init_mem =3D false;
```

This write is outside the `cctx->lock` spinlock, while the read in the succ=
ess path is inside `spin_lock_irqsave(&cctx->lock, ...)`. This is a data ra=
ce, although the practical impact may be low if concurrent static process c=
reation is unlikely.

**Suggestion for rework:** Consider not using `struct fastrpc_buf` at all f=
or the probe-allocated reserved memory tracking. A simpler approach would b=
e to store `dma_addr` and `size` directly in `fastrpc_channel_ctx`, or use =
a dedicated struct that doesn't carry `fl`/`dev`/`virt` baggage. If `fastrp=
c_buf` must be used, the cleanup in `fastrpc_rpmsg_remove` should use `kfre=
e()` instead of `fastrpc_buf_free()`.

---

---
Generated by Claude Code Patch Reviewer