From mboxrd@z Thu Jan 1 00:00:00 1970 From: Claude Code Review Bot To: dri-devel-reviews@example.com Subject: Claude review: misc: fastrpc: Add cache maintenance for non-coherent platforms Date: Fri, 05 Jun 2026 05:54:35 +1000 Message-ID: In-Reply-To: <20260604194811.2437567-1-abhinav.parihar@oss.qualcomm.com> References: <20260604194811.2437567-1-abhinav.parihar@oss.qualcomm.com> <20260604194811.2437567-1-abhinav.parihar@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 **BUG (Critical): Direction mismatch in `fastrpc_inv_args()`** ```c if (ctx->olaps[i].mstart) { dma_buf_begin_cpu_access(map->buf, DMA_FROM_DEVICE); dma_buf_end_cpu_access(map->buf, DMA_TO_DEVICE); } ``` The `begin_cpu_access` uses `DMA_FROM_DEVICE` (invalidate =E2=80=94 correct= for output buffers being read by CPU), but `end_cpu_access` uses `DMA_TO_D= EVICE` instead of `DMA_FROM_DEVICE`. The direction passed to `end_cpu_acces= s` must match the direction used in `begin_cpu_access` =E2=80=94 these are = paired calls bracketing a CPU access with a specific direction. Using `DMA_= TO_DEVICE` in the end call means the exporter receives a mismatched directi= on, which can result in the wrong cache operation (a clean instead of an in= validate) or no operation at all, depending on the exporter implementation.= This should be: ```c dma_buf_begin_cpu_access(map->buf, DMA_FROM_DEVICE); dma_buf_end_cpu_access(map->buf, DMA_FROM_DEVICE); ``` **Issue (Medium): Questionable cache maintenance pattern** Both `fastrpc_flush_args()` and `fastrpc_inv_args()` immediately pair `begi= n_cpu_access` + `end_cpu_access` with no actual CPU access in between: ```c dma_buf_begin_cpu_access(map->buf, DMA_TO_DEVICE); dma_buf_end_cpu_access(map->buf, DMA_TO_DEVICE); ``` The dma-buf API documentation states: *"Only when cpu access is bracketed b= y both calls is it guaranteed to be coherent with other DMA access."* The i= ntended usage is: `begin` =E2=86=92 perform CPU reads/writes =E2=86=92 `end= `. Here the CPU access (the copy_from_user into the buffer) has *already ha= ppened* before `fastrpc_flush_args()` is called, so the bracket is empty. W= hile this may work on some exporters as a side-effect (the `end_cpu_access`= callback might do a cache clean), it's relying on implementation details r= ather than the documented contract. The correct approach would be to call `= dma_buf_begin_cpu_access` *before* the CPU writes to the buffer, and `dma_b= uf_end_cpu_access` after. Similarly for the invalidation path =E2=80=94 `be= gin_cpu_access(DMA_FROM_DEVICE)` should be called before `fastrpc_put_args`= reads the output data, and `end_cpu_access` after. **Issue (Medium): Return values of dma_buf_{begin,end}_cpu_access are ignor= ed** Both `dma_buf_begin_cpu_access()` and `dma_buf_end_cpu_access()` return `in= t` error codes. Failures are silently ignored: ```c dma_buf_begin_cpu_access(map->buf, DMA_TO_DEVICE); dma_buf_end_cpu_access(map->buf, DMA_TO_DEVICE); ``` At minimum, `begin_cpu_access` failures should be checked and, if failing, = the `end_cpu_access` call skipped (and preferably the error propagated upwa= rd, since `fastrpc_get_args` already returns int). **Issue (Minor): `uintptr_t` cast on kernel virtual address in `fastrpc_inv= _args`** ```c if (((uintptr_t)rpra & PAGE_MASK) =3D=3D ((uintptr_t)rpra[raix].buf.pv & PAGE_MASK)) ``` `rpra` is a kernel pointer, `rpra[raix].buf.pv` is a `u64` from the remote = arg union. Casting a kernel pointer to `uintptr_t` and masking it works but= is a bit fragile =E2=80=94 would be clearer to compare against the buffer'= s known DMA address or page frame, and it's worth a comment explaining that= `buf.pv` here holds a kernel virtual address within the same allocation as= `rpra`. **Observation: `coherent` field not propagated to duplicate sessions** The `coherent` field is set per session in `fastrpc_cb_probe()`: ```c sess->coherent =3D of_property_read_bool(dev->of_node, "dma-coherent"); ``` The duplicate sessions created in the loop below are copied via `memcpy(dup= _sess, sess, sizeof(*dup_sess))`, so they *do* inherit the `coherent` flag.= This is correct. **Nit: Filtering logic in `fastrpc_flush_args` could be simplified** ```c if (raix + 1 > inbufs) continue; ``` This is equivalent to `raix >=3D inbufs` which would be more idiomatic. Sim= ilarly in `fastrpc_inv_args`: `raix + 1 <=3D inbufs` is `raix < inbufs`. Wh= ile functionally correct, the `+1` pattern is slightly confusing. **Summary of required changes:** 1. Fix `dma_buf_end_cpu_access` direction to `DMA_FROM_DEVICE` in `fastrpc_= inv_args()` =E2=80=94 this is a correctness bug. 2. Check return values of `dma_buf_begin_cpu_access()`. 3. Reconsider the placement of begin/end brackets to properly surround the = actual CPU access rather than being empty pairs. --- Generated by Claude Code Patch Reviewer