Claude review: drm/virtio: abort virtqueue wait on device removal to avoid hung task

[Claude Code Review Bot <claude-review@example.com>]

Patch Review

**Correctness — memory ordering of `vqs_released`**

The `vqs_released` flag is a plain `bool` written on one CPU and read on another (the waiting thread). While `wake_up_all()` does provide a memory barrier on the waking side, and `wait_event()` rechecks the condition after being woken, there's a subtlety: the `wait_event()` macro checks the condition *before* sleeping. If the flag is set just as another CPU is entering the wait path, the plain bool read could theoretically be stale without an explicit barrier or atomic.

In practice, this is likely fine on x86 (strong memory model) and the `wait_event` internal barriers paper over most cases, but for correctness on all architectures, consider using `READ_ONCE(vgdev->vqs_released)` in the wait conditions and `WRITE_ONCE(vgdev->vqs_released, true)` in `virtio_gpu_release_vqs()`. This is the standard kernel pattern for lockless flag communication:

```c
// In virtio_gpu_release_vqs():
WRITE_ONCE(vgdev->vqs_released, true);

// In wait conditions:
wait_event(vgdev->ctrlq.ack_queue,
           vq->num_free >= elemcnt || READ_ONCE(vgdev->vqs_released));
```

This prevents compiler tearing/reordering and is the idiomatic way to do this in the kernel.

**Correctness — the ctrl queue abort path looks correct**

```c
if (vgdev->vqs_released) {
    if (fence && vbuf->objs)
        virtio_gpu_array_unlock_resv(vbuf->objs);
    free_vbuf(vgdev, vbuf);
    drm_dev_exit(idx);
    return -ENODEV;
}
```

This matches the cleanup in the `drm_dev_enter()` failure path at lines 383-388 of the current tree. The `drm_dev_exit(idx)` call is correct since we entered the SRCU critical section earlier. Good.

**Correctness — the cursor queue abort path looks correct**

```c
if (vgdev->vqs_released) {
    free_vbuf(vgdev, vbuf);
    drm_dev_exit(idx);
    return;
}
```

Mirrors the `drm_dev_enter()` failure path at lines 555-558. The cursor path doesn't have fence/objs handling, consistent with the existing code. Good.

**Design — ordering of `virtio_gpu_release_vqs()` vs `drm_dev_unplug()`**

Placing `virtio_gpu_release_vqs()` before `drm_dev_unplug()` is correct — this is the whole point. The flag unblocks waiters so their SRCU critical sections can complete, allowing `synchronize_srcu()` inside `drm_dev_unplug()` to proceed. The ordering is sound.

**Minor — race window between `vqs_released` check and `goto again`/`goto retry`**

In the ctrl queue path, after the `vqs_released` check:
```c
if (vgdev->vqs_released) {
    ...
    return -ENODEV;
}
goto again;
```

If `vqs_released` is false when checked but becomes true before re-entering the `wait_event`, the next iteration will catch it. This is fine — the condition is rechecked in `wait_event`.

**Minor — `vqs_released` initialization**

The bool field is added to `struct virtio_gpu_device`. Since the struct is allocated with `kzalloc` (through `drm_dev_alloc`), it will be zero-initialized, so `vqs_released` starts as `false`. This is correct.

**Style — comment verbosity**

The comment block in `virtio_gpu_queue_ctrl_sgs()`:
```c
/*
 * Set by virtio_gpu_release_vqs() to unblock
 * synchronize_srcu() wait in drm_dev_unplug().
 */
```
This is reasonable for explaining the non-obvious interaction with `drm_dev_unplug()`. The shorter `/* See comment in virtio_gpu_queue_ctrl_sgs(). */` cross-reference in the cursor path is appropriate.

**Overall assessment: Good fix. The `READ_ONCE`/`WRITE_ONCE` suggestion is the main feedback worth acting on. The logic, cleanup paths, and ordering are all correct.**

---
Generated by Claude Code Patch Reviewer