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/amd/display: use READ_ONCE() in psr_su_set_dsc_slice_height() Date: Tue, 26 May 2026 07:36:07 +1000 Message-ID: In-Reply-To: References: X-Mailer: Claude Code Patch Reviewer Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Patch Review **Positive aspects:** 1. The duplicate zero-check is consolidated into a single check, which is c= leaner. 2. The local `num_slices_v` variable ensures the value used in the division= matches the value that was checked, eliminating the static checker warning. 3. The commit message correctly notes the caller ignores the return value, = justifying the change from `return true` to `return false` for the zero cas= e. **Concerns:** 1. **READ_ONCE() alone doesn't fix a real race.** If `num_slices_v` can gen= uinely change concurrently, then `v_addressable`, `v_border_top`, and `v_bo= rder_bottom` are read from the same `stream->timing` without any protection: ```c pic_height =3D stream->timing.v_addressable + stream->timing.v_border_top + stream->timing.v_border_bottom; ``` A concurrent update to the timing struct could make `pic_height` and `nu= m_slices_v` inconsistent with each other. `READ_ONCE()` on one field create= s a false sense of safety. If this is truly racy, proper locking is needed. 2. **Behavioral change in return value.** The original code returned `true`= when `num_slices_v =3D=3D 0` (in the early compound guard). The patch now = returns `false`. The commit message acknowledges this and correctly notes t= he only caller discards the return value (line 90). This is fine today, but= if a future caller starts checking the return value, it could be surprisin= g that the semantics changed. A brief inline comment on the `return false` = wouldn't hurt, but isn't essential. 3. **The commit message says "there is a race condition" but this is specul= ative.** The duplicate check (commit `21fc0ff38f57`) was more likely added = as a defensive guard against a NULL/zero path that was hit during testing = =E2=80=94 not necessarily due to concurrency. The original fix commit messa= ge says "fix a divided-by-zero error", not "fix a race". The `READ_ONCE()` = may be solving the wrong problem. That said, it doesn't make things worse. **Minor nit:** The commit message says "In the original code we returns true" =E2=80=94 mi= nor grammar ("we return" or "it returns"). **Verdict:** Low risk, acceptable as a static-checker cleanup. The `READ_ON= CE()` is harmless but shouldn't be taken as evidence that the race theory i= s correct or that this area is now concurrency-safe. If real races exist he= re, a follow-up with proper locking would be needed. --- Generated by Claude Code Patch Reviewer