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/amdgpu: Add HAINAN clock adjustment
Date: Wed, 11 Feb 2026 16:34:52 +1000
Message-ID: <review-patch1-20260210072309.14911-1-decce6@proton.me>
In-Reply-To: <20260210072309.14911-1-decce6@proton.me>
References: <20260210072309.14911-1-decce6@proton.me> <20260210072309.14911-1-decce6@proton.me>
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: 8bit
X-Mailer: Claude Code Patch Reviewer

Patch Review

**Commit Message Issues:**

The commit message is too brief and lacks critical information:

```
This patch limits the clock speeds of the AMD Radeon R5 M420 GPU from
850/1000MHz (core/memory) to 800/950 MHz, making it work stably. This
patch is for amdgpu.
```

**Problems:**
- "making it work stably" - What were the symptoms? Crashes? Corruption? Hangs?
- No mention of how these values were determined
- No testing methodology described
- No reference to bug reports or user complaints
- "This patch is for amdgpu" is redundant (obvious from the subsystem prefix)

**Required Information:**
- What specific stability problems occurred at stock clocks?
- How were the reduced clock values determined? (Bisection? Hardware specs? Vendor recommendation?)
- What testing was performed to validate stability?
- How many systems were tested?
- Any performance impact quantification?

**Code Review:**

```c
+		if ((adev->pdev->device == 0x666f) &&
+		    (adev->pdev->revision == 0x00)) {
+			max_sclk = 80000;
+			max_mclk = 95000;
+		}
```

**Issues:**

1. **Magic numbers without documentation:**
   - `0x666f` - Should have a #define or at least a comment identifying this as HAINAN/R5 M420
   - `80000` and `95000` - These values (800MHz and 950MHz in 10kHz units) need documentation explaining why these specific values

2. **Formatting inconsistency:**
   - Uses tabs for indentation (correct for kernel style)
   - Matches surrounding code style (good)
   - Alignment is consistent with the existing HAINAN block above

3. **Placement:**
   - The code is placed correctly within the CHIP_HAINAN conditional block
   - Follows the pattern established by the existing device-specific override above it
   - Good: Placed after the general HAINAN case, allowing specific revision override

4. **Logic concern:**
   - Only targets revision 0x00. Are there other revisions of this device ID that don't need this limit?
   - Should this apply to all R5 M420 cards regardless of revision?
   - The existing code above shows a pattern of handling multiple revisions together

**Technical Questions:**

1. Where did the original 850/1000MHz values come from? The code doesn't show them - are they in the VBIOS or power tables?

2. Why these specific reduced values (800/950)? Were other values tested?

3. Does this affect all power states or just the maximum?

4. Have you verified this doesn't break anything else (e.g., temperature management, fan curves)?

**Comparison with existing code:**

Looking at the existing HAINAN workaround above (lines 82-84 in context):
```c
		if ((adev->pdev->device == 0x6660) ||
		    (adev->pdev->subsystem_device == 0x8d3)) {
			max_sclk = 60000;
			max_mclk = 80000;
		}
```

This shows a pattern of clock limiting for problematic HAINAN variants. However:
- The existing code checks subsystem_device in addition to device ID
- Should you also check subsystem_device to be more specific?

**Required Changes:**

1. **Expand commit message** to include:
   - Detailed symptom description
   - How values were determined
   - Testing performed
   - Link to bug report or lore discussion
   - Hardware identification details

2. **Add code comments** explaining:
   - What device 0x666f is (R5 M420)
   - Why these specific clock limits are needed
   - Whether this is based on hardware erratum or field observations

3. **Consider** checking subsystem_device for more precise targeting if there are known-good variants with the same device ID

4. **Verification needed:**
   - Confirm this is the right approach vs. VBIOS/SBIOS update
   - Check if AMD has documentation or recommendations for this hardware
   - Verify no other revisions of 0x666f need the same treatment

**Example improved commit message:**

```
drm/amdgpu: Limit clocks for unstable HAINAN R5 M420 (0x666f rev 0x00)

The AMD Radeon R5 M420 (HAINAN, PCI ID 0x666f revision 0x00) exhibits
system instability when running at its default maximum clock speeds of
850MHz core and 1000MHz memory. Symptoms include [specific symptoms:
GPU hangs, display corruption, system crashes, etc.].

Reduce maximum clocks to 800MHz core and 950MHz memory, which testing
has shown to be stable across [testing details: workloads tested,
duration, number of systems].

This follows the pattern of existing HAINAN-specific clock limits for
other problematic variants.

Link: [bug report or discussion]
Tested-by: [tester name/email if applicable]
Signed-off-by: decce6 <decce6@proton.me>
```

**Verdict: NEEDS REVISION**

The code change itself is straightforward and follows existing patterns, but the patch needs substantial improvement in documentation and justification before it can be accepted.

---
Generated by Claude Code Patch Reviewer