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_scm: Migrate to generic PAS service Date: Mon, 09 Mar 2026 08:59:07 +1000 Message-ID: In-Reply-To: <20260306105027.290375-4-sumit.garg@kernel.org> References: <20260306105027.290375-1-sumit.garg@kernel.org> <20260306105027.290375-4-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 **Unsafe type punning.** This patch uses raw C-style casts between `struct = qcom_scm_pas_context *` and `struct qcom_pas_context *` throughout: ```c return (struct qcom_scm_pas_context *)ctx; ``` ```c return __qcom_scm_pas_init_image2(__scm->dev, pas_id, metadata, size, (struct qcom_pas_context *)ctx); ``` These two structs are defined independently with identical layouts. This is= fragile =E2=80=94 if either struct gains or loses a field, the cast silent= ly produces UB. A better approach would be to make `qcom_scm_pas_context` a= typedef or contain a `qcom_pas_context` as its first member (composition),= or even just use the new type directly in the transitional wrappers since = they're going away in patch 14 anyway. **The `devm_qcom_scm_pas_context_alloc` wrapper is duplicating `devm_qcom_p= as_context_alloc` logic.** Since patch 2 already provides the generic alloc= ator, the SCM wrapper could just call it and cast the result, rather than r= e-implementing the allocation. This would also eliminate one source of dive= rgence. The overall structure of the patch (extract `__` static functions, keep exp= orted wrappers, register ops at probe) is reasonable for the transitional p= eriod. --- Generated by Claude Code Patch Reviewer