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 generic PAS service Date: Tue, 28 Apr 2026 14:49:22 +1000 Message-ID: In-Reply-To: <20260427095603.1157963-3-sumit.garg@kernel.org> References: <20260427095603.1157963-1-sumit.garg@kernel.org> <20260427095603.1157963-3-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 Patch Review This is the core abstraction patch. Several observations: **Race condition between ops access and unregistration.** Every exported function does a bare pointer check then dereferences through it: ```c int qcom_pas_init_image(u32 pas_id, const void *metadata, size_t size, struct qcom_pas_context *ctx) { if (!ops_ptr) return -ENODEV; return ops_ptr->init_image(ops_ptr->dev, pas_id, metadata, size, ctx); } ``` If `qcom_pas_ops_unregister()` sets `ops_ptr = NULL` between the check and the dereference, this is a NULL pointer dereference. The `smp_load_acquire`/`smp_store_release` in `qcom_pas_is_available()` and `qcom_pas_ops_register()` only provide memory ordering, not mutual exclusion. In practice, shutdown is serialized with driver usage, so this is unlikely to trigger, but it would be more robust to use `rcu_dereference`/`rcu_assign_pointer` (with an `rcu_read_lock()` around each call) or an `srcu` approach. At minimum, each exported function should do: ```c struct qcom_pas_ops *ops = smp_load_acquire(&ops_ptr); if (!ops) return -ENODEV; return ops->init_image(ops->dev, ...); ``` to snapshot the pointer once rather than reading the global twice. **Stale kerneldoc reference.** The `qcom_pas_prepare_and_auth_reset()` doc says: ``` * @ctx: Context saved during call to qcom_scm_pas_context_init() ``` This should reference `devm_qcom_pas_context_alloc()` instead. **`qcom_pas_ops_register()` silently fails.** If called when ops are already registered, it prints `pr_err` but otherwise does nothing and returns void. There's no way for the caller to know it failed. Consider returning an int error code, or at least using `WARN_ON`. **`drv_name` field in `qcom_pas_ops` is never used.** It's set but never referenced -- dead code. --- Generated by Claude Code Patch Reviewer