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: Make sure PMD entries get the writeable upgrade
Date: Sun, 22 Mar 2026 03:39:52 +1000
Message-ID: <review-patch1-20260320151914.586945-1-boris.brezillon@collabora.com>
In-Reply-To: <20260320151914.586945-1-boris.brezillon@collabora.com>
References: <20260320151914.586945-1-boris.brezillon@collabora.com> <20260320151914.586945-1-boris.brezillon@collabora.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

**The core fix is correct.** Passing `vmf->flags & FAULT_FLAG_WRITE` to `vm=
f_insert_pfn_pmd()` and recording dirty/mkwrite in the PMD path addresses t=
he issue introduced by commit 28e3918179aa.

**Bug: Error handling regression in `drm_gem_shmem_pfn_mkwrite()`.**

The original code returned `VM_FAULT_SIGBUS` on the error path:

```c
if (drm_WARN_ON(obj->dev, !shmem->pages || page_offset >=3D num_pages))
    return VM_FAULT_SIGBUS;
```

The new `drm_gem_shmem_record_mkwrite()` is a `void` function that simply r=
eturns on the WARN path:

```c
static void drm_gem_shmem_record_mkwrite(struct vm_fault *vmf)
{
    ...
    if (drm_WARN_ON(obj->dev, !shmem->pages || page_offset >=3D num_pages))
        return;
    ...
}
```

And `drm_gem_shmem_pfn_mkwrite()` now unconditionally returns 0 (success):

```c
static vm_fault_t drm_gem_shmem_pfn_mkwrite(struct vm_fault *vmf)
{
    drm_gem_shmem_record_mkwrite(vmf);
    return 0;
}
```

So if the WARN fires (pages are NULL or offset is out of range), the old co=
de signalled `VM_FAULT_SIGBUS` to userspace, but the new code silently succ=
eeds. This should be preserved =E2=80=94 either by making `drm_gem_shmem_re=
cord_mkwrite()` return a `vm_fault_t`, or by keeping the validation inline =
in `drm_gem_shmem_pfn_mkwrite()`.

**Minor: Comment style.** The multi-line comment at the PMD insertion site =
doesn't follow kernel style:

```c
			/* Unlike PTEs which are automatically upgraded to
			 * writeable entries, the PMD upgrades go through
```

Kernel style uses:

```c
			/*
			 * Unlike PTEs which are automatically upgraded to
			 * writeable entries, the PMD upgrades go through
```

**Locking question.** The author raises this themselves in the cover letter=
. `drm_gem_shmem_record_mkwrite()` accesses `shmem->pages[page_offset]` wit=
hout holding `dma_resv_lock`. In the PMD path (called from `drm_gem_shmem_f=
ault()`), the lock is already held, so it's safe. In the PTE `.pfn_mkwrite(=
)` path, the lock is not held =E2=80=94 but this was already the case befor=
e the patch, so it's not a regression. Still, it would be worth confirming =
that `shmem->pages` cannot be freed while an mmap exists (it shouldn't be, =
due to the `pages_use_count` refcount taken in `drm_gem_shmem_mmap()`/`vm_o=
pen()`).

**Summary:** The fix is logically correct and well-motivated. The `VM_FAULT=
_SIGBUS` -> silent success error handling change should be fixed before mer=
ging.

---
Generated by Claude Code Patch Reviewer