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: Add polling mode support for fastRPC driver
Date: Thu, 23 Apr 2026 07:50:44 +1000
Message-ID: <review-patch4-20260422092409.4107093-5-ekansh.gupta@oss.qualcomm.com>
In-Reply-To: <20260422092409.4107093-5-ekansh.gupta@oss.qualcomm.com>
References: <20260422092409.4107093-1-ekansh.gupta@oss.qualcomm.com> <20260422092409.4107093-5-ekansh.gupta@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

**Assessment: Has several issues that need addressing.**

**Issue 1 (Build break): `of_machine_get_match` does not exist**

```c
data->poll_mode_supported =3D soc_data->poll_mode_supported ||
    of_machine_get_match(fastrpc_poll_supported_machines);
```

The kernel has `of_machine_device_match()` (returns `bool`) and `of_machine=
_get_match_data()` (returns `const void *`). Since you only need a boolean =
"is this machine in the list", use `of_machine_device_match()`:

```c
data->poll_mode_supported =3D soc_data->poll_mode_supported ||
    of_machine_device_match(fastrpc_poll_supported_machines);
```

**Issue 2: `readl_poll_timeout_atomic` on DMA buffer memory**

```c
ret =3D readl_poll_timeout_atomic(ctx->poll_addr, val,
                (val =3D=3D FASTRPC_POLL_RESPONSE) || ctx->is_work_done, 1,
                FASTRPC_POLL_MAX_TIMEOUT_US);
```

`ctx->poll_addr` points into `ctx->buf->virt`, which is a DMA coherent buff=
er allocated via `dma_alloc_coherent()`, not an MMIO register. `readl()` is=
 intended for MMIO and may apply `__iomem` annotations or arch-specific I/O=
 accessors that are inappropriate for regular memory.

Use `read_poll_timeout(READ_ONCE, ...)` or a custom polling loop with `READ=
_ONCE()` instead. This also avoids the atomic context restriction (see Issu=
e 3).

**Issue 3: 10ms atomic busy-wait**

`readl_poll_timeout_atomic` uses `udelay()` and disables preemption for the=
 entire duration. With `FASTRPC_POLL_MAX_TIMEOUT_US =3D 10000` (10ms), this=
 is a very long time to hold a CPU non-preemptible. The non-atomic variant =
(`readl_poll_timeout` / `read_poll_timeout`) uses `usleep_range()` which is=
 much more scheduler-friendly, and this code path is in process context (io=
ctl handler), so sleeping is fine.

**Issue 4: Data race on `ctx->is_work_done`**

`ctx->is_work_done` is read in the polling condition and written from `fast=
rpc_rpmsg_callback` (interrupt context) without any synchronization:

```c
// In poll_for_remote_response (process context):
(val =3D=3D FASTRPC_POLL_RESPONSE) || ctx->is_work_done

// In fastrpc_rpmsg_callback (interrupt context):
ctx->is_work_done =3D true;
```

A plain `bool` is not guaranteed to be atomically visible across CPUs. This=
 should use `READ_ONCE(ctx->is_work_done)` in the polling condition and `WR=
ITE_ONCE(ctx->is_work_done, true)` in the callback (or use an atomic flag).

Similarly in `poll_for_remote_response`:
```c
if (!ret && val =3D=3D FASTRPC_POLL_RESPONSE) {
    ctx->is_work_done =3D true;
```
This write should also use `WRITE_ONCE`.

**Issue 5: `ctx->retval =3D 0` in poll path may race with callback**

In `poll_for_remote_response`:
```c
if (!ret && val =3D=3D FASTRPC_POLL_RESPONSE) {
    ctx->is_work_done =3D true;
    ctx->retval =3D 0;
}
```

The callback also sets `ctx->retval =3D rsp->retval`. If the DSP writes the=
 poll response *and* sends an rpmsg response, both paths could write `retva=
l` concurrently. The poll path hardcodes `retval =3D 0`, which may not matc=
h the actual DSP return value. If the DSP puts the real return value in the=
 rpmsg response, blindly setting it to 0 in the poll path loses error infor=
mation.

**Issue 6: Missing `dma_rmb()` after polling**

The existing code has a `dma_rmb()` barrier between receiving the completio=
n and calling `fastrpc_put_args()`:
```c
/* make sure that all memory writes by DSP are seen by CPU */
dma_rmb();
```

After the polling path succeeds, the same barrier is needed to ensure the C=
PU sees all DSP writes to the output buffers. The current code does fall th=
rough to `fastrpc_put_args()` after `fastrpc_wait_for_completion()` returns=
, but the `dma_rmb()` is placed after the original wait code. Verify the ba=
rrier is still hit on the polling-success path =E2=80=94 looking at the pat=
ched flow, `fastrpc_wait_for_completion()` returns 0, then execution contin=
ues to the existing `dma_rmb()` + `fastrpc_put_args()`, so this should be f=
ine.

**Issue 7: `__maybe_unused` on the match table**

```c
static const struct of_device_id fastrpc_poll_supported_machines[] __maybe_=
unused =3D {
```

This suggests it may be unused when `CONFIG_OF` is off. But `of_machine_dev=
ice_match()` has a stub returning `false` when `!CONFIG_OF`, so the table w=
ould indeed be unused. A cleaner approach would be to wrap the call with `I=
S_ENABLED(CONFIG_OF)` or just accept that this driver practically requires =
OF.

**Issue 8: IOCTL number 12 skipped for `SET_OPTION`**

The existing header has ioctl numbers 1-11 and then 13. The new `FASTRPC_IO=
CTL_SET_OPTION` is assigned number 12, which fills the gap. This is fine an=
d intentional.

**Issue 9: `FASTRPC_POLL_MODE` value documentation says "handle > 20" but c=
ode uses `FASTRPC_MAX_STATIC_HANDLE`**

The UAPI header comment says:
```c
 * - Only applies to dynamic modules (handle > 20)
```

And the code uses:
```c
if (handle > FASTRPC_MAX_STATIC_HANDLE && ...)
```

Where `FASTRPC_MAX_STATIC_HANDLE` is defined as 20. This is consistent, but=
 hardcoding "20" in the UAPI comment is fragile. Consider referencing the m=
acro name instead.

**Minor: `fastrpc_wait_for_response` is marked `inline`**

```c
static inline int fastrpc_wait_for_response(...)
```

The `inline` keyword is unnecessary =E2=80=94 the compiler will decide. Ker=
nel style generally avoids explicit `inline` on static functions unless the=
re's a specific reason.

---
Generated by Claude Code Patch Reviewer