From mboxrd@z Thu Jan  1 00:00:00 1970
From: Claude Code Review Bot <claude-review@example.com>
To: dri-devel-reviews@example.com
Subject: Claude review: drm/msm/a8xx: Make a8xx_recover IFPC safe
Date: Sun, 12 Apr 2026 13:46:55 +1000
Message-ID: <review-patch5-20260407-gfx-clk-fixes-v1-5-4bb5583a5054@oss.qualcomm.com>
In-Reply-To: <20260407-gfx-clk-fixes-v1-5-4bb5583a5054@oss.qualcomm.com>
References: <20260407-gfx-clk-fixes-v1-0-4bb5583a5054@oss.qualcomm.com> <20260407-gfx-clk-fixes-v1-5-4bb5583a5054@oss.qualcomm.com>
X-Mailer: Claude Code Patch Reviewer
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0

Patch Review

This correctly gates GX MMIO access behind a `gx_is_on()` check, preventing bus errors when GX is power-collapsed:

```c
+	if (adreno_gpu->funcs->gx_is_on(adreno_gpu)) {
+		gpu_write(gpu, REG_A8XX_CP_SQE_CNTL, 3);
+
+		if (hang_debug)
+			a8xx_dump(gpu);
+	}
```

**Ordering difference from a6xx_recover.** The commit message says "matching a6xx behavior", but the ordering differs. In `a6xx_recover()` (line 1648-1662 of `a6xx_gpu.c`), the sequence is:

```c
// a6xx_recover:
adreno_dump_info(gpu);
if (gx_is_on) { ... }     // GX check first
a6xx_gpu->hung = true;      // hung set after
```

In the patched `a8xx_recover`:
```c
// a8xx_recover (patched):
adreno_dump_info(gpu);
a6xx_gpu->hung = true;      // hung set first
if (gx_is_on) { ... }       // GX check after
```

The `hung` flag is set *before* the `gx_is_on` check in a8xx but *after* in a6xx. While this probably doesn't cause a functional problem (the flag is consumed during the subsequent RPM suspend), it would be better to match a6xx's ordering exactly if that's the stated intent. Consider moving `a6xx_gpu->hung = true` after the `gx_is_on` block.

---
Generated by Claude Code Patch Reviewer