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

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

Patch Review

**Purpose:** Move Audio PD reserved memory allocation from lazy (on first static process creation) to probe time, tying its lifetime to the rpmsg channel.

**Analysis:** This is the most significant patch in the series.

**Probe changes:**
```c
-	if (domain_id == SDSP_DOMAIN_ID) {
+	if (domain_id == SDSP_DOMAIN_ID || domain_id == ADSP_DOMAIN_ID) {
 		struct resource res;
 		u64 src_perms;
 
 		err = of_reserved_mem_region_to_resource(rdev->of_node, 0, &res);
 		if (!err) {
+			if (domain_id == ADSP_DOMAIN_ID) {
+				data->remote_heap =
+					kzalloc_obj(*data->remote_heap, GFP_KERNEL);
+				if (!data->remote_heap)
+					return -ENOMEM;
+
+				data->remote_heap->dma_addr = res.start;
+				data->remote_heap->size = resource_size(&res);
+			}
 			src_perms = 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` which does not free `data->remote_heap`. This is a leak.

**Issue 2 - ADSP without reserved memory:** If `of_reserved_mem_region_to_resource` returns an error for ADSP, the code skips the entire block — `remote_heap` remains NULL. Combined with patch 3, this means Audio PD init will always fail on platforms without reserved memory, which is the intended behavior per the cover letter. Good.

**Issue 3 - Missing return value check:** The `return -ENOMEM` in probe skips the error cleanup path. The probe function has allocated other resources before this point (potentially `data` itself, sessions, etc.) that won't be 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 = fl->cctx->remote_heap->dma_addr;
+		pages[0].size = fl->cctx->remote_heap->size;
+		fl->cctx->audio_init_mem = true;
+		inbuf.pageslen = 1;
+	} else {
+		pages[0].addr = 0;
+		pages[0].size = 0;
+	}
+	spin_unlock_irqrestore(&cctx->lock, flags);
```

**Issue 4 - Inconsistent use of cctx vs fl->cctx:** The code introduces `struct fastrpc_channel_ctx *cctx = 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 = NULL;
-err_name:
+	fl->cctx->audio_init_mem = 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 = 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 old code called `fastrpc_buf_free` unconditionally. This is a deliberate choice (can't free memory the DSP still has access to), but the error should at least be logged.

**Issue 6 - SDSP remote_heap handling regression:** The removal path previously called `fastrpc_buf_free(cctx->remote_heap)` unconditionally for any domain 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 `kfree` only, which would leak DMA memory for SDSP. Wait — looking more carefully, the SDSP path doesn't set `remote_heap` in the current upstream; `remote_heap` is only set in `fastrpc_init_create_static_process`. So SDSP 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 allocates `remote_heap`. This means SDSP `remote_heap` allocation is now broken — there's no code path left that allocates it for SDSP.

Actually, re-reading more carefully: the SDSP probe path does SCM assign but does NOT set `remote_heap`. The SDSP `remote_heap` would still be allocated in `fastrpc_init_create_static_process`... but patch 4 removes that allocation code. This seems like a regression for SDSP.

**Verdict:** Several issues need fixing: the probe error path leak, inconsistent variable usage, and the potential SDSP regression.

---

---
Generated by Claude Code Patch Reviewer