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 OP-TEE PAS service support Date: Mon, 25 May 2026 18:55:29 +1000 Message-ID: In-Reply-To: <20260522115936.201208-1-sumit.garg@kernel.org> References: <20260522115936.201208-1-sumit.garg@kernel.org> X-Mailer: Claude Code Patch Reviewer Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 7bit MIME-Version: 1.0 Overall Series Review Subject: firmware: qcom: Add OP-TEE PAS service support Author: Sumit Garg Patches: 16 Reviewed: 2026-05-25T18:55:29.059603 --- This is a well-structured v7 series that introduces a generic Peripheral Authentication Service (PAS) abstraction layer for Qualcomm platforms, allowing multiple TrustZone backends (SCM/QTEE and OP-TEE) to be plugged in. The series follows a clean pattern: introduce the generic layer (patch 2), wire up SCM as a backend (patch 3), add the TEE backend (patch 4), migrate all client drivers (patches 5-13), remove the now-unused SCM wrappers (patch 14), and add MAINTAINERS (patch 15). The overall design is sound and the migration is mechanical and correct across the client drivers. The series has good test coverage (Tested-by tags for Lemans and IPQ9650) and appropriate acks from subsystem maintainers. However, there are several concerns, primarily around the global singleton `ops_ptr` pattern, race conditions during registration, and some issues in the TEE backend error handling. **Key concerns:** 1. **Global singleton `ops_ptr` with no locking**: Only one PAS backend can be active at a time. The registration uses `smp_store_release`/`smp_load_acquire` for visibility but has a TOCTOU race in `qcom_pas_ops_register()` -- the check-then-set is not atomic. If both SCM and TEE backends probe simultaneously, one could silently fail to register with only a `pr_err`. 2. **No module dependency between qcom_pas.c and backends**: `qcom_pas.c` is a module with no `init`/`probe` -- it's just a library of exported symbols. The `ops_ptr` is a module-scoped global. If `qcom_pas.ko` is unloaded while a backend is registered, the ops pointer becomes dangling. 3. **TEE `get_rsc_table` has subtle issues**: The first `tee_client_invoke_func` call is made without any shared memory backing `param[1].u.memref.shm`, which may not be valid for all TEE implementations. 4. **Client drivers still `select QCOM_SCM`**: Several migrated drivers (e.g., DRM_MSM, VIDEO_QCOM_IRIS, QCOM_Q6V5_PAS) still `select QCOM_SCM` alongside the new `select QCOM_PAS`. This is presumably intentional since those drivers still use other SCM APIs, but it means the SCM backend will always register, and the TEE backend will always fail to register when both are built. --- Generated by Claude Code Patch Reviewer