Claude review: misc: fastrpc: Allocate entire reserved memory for Audio PD in probe

[Claude Code Review Bot <claude-review@example.com>]

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 than allowing unsafe userspace-controlled alloc/free. However, the implementation 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 =
    kzalloc_obj(*data->remote_heap, GFP_KERNEL);
// ...
data->remote_heap->dma_addr = res.start;
data->remote_heap->size = resource_size(&res);
```

This leaves `buf->fl` and `buf->dev` as `NULL`. But `fastrpc_buf_free()` dereferences 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 → 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_coherent(NULL, ...)` would also crash. This buffer is a tracking struct for reserved DT memory, not a DMA-coherent allocation — 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 == SDSP_DOMAIN_ID || domain_id == ADSP_DOMAIN_ID) {
    struct resource res;
    err = of_reserved_mem_region_to_resource(rdev->of_node, 0, &res);
    if (!err) {
        // SCM assign using res...
    }
    if (domain_id == ADSP_DOMAIN_ID) {
        // ...
        data->remote_heap->dma_addr = res.start;       // used unconditionally
        data->remote_heap->size = resource_size(&res);  // used unconditionally
    }
}
```

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 garbage 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` block 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 = -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 — redundant
```

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

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

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

This write is outside the `cctx->lock` spinlock, while the read in the success path is inside `spin_lock_irqsave(&cctx->lock, ...)`. This is a data race, although the practical impact may be low if concurrent static process creation is unlikely.

**Suggestion for rework:** Consider not using `struct fastrpc_buf` at all for the probe-allocated reserved memory tracking. A simpler approach would be 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 `fastrpc_buf` must be used, the cleanup in `fastrpc_rpmsg_remove` should use `kfree()` instead of `fastrpc_buf_free()`.

---

---
Generated by Claude Code Patch Reviewer