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/virtio: Fix driver removal with disabled KMS
Date: Fri, 05 Jun 2026 06:30:56 +1000
Message-ID: <review-patch1-20260604122743.13383-1-dmitry.osipenko@collabora.com>
In-Reply-To: <20260604122743.13383-1-dmitry.osipenko@collabora.com>
References: <20260604122743.13383-1-dmitry.osipenko@collabora.com> <20260604122743.13383-1-dmitry.osipenko@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

**Correctness of the fix: Good.**

The guard `drm_core_check_feature(dev, DRIVER_ATOMIC)` is the right check. =
When KMS is disabled, `virtgpu_kms.c:241` clears `DRIVER_ATOMIC` from `dev-=
>driver_features`:

```c
dev->driver_features &=3D ~(DRIVER_MODESET | DRIVER_ATOMIC);
```

And `virtio_gpu_modeset_init()` returns early without calling `drmm_mode_co=
nfig_init()`:

```c
if (!vgdev->num_scanouts)
    return 0;
```

So mode_config mutexes/locks and CRTC/connector lists are never initialized=
. The `drm_atomic_helper_shutdown()` path calls `DRM_MODESET_LOCK_ALL_BEGIN=
` which touches `mode_config.mutex` and modeset locks that were never initi=
alized =E2=80=94 that's the crash.

**Issue 1 =E2=80=94 Patch does not apply to drm-next.** The hunk context at=
 `@@ -138,7 +138,10 @@` includes:

```
 	virtio_gpu_release_vqs(dev);
 	drm_dev_unplug(dev);
```

But in the current drm-next tree (`virtgpu_drv.c:122-130`), `virtio_gpu_rem=
ove` has no `virtio_gpu_release_vqs` call. The patch needs rebasing.

**Issue 2 =E2=80=94 Commit message could mention the root cause more precis=
ely.** The message says "access of uninitialized data" which is accurate bu=
t vague. Mentioning that `drmm_mode_config_init()` is skipped when `num_sca=
nouts =3D=3D 0`, leaving mode_config locks uninitialized, would make the cr=
ash mechanism clearer for future readers.

**Issue 3 =E2=80=94 Consider whether `drm_atomic_helper_shutdown` itself sh=
ould guard.** The `DRM_MODESET_LOCK_ALL_BEGIN` macro already checks `drm_dr=
v_uses_atomic_modeset(dev)` for the mutex path (line 176 of `drm_modeset_lo=
ck.h`):

```c
#define DRM_MODESET_LOCK_ALL_BEGIN(dev, ctx, flags, ret)		\
	if (!drm_drv_uses_atomic_modeset(dev))				\
		mutex_lock(&dev->mode_config.mutex);			\
```

...but it still proceeds unconditionally into `drm_modeset_lock_all_ctx` wi=
th potentially uninitialized locks. This suggests the core helper itself ma=
y be fragile when called on a device that never initialized mode_config. Th=
at's a broader DRM core issue, so fixing it in the driver is reasonable for=
 now, but it might be worth a note or a follow-up to harden `drm_atomic_hel=
per_shutdown` itself.

**Minor style nit:** The added blank lines around the `if` block are fine a=
nd follow kernel conventions, no issue there.

**Verdict:** The logic is correct and the Fixes tag is appropriate. The pat=
ch needs a rebase onto current drm-next to resolve the context mismatch.

---
Generated by Claude Code Patch Reviewer