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/shmem-helper: Fix huge page mapping in fault handler
Date: Sun, 22 Mar 2026 04:58:23 +1000
Message-ID: <review-patch1-20260319015224.46896-1-pedrodemargomes@gmail.com>
In-Reply-To: <20260319015224.46896-1-pedrodemargomes@gmail.com>
References: <20260319015224.46896-1-pedrodemargomes@gmail.com> <20260319015224.46896-1-pedrodemargomes@gmail.com>
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:**
- The bug report and commit message are clear and detailed with a full stac=
k trace
- The approach of using `.huge_fault` is architecturally correct =E2=80=94 =
it's exactly how the MM subsystem intends PMD-level faults to be handled fo=
r 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 subst=
antially refactored compared to the patch's base (`7b5a49935ae4`). The curr=
ent code has `drm_gem_shmem_try_insert_pfn_pmd()`, `folio_mark_accessed()`,=
 and `.pfn_mkwrite` =E2=80=94 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 =3D=3D 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 woul=
d be to keep the `#ifdef` wrapping a complete code block rather than splitt=
ing a conditional.

3. **Overly generic function name.** `try_insert_pfn()` is a very generic s=
tatic function name that could easily be confused with core MM functions li=
ke `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 curr=
ent drm-next code, `folio_test_pmd_mappable()` is checked *before* attempti=
ng PMD insertion. In this patch, the check is inside `try_insert_pfn()` for=
 the PMD path. This means for order=3DPMD_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 functi=
onally fine, it means a PMD fault for a non-PMD-mappable folio will take th=
e lock, look up the page, then immediately fall back =E2=80=94 slightly les=
s efficient but not a correctness issue.

5. **`VM_FAULT_FALLBACK` from the `.fault` path.** When `drm_gem_shmem_faul=
t()` calls `drm_gem_shmem_any_fault(vmf, 0)` with order=3D0, and the `!orde=
r` branch returns `vmf_insert_pfn()` result, that's fine. But if somehow or=
der 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 va=
lid return from `.fault` =E2=80=94 only from `.huge_fault`. In practice thi=
s can't happen because order=3D0 always takes the first branch, but it's wo=
rth 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 remo=
ves 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)` befo=
re 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=
=3Dfalse` to `vmf_insert_pfn_pmd()`, write faults shouldn't normally hit th=
is, 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 pat=
ch needs to be rebased onto drm-next and the `#ifdef` style cleaned up. Con=
sider keeping the `pmd_none()` check for defense-in-depth, and use a more d=
escriptive function name.

---
Generated by Claude Code Patch Reviewer