public inbox for drm-ai-reviews@public-inbox.freedesktop.org
 help / color / mirror / Atom feed
From: Claude Code Review Bot <claude-review@example.com>
To: dri-devel-reviews@example.com
Subject: Claude review: misc: fastrpc: Fail Audio PD init when reserved memory is missing
Date: Thu, 04 Jun 2026 13:11:15 +1000	[thread overview]
Message-ID: <review-patch3-20260602071750.526-4-jianping.li@oss.qualcomm.com> (raw)
In-Reply-To: <20260602071750.526-4-jianping.li@oss.qualcomm.com>

Patch Review

**Purpose:** Add early validation that `remote_heap` exists before proceeding with static process creation.

**Analysis:** This patch adds a check at the very top of `fastrpc_init_create_static_process`:

```c
+	if (!fl->cctx->remote_heap ||
+	    !fl->cctx->remote_heap->dma_addr ||
+	    !fl->cctx->remote_heap->size) {
+		err = -ENOMEM;
+		dev_dbg(fl->sctx->dev, "remote heap memory region is not added\n");
+		return err;
+	}
```

**Issue 1 - Series ordering problem:** This check is added in patch 3, but at this point in the series (after patches 1-2), `remote_heap` is still allocated lazily inside `fastrpc_init_create_static_process` itself (the `if (!fl->cctx->remote_heap)` block from line 1328). So this check would cause the *first* call to `fastrpc_init_create_static_process` to always fail with `-ENOMEM`, since `remote_heap` hasn't been allocated yet. This only makes sense *after* patch 4 moves allocation to probe. The patch ordering should be swapped: patch 4 should come before patch 3, or they should be squashed.

**Issue 2 - Error code:** `-ENOMEM` is not the right error code when reserved memory is missing from the DT configuration. `-ENODEV` or `-EINVAL` would be more appropriate, as this is a configuration/platform issue, not a memory allocation failure.

**Issue 3:** The function also applies to SDSP, not just Audio PD. This check could break SDSP static process creation if `remote_heap` is not set up for SDSP (which it wouldn't be, since SDSP remote heap allocation works differently). Need to verify whether this function is called for SDSP contexts as well or only ADSP.

**Verdict:** The ordering relative to patch 4 is problematic. As written, this would break Audio PD initialization when applied without patch 4. Additionally, the check is applied unconditionally but the cover letter only discusses Audio PD.

---

---
Generated by Claude Code Patch Reviewer

  parent reply	other threads:[~2026-06-04  3:11 UTC|newest]

Thread overview: 15+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2026-06-02  7:17 [PATCH v7 0/5] misc: fastrpc: Add missing bug fixes Jianping Li
2026-06-02  7:17 ` [PATCH v7 1/5] misc: fastrpc: Fix initial memory allocation for Audio PD memory pool Jianping Li
2026-06-04  3:11   ` Claude review: " Claude Code Review Bot
2026-06-02  7:17 ` [PATCH v7 2/5] misc: fastrpc: Remove buffer from list prior to unmap operation Jianping Li
2026-06-04  3:11   ` Claude review: " Claude Code Review Bot
2026-06-02  7:17 ` [PATCH v7 3/5] misc: fastrpc: Fail Audio PD init when reserved memory is missing Jianping Li
2026-06-02  9:25   ` Jie Gan
2026-06-04  3:11   ` Claude Code Review Bot [this message]
2026-06-02  7:17 ` [PATCH v7 4/5] misc: fastrpc: Allocate entire reserved memory for Audio PD in probe Jianping Li
2026-06-02  9:41   ` Jie Gan
2026-06-04  3:11   ` Claude review: " Claude Code Review Bot
2026-06-02  7:17 ` [PATCH v7 5/5] misc: fastrpc: Allow fastrpc_buf_free() to accept NULL Jianping Li
2026-06-04  3:11   ` Claude review: " Claude Code Review Bot
2026-06-04  3:11 ` Claude review: misc: fastrpc: Add missing bug fixes Claude Code Review Bot
  -- strict thread matches above, loose matches on Subject: below --
2026-05-15 12:42 [PATCH v5 0/5] " Jianping Li
2026-05-15 12:42 ` [PATCH v5 3/5] misc: fastrpc: Fail Audio PD init when reserved memory is missing Jianping Li
2026-05-15 23:09   ` Claude review: " Claude Code Review Bot

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=review-patch3-20260602071750.526-4-jianping.li@oss.qualcomm.com \
    --to=claude-review@example.com \
    --cc=dri-devel-reviews@example.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox