Claude review: drm/shmem-helper: Fix huge page mapping in fault handler

[Claude Code Review Bot <claude-review@example.com>]

Patch Review

**Positive aspects:**
- The bug report and commit message are clear and detailed with a full stack trace
- The approach of using `.huge_fault` is architecturally correct — it's exactly how the MM subsystem intends PMD-level faults to be handled for file-backed VMAs
- The `VM_FAULT_FALLBACK` return for unsupported orders is correct

**Issues:**

1. **Needs rebase onto drm-next.** The current drm-next code has been substantially refactored compared to the patch's base (`7b5a49935ae4`). The current code has `drm_gem_shmem_try_insert_pfn_pmd()`, `folio_mark_accessed()`, and `.pfn_mkwrite` — none of which exist in the patch's base. This patch will need to be reworked against the current tree.

2. **Awkward `#ifdef` placement inside `try_insert_pfn()`:**

```c
if (!order) {
    return vmf_insert_pfn(vmf->vma, vmf->address, pfn);
#ifdef CONFIG_ARCH_SUPPORTS_PMD_PFNMAP
} else if (order == PMD_ORDER) {
    ...
    }
#endif
}
```

While syntactically valid, this is hard to read. The `#ifdef` splits an `if/else if` chain, making the control flow confusing. A cleaner approach would be to keep the `#ifdef` wrapping a complete code block rather than splitting a conditional.

3. **Overly generic function name.** `try_insert_pfn()` is a very generic static function name that could easily be confused with core MM functions like `insert_pfn()`. A name like `drm_gem_shmem_insert_pfn()` or keeping the DRM subsystem prefix would be more appropriate for kernel style.

4. **Missing `folio_test_pmd_mappable()` check in the caller.** In the current drm-next code, `folio_test_pmd_mappable()` is checked *before* attempting PMD insertion. In this patch, the check is inside `try_insert_pfn()` for the PMD path. This means for order=PMD_ORDER, the function acquires the resv lock, looks up the page, calls `try_insert_pfn()`, which then checks `folio_test_pmd_mappable()` and may return `VM_FAULT_FALLBACK`. While functionally fine, it means a PMD fault for a non-PMD-mappable folio will take the lock, look up the page, then immediately fall back — slightly less efficient but not a correctness issue.

5. **`VM_FAULT_FALLBACK` from the `.fault` path.** When `drm_gem_shmem_fault()` calls `drm_gem_shmem_any_fault(vmf, 0)` with order=0, and the `!order` branch returns `vmf_insert_pfn()` result, that's fine. But if somehow order 0 fell through to the bottom `return VM_FAULT_FALLBACK`, that would be problematic from the `.fault` handler since `VM_FAULT_FALLBACK` is not a valid return from `.fault` — only from `.huge_fault`. In practice this can't happen because order=0 always takes the first branch, but it's worth noting the code structure allows it theoretically.

6. **Missing `pmd_none()` check.** The original code (and drm-next) checks `pmd_none(*vmf->pmd)` before calling `vmf_insert_pfn_pmd()`. The patch removes this check. When called via `.huge_fault` from `create_huge_pmd()` (mm/memory.c:6414-6417), the MM core already verifies `pmd_none(*vmf.pmd)` before calling `create_huge_pmd()`, so this is safe for the initial fault path. However, for the `wp_huge_pmd()` path (mm/memory.c:6167), `huge_fault` can be called without a `pmd_none` guarantee. Since this mapping passes `write=false` to `vmf_insert_pfn_pmd()`, write faults shouldn't normally hit this, but it would be safer to keep the check or document why it's not needed.

**Summary:** The fix direction is correct and addresses a real bug. The patch needs to be rebased onto drm-next and the `#ifdef` style cleaned up. Consider keeping the `pmd_none()` check for defense-in-depth, and use a more descriptive function name.

---
Generated by Claude Code Patch Reviewer