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: Add PM notifier to restore objects after hibernation
Date: Tue, 05 May 2026 11:00:35 +1000
Message-ID: <review-patch3-20260429204729.993669-4-dongwon.kim@intel.com>
In-Reply-To: <20260429204729.993669-4-dongwon.kim@intel.com>
References: <20260429204729.993669-1-dongwon.kim@intel.com> <20260429204729.993669-4-dongwon.kim@intel.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

This patch ties everything together with the PM notifier and adds the unref=
-before-hibernate / restore-after-resume logic.

**Issue 1 (bug): Use-after-free risk in `virtio_gpu_cmd_unref_resource` wit=
h `no_cb=3Dtrue`**

```c
vbuf->resp_cb_data =3D bo;
ret =3D virtio_gpu_queue_ctrl_buffer(vgdev, vbuf);
if (ret < 0)
    virtio_gpu_cleanup_object(bo);
```

When `no_cb=3Dtrue` (called from `virtio_gpu_object_unref_all`), if `virtio=
_gpu_queue_ctrl_buffer()` fails, `virtio_gpu_cleanup_object(bo)` is called.=
 This frees the BO while it's still on the restore list and still reference=
d by userspace GEM handles. The `no_cb` path needs different error handling=
 =E2=80=94 it should not call `virtio_gpu_cleanup_object()` since the inten=
t is to keep the guest-side object alive. A simple approach:

```c
if (ret < 0 && !no_cb)
    virtio_gpu_cleanup_object(bo);
```

**Issue 2 (style): `no_cb` parameter type and naming**

```c
void virtio_gpu_cmd_unref_resource(struct virtio_gpu_device *vgdev,
                   struct virtio_gpu_object *bo,
                   int no_cb);
```

`int no_cb` should be `bool`. Also, negative-sense parameter names are conf=
using =E2=80=94 `true` means "don't do the callback". Consider `bool skip_c=
leanup` for clarity.

**Issue 3 (style): Missing braces in `virtio_gpu_object_unref_all`**

```c
list_for_each_entry_safe(bo, tmp, &vgdev->obj_restore_list,
             restore_node)
    if (bo->created) {
        virtio_gpu_cmd_unref_resource(vgdev, bo, true);
        virtio_gpu_notify(vgdev);
    }
```

The `list_for_each_entry_safe` macro body should have braces around it per =
kernel coding style when the body is a compound statement. Also, `_safe` is=
 not needed here since entries are not removed from the list during iterati=
on =E2=80=94 plain `list_for_each_entry` would suffice.

**Issue 4 (minor): Unnecessary include**

```c
#include <linux/pm_runtime.h>
```

in `virtgpu_kms.c` =E2=80=94 the code only uses `register_pm_notifier()` / =
`unregister_pm_notifier()` from `<linux/suspend.h>`. `pm_runtime.h` appears=
 unused.

**Issue 5 (minor): Per-object `virtio_gpu_notify` in unref_all loop**

```c
list_for_each_entry_safe(bo, tmp, ...)=20
    if (bo->created) {
        virtio_gpu_cmd_unref_resource(vgdev, bo, true);
        virtio_gpu_notify(vgdev);
    }
```

Calling `virtio_gpu_notify()` after every single unref is inefficient. A si=
ngle notify after the loop would batch the kick to the host.

**Issue 6 (minor): Stale `hibernation` flag after aborted hibernation**

The PM notifier sets `vgdev->hibernation =3D true` on `PM_HIBERNATION_PREPA=
RE` but never clears it on `PM_POST_HIBERNATION` (which fires if hibernatio=
n is aborted). The flag is only cleared in `virtgpu_restore()`. If hibernat=
ion is attempted but aborts before freeze() runs, the flag remains set. Whi=
le this is likely benign in practice (freeze/restore are only called during=
 actual S4), handling `PM_POST_HIBERNATION` to clear the flag would be more=
 robust:

```c
if (mode =3D=3D PM_POST_HIBERNATION)
    vgdev->hibernation =3D false;
```

**Issue 7 (minor): Partial state on restore failure**

In `virtgpu_restore()`, if `virtio_gpu_object_restore_all()` fails:
```c
if (vgdev->hibernation) {
    vgdev->hibernation =3D false;
    error =3D virtio_gpu_object_restore_all(vgdev);
    if (error) {
        DRM_ERROR("Failed to recover virtio-gpu objects\n");
        return error;
    }
}
```

The function returns an error but the device is already marked ready (`virt=
io_device_ready(vdev)`) and virtqueues are established. The error path does=
n't tear down the partially initialized state, though there may not be a cl=
ean way to recover at that point.

---
Generated by Claude Code Patch Reviewer