[PATCH] drm/amd/display: use READ_ONCE() in psr_su_set_dsc_slice_height()

[PATCH] drm/amd/display: use READ_ONCE() in psr_su_set_dsc_slice_height()

[Dan Carpenter]

This code has two checks for if "stream->timing.dsc_cfg.num_slices_v" is
zero so static checkers complain.  The second check was added based on
real life crashes so it suggests there is a race condition.  Use
READ_ONCE() to fix this more reliably.

In the original code we returns true for the first zero check and false
for the second check.  The caller doesn't care about returns so it
doesn't matter whether we return true or false.

Fixes: 21fc0ff38f57 ("drm/amd/display: fix a divided-by-zero error")
Signed-off-by: Dan Carpenter <error27@gmail.com>
---
Untested.  Just reviewing static checker warnings.  I wanted a chance
to use READ_ONCE().

 .../gpu/drm/amd/display/modules/power/power_psr.c   | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/amd/display/modules/power/power_psr.c b/drivers/gpu/drm/amd/display/modules/power/power_psr.c
index 5ecb570c204e..92c0aed08170 100644
--- a/drivers/gpu/drm/amd/display/modules/power/power_psr.c
+++ b/drivers/gpu/drm/amd/display/modules/power/power_psr.c
@@ -635,22 +635,23 @@ bool psr_su_set_dsc_slice_height(struct dc *dc, struct dc_link *link,
 {
 	uint32_t pic_height;
 	uint32_t slice_height;
+	uint32_t num_slices_v;
 
 	config->dsc_slice_height = 0;
 	if (!(link->connector_signal & SIGNAL_TYPE_EDP) ||
 	    !dc->caps.edp_dsc_support ||
 	    link->panel_config.dsc.disable_dsc_edp ||
-	    !link->dpcd_caps.dsc_caps.dsc_basic_caps.fields.dsc_support.DSC_SUPPORT ||
-	    !stream->timing.dsc_cfg.num_slices_v)
+	    !link->dpcd_caps.dsc_caps.dsc_basic_caps.fields.dsc_support.DSC_SUPPORT)
 		return true;
 
+	num_slices_v = READ_ONCE(stream->timing.dsc_cfg.num_slices_v);
+	if (!num_slices_v)
+		return false;
+
 	pic_height = stream->timing.v_addressable +
 		stream->timing.v_border_top + stream->timing.v_border_bottom;
 
-	if (stream->timing.dsc_cfg.num_slices_v == 0)
-		return false;
-
-	slice_height = pic_height / stream->timing.dsc_cfg.num_slices_v;
+	slice_height = pic_height / num_slices_v;
 	config->dsc_slice_height = (uint16_t)slice_height;
 
 	if (slice_height) {
-- 
2.53.0

Claude review: drm/amd/display: use READ_ONCE() in psr_su_set_dsc_slice_height()

[Claude Code Review Bot]

Overall Series Review

Subject: drm/amd/display: use READ_ONCE() in psr_su_set_dsc_slice_height()
Author: Dan Carpenter <error27@gmail.com>
Patches: 1
Reviewed: 2026-05-26T07:36:07.237075

---

This is a single patch from Dan Carpenter addressing a static checker warning about a potential divide-by-zero in `psr_su_set_dsc_slice_height()`. The original code checked `stream->timing.dsc_cfg.num_slices_v` for zero twice — once in the early-return guard and again before the division — suggesting a possible race. The patch consolidates both checks into one, using `READ_ONCE()` to snapshot the value.

The patch is **reasonable but the race-condition narrative is questionable**. The function operates on a `dc_stream_state` that is typically set up during mode-set and not concurrently modified. If there were a genuine race on `num_slices_v`, `READ_ONCE()` on that single field wouldn't be sufficient — the neighboring fields (`v_addressable`, `v_border_top`, `v_border_bottom`) read from the same `stream->timing` could also be stale or torn. A real TOCTOU here would need broader locking or a full structure snapshot, not just `READ_ONCE()` on one field.

That said, the patch does fix the static checker warning, eliminates the redundant check, and the resulting code is marginally cleaner. It's a low-risk cleanup.

---
Generated by Claude Code Patch Reviewer

Claude review: drm/amd/display: use READ_ONCE() in psr_su_set_dsc_slice_height()

[Claude Code Review Bot]

Patch Review

**Positive aspects:**

1. The duplicate zero-check is consolidated into a single check, which is cleaner.
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 case.

**Concerns:**

1. **READ_ONCE() alone doesn't fix a real race.** If `num_slices_v` can genuinely change concurrently, then `v_addressable`, `v_border_top`, and `v_border_bottom` are read from the same `stream->timing` without any protection:
   ```c
   pic_height = 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 `num_slices_v` inconsistent with each other. `READ_ONCE()` on one field creates 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 == 0` (in the early compound guard). The patch now returns `false`. The commit message acknowledges this and correctly notes the 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 surprising 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 speculative.** The duplicate check (commit `21fc0ff38f57`) was more likely added as a defensive guard against a NULL/zero path that was hit during testing — not necessarily due to concurrency. The original fix commit message 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" — minor grammar ("we return" or "it returns").

**Verdict:** Low risk, acceptable as a static-checker cleanup. The `READ_ONCE()` is harmless but shouldn't be taken as evidence that the race theory is correct or that this area is now concurrency-safe. If real races exist here, a follow-up with proper locking would be needed.

---
Generated by Claude Code Patch Reviewer