From mboxrd@z Thu Jan 1 00:00:00 1970 From: Claude Code Review Bot To: dri-devel-reviews@example.com Subject: Claude review: soc: qcom: rpmh-rsc: Skip TCS init when RSC is managed by firmware Date: Mon, 25 May 2026 19:16:36 +1000 Message-ID: In-Reply-To: <20260522060645.4399-2-graham.oconnor@gmail.com> References: <20260522060645.4399-1-graham.oconnor@gmail.com> <20260522060645.4399-2-graham.oconnor@gmail.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 **Concept**: When UEFI firmware leaves the RSC hardware solver active, skip= TCS reinitialization to avoid a security violation. **Issue 1 =E2=80=94 Too much is skipped.** Looking at the normal probe path= in `rpmh_rsc_probe()` (rpmh-rsc.c:1077-1131), the early return skips: - `rpmh_probe_tcs_config()` (intended) - Spinlock/waitqueue/bitmap initialization - IRQ registration - Active TCS interrupt enable - Client cache list initialization - **`devm_of_platform_populate()`** =E2=80=94 child nodes (rpmh clients) wi= ll never probe ```c + if (readl_relaxed(drv->base + drv->regs[DRV_SOLVER_CONFIG]) & + (DRV_HW_SOLVER_MASK << DRV_HW_SOLVER_SHIFT)) { + dev_dbg(&pdev->dev, "RSC already managed by firmware, skipping TCS init\= n"); + platform_set_drvdata(pdev, drv); + return 0; + } ``` The `drv` set as driver data has uninitialized `lock`, `tcs_wait`, `tcs_in_= use`, `client.cache`, and `client.batch_cache`. If any consumer obtains thi= s `drv` pointer and attempts an rpmh operation, it will operate on uninitia= lized data. **Issue 2 =E2=80=94 No `devm_of_platform_populate()`.** The commit message = says "allowing other drivers to find the controller," but child platform de= vices of the RSC node won't be created without this call. Is the intent tha= t the entire RSC subsystem should be inert on this platform? If so, the com= mit message should explain that. If children should still probe, then at mi= nimum `devm_of_platform_populate()` needs to be called in the early-return = path. **Issue 3 =E2=80=94 dev_dbg vs dev_info.** Silently skipping the entire RSC= init path is a significant behavioral change. Using `dev_info` or `dev_not= ice` would help debug boot issues. **Suggestion**: If the firmware-managed case truly means the kernel should = not interact with this RSC at all, consider whether probing should succeed = at all, or whether the driver should explicitly populate children while lea= ving TCS alone. The current approach creates a half-initialized driver data= structure. --- --- Generated by Claude Code Patch Reviewer