From mboxrd@z Thu Jan 1 00:00:00 1970 From: Claude Code Review Bot To: dri-devel-reviews@example.com Subject: Claude review: drm/rockchip: vop2: Use vop2->old_layer_sel directly in wait_for_layer_cfg_done() Date: Tue, 05 May 2026 08:08:46 +1000 Message-ID: In-Reply-To: <20260504-vop2-layer-cfg-tmout-v1-5-730226a7331e@collabora.com> References: <20260504-vop2-layer-cfg-tmout-v1-0-730226a7331e@collabora.com> <20260504-vop2-layer-cfg-tmout-v1-5-730226a7331e@collabora.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 **Verdict: Correct but has a subtle consideration.** This removes the `cfg` parameter from `rk3568_vop2_wait_for_layer_cfg_done(= )` since the only caller always passes `vop2->old_layer_sel`. ```diff -static void rk3568_vop2_wait_for_layer_cfg_done(struct vop2 *vop2, u32 cfg) +static void rk3568_vop2_wait_for_layer_cfg_done(struct vop2 *vop2) ``` And inside: ```diff - atv_layer_cfg =3D=3D cfg, 10, 50 * 1000); + atv_layer_cfg =3D=3D vop2->old_layer_sel, 10, 50 * 1000); ``` **Minor concern**: `readx_poll_timeout_atomic` re-evaluates the condition e= xpression on each poll iteration. With the parameter version, the target va= lue `cfg` was a stack copy captured before the loop started. With the new v= ersion, `vop2->old_layer_sel` is read from the struct on every poll iterati= on. If another thread (another VP's atomic commit) were to concurrently mod= ify `vop2->old_layer_sel` while this spin is running, the target would shif= t mid-poll. In practice, this is likely safe because: - The function runs under `vop2->ovl_lock` mutex, and all paths that write = `vop2->old_layer_sel` (after patch 3) also hold that mutex. - So no concurrent writer should be able to change the value during the spi= n. But this is worth noting =E2=80=94 the parameter version was inherently imm= une to this class of bug, while the new version relies on the caller holdin= g the mutex. If a future code path ever calls this function or modifies `ol= d_layer_sel` without the lock, it would introduce a subtle bug. This is a r= easonable tradeoff for code simplicity, but it might be worth a brief comme= nt, or the maintainer may prefer keeping the parameter for robustness. This is a style/maintainability judgment call rather than a correctness bug. --- **Summary**: The series is well-crafted. Patches 1 and 2 are clear, correct= bug fixes suitable for stable backport. Patches 3-5 are clean follow-up re= factors. The only point worth discussing with the author is whether patch 5= 's removal of the explicit parameter is worth the implicit coupling to the = mutex contract =E2=80=94 but it's defensible either way. --- Generated by Claude Code Patch Reviewer