From mboxrd@z Thu Jan 1 00:00:00 1970 From: Claude Code Review Bot To: dri-devel-reviews@example.com Subject: Claude review: firmware: qcom: Add a PAS TEE service Date: Fri, 13 Mar 2026 14:26:32 +1000 Message-ID: In-Reply-To: <20260312062756.694390-5-sumit.garg@kernel.org> References: <20260312062756.694390-1-sumit.garg@kernel.org> <20260312062756.694390-5-sumit.garg@kernel.org> X-Mailer: Claude Code Patch Reviewer Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Patch Review The OP-TEE backend implementation. Notes: **Error message copy-paste bug in `qcom_pas_tee_set_remote_state()`.** At `= qcom_pas_tee.c:351`: ```c dev_err(dev, "PAS shutdown failed, pas_id: %d, err: %x\n", ``` This function sets remote state, not shutdown. The error message should say= "PAS set remote state failed". **`qcom_pas_tee_get_rsc_table()` =E2=80=94 first invocation with NULL shm.*= * The first `tee_client_invoke_func` call at line 235 is done with `param[1= ].u.memref.shm` left as NULL/zero (no shm is set), only `param[1].u.memref.= size =3D input_rt_size`. This appears to be a size query, but it's not well= -documented. If `input_rt_size` is 0, `param[1]` is MEMREF_INOUT with size = 0 and no shm =E2=80=94 behavior depends on the OP-TEE PTA implementation. **Missing NULL check after `tee_shm_get_va` returns non-ERR NULL.** At line= 254: ```c if (IS_ERR_OR_NULL(rt_shm_va)) { ``` Good =E2=80=94 handles both ERR and NULL. But should it log which case occu= rred? **`ret =3D 0` initialization in `qcom_pas_tee_shutdown()` is unnecessary** = since `ret` is immediately assigned from `tee_client_invoke_func`. **`DEFINE_FREE(shm_free, ...)` scope.** The `__free(shm_free)` cleanup is u= sed for `rt_shm` inside `qcom_pas_tee_get_rsc_table()`. This means the shm = is freed when it goes out of scope, which is correct since the data is copi= ed to `rt_buf` via `kmalloc`+`memcpy`. Good pattern. **No `data->dev` is actually used.** The `qcom_pas_tee_private` struct has = a `dev` field, but it's never assigned or used =E2=80=94 `dev` is always ob= tained from the function parameter. --- Generated by Claude Code Patch Reviewer