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: Wed, 27 May 2026 14:17:08 +1000
Message-ID: <review-patch3-20260526192814.179673-4-dongwon.kim@intel.com>
In-Reply-To: <20260526192814.179673-4-dongwon.kim@intel.com>
References: <20260526192814.179673-1-dongwon.kim@intel.com> <20260526192814.179673-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

**Overall:** The notifier approach correctly gates the restore logic behind=
 PM_HIBERNATION_PREPARE. The VIRGL 3D check is a reasonable guard since vir=
gl resources can't be restored.

**Issue 1 =E2=80=94 `no_cb` parameter should be `bool`, not `int`:**

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

All callers pass `true` or `false`. Using `bool` would be more idiomatic an=
d self-documenting.

**Issue 2 =E2=80=94 Unnecessary `#include <linux/pm_runtime.h>`:**

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

Nothing from `pm_runtime.h` is used. Only `<linux/suspend.h>` is needed for=
 `register_pm_notifier()` and `PM_HIBERNATION_PREPARE`.

**Issue 3 =E2=80=94 `virtio_gpu_notify` called per-object inside the loop:**

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 `virtio_gpu_notify` kicks the virtqueue. It could be called once after =
the loop instead of per-object. Not a correctness issue since this is the h=
ibernation path, but it's cleaner.

**Issue 4 =E2=80=94 Missing braces around multi-statement loop body:**

```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);
    }
```

Kernel coding style requires braces when the loop body is more than a simpl=
e statement. The `if` block should be wrapped:

```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);
    }
}
```

**Issue 5 =E2=80=94 `hibernation` flag not cleared on cancelled hibernation=
:**

If PM_HIBERNATION_PREPARE fires but hibernation is aborted before freeze is=
 called, the `hibernation` flag remains `true` indefinitely. It's only clea=
red in `virtgpu_restore()`. While this is practically harmless (since `.fre=
eze`/`.restore` are only called during S4 and the next S4 attempt would set=
 the flag again anyway), handling `PM_POST_HIBERNATION` to clear the flag w=
ould be cleaner:

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

**Issue 6 =E2=80=94 Error recovery in freeze with hibernation (same as patc=
h 1):**

Patch 3 adds `virtio_gpu_object_unref_all()` before flush_work:

```c
if (vgdev->hibernation)
    virtio_gpu_object_unref_all(vgdev);

flush_work(&vgdev->obj_free_work);
```

If the subsequent `wait_queue` calls fail, the objects have already been un=
reffed on the host side but the error path doesn't attempt to re-create the=
m. This could leave the driver in an inconsistent state where guest objects=
 exist but host resources have been destroyed. Combined with the missing `d=
rm_mode_config_helper_resume` from patch 1, a timeout during hibernation fr=
eeze would leave the driver in a broken state.

---
Generated by Claude Code Patch Reviewer