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/gem-shmem: Refactor drm_gem_shmem_try_map_pmd()
Date: Wed, 11 Feb 2026 17:04:33 +1000
Message-ID: <review-patch4-20260209133241.238813-5-tzimmermann@suse.de>
In-Reply-To: <20260209133241.238813-5-tzimmermann@suse.de>
References: <20260209133241.238813-1-tzimmermann@suse.de> <20260209133241.238813-5-tzimmermann@suse.de>
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: 8bit
X-Mailer: Claude Code Patch Reviewer

Patch Review

**Review:**

This refactors the PMD mapping function:

**Function rename:**
```c
-drm_gem_shmem_try_map_pmd() -> drm_gem_shmem_try_insert_pfn_pmd()
```

Better name that parallels `vmf_insert_pfn_pmd()`.

**Parameter simplification:**
```c
-static vm_fault_t drm_gem_shmem_try_map_pmd(struct vm_fault *vmf, unsigned long addr,
-                                            struct page *page)
+static vm_fault_t drm_gem_shmem_try_insert_pfn_pmd(struct vm_fault *vmf, unsigned long pfn)
```

**Issue 1 - Lost parameter:**
The old version took `addr` parameter which could differ from `vmf->address`. The new version always uses `vmf->address`:

```c
-	bool aligned = (addr & ~PMD_MASK) == (paddr & ~PMD_MASK);
+	bool aligned = (vmf->address & ~PMD_MASK) == (paddr & ~PMD_MASK);
```

Looking at the caller in previous patches:
```c
drm_gem_shmem_try_map_pmd(vmf, vmf->address, page)
```

So `addr` was always `vmf->address` anyway. This simplification is correct.

**Issue 2 - Moved check:**
```c
-	if (aligned &&
-	    pmd_none(*vmf->pmd) &&
-	    folio_test_pmd_mappable(page_folio(page))) {
+	if (aligned && pmd_none(*vmf->pmd)) {
```

The `folio_test_pmd_mappable()` check moved to the caller:
```c
+	folio = page_folio(page);
+	pfn = page_to_pfn(page);
+
+	if (folio_test_pmd_mappable(folio))
+		ret = drm_gem_shmem_try_insert_pfn_pmd(vmf, pfn);
```

**This introduces a subtle behavior change:** Previously, if the folio wasn't PMD mappable, the function would not even attempt PMD insertion. Now, `drm_gem_shmem_try_insert_pfn_pmd()` will be called but might fail differently.

Wait, looking more carefully:
```c
if (folio_test_pmd_mappable(folio))
    ret = drm_gem_shmem_try_insert_pfn_pmd(vmf, pfn);
if (ret != VM_FAULT_NOPAGE)
    ret = vmf_insert_pfn(vma, vmf->address, pfn);
```

**Bug - Uninitialized variable:** If `folio_test_pmd_mappable(folio)` is false, `ret` is never set before the second `if` statement! The code would use whatever value `ret` had from earlier:

```c
ret = drm_gem_shmem_try_insert_pfn_pmd(vmf, pfn);  // Only executed if PMD mappable
if (ret != VM_FAULT_NOPAGE)  // Uses uninitialized 'ret' if not PMD mappable!
    ret = vmf_insert_pfn(vma, vmf->address, pfn);
```

Actually, wait. Looking at the full context from PATCH 2:
```c
vm_fault_t ret = VM_FAULT_SIGBUS;  // Initialized at function start
...
ret = drm_gem_shmem_try_insert_pfn_pmd(vmf, pfn);
```

So if `folio_test_pmd_mappable()` is false:
- `ret` stays as `VM_FAULT_SIGBUS`
- `ret != VM_FAULT_NOPAGE` is true
- Falls through to `vmf_insert_pfn()`
- Overwrites `ret` with the PFN insertion result

Actually this works correctly, but it's confusing logic. Would be clearer as:
```c
if (folio_test_pmd_mappable(folio))
    ret = drm_gem_shmem_try_insert_pfn_pmd(vmf, pfn);
else
    ret = 0;  // Explicitly signal no PMD

if (ret != VM_FAULT_NOPAGE)
    ret = vmf_insert_pfn(vma, vmf->address, pfn);
```

**Issue 3 - New comment:**
```c
+		/* Read-only mapping; split upon write fault */
```

This comment suggests the PMD mapping is read-only and will split on write. Looking at the call:
```c
return vmf_insert_pfn_pmd(vmf, pfn, false);  // false = no write
```

The `false` parameter means the PMD is inserted without write permission. **But where is the write fault handled?** I don't see `pfn_mkwrite` for PMD entries. This might cause issues with dirty tracking for huge pages.

**Verdict:** ⚠ Confusing control flow and potential issue with PMD write faults

---

---
Generated by Claude Code Patch Reviewer