From mboxrd@z Thu Jan 1 00:00:00 1970 From: Claude Code Review Bot To: dri-devel-reviews@example.com Subject: Claude review: drm/virtio: add timeout to virtqueue wait to avoid hung task Date: Sat, 16 May 2026 13:58:17 +1000 Message-ID: In-Reply-To: <20260512-virtio-gpu_wait_event-v1-1-207eb4c1a69a@redhat.com> References: <20260512-virtio-gpu_wait_event-v1-1-207eb4c1a69a@redhat.com> X-Mailer: Claude Code Patch Reviewer Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Overall Series Review Subject: drm/virtio: add timeout to virtqueue wait to avoid hung task Author: Ryosuke Yasuoka Patches: 7 Reviewed: 2026-05-16T13:58:17.315172 --- This is a single patch addressing a real syzbot-reported hung task problem = where `wait_event()` can block indefinitely in `virtio_gpu_queue_ctrl_sgs()= ` and `virtio_gpu_queue_cursor()` when the host stops processing commands. = The approach =E2=80=94 replacing `wait_event()` with `wait_event_timeout()`= =E2=80=94 is a reasonable direction, but the patch has several issues that= need to be addressed before it's mergable. **Key concerns:** 1. **The 5-second timeout is arbitrary and can trigger under legitimate loa= d.** A busy virtio-gpu host might legitimately take more than 5 seconds to = drain the queue under heavy load. This would cause spurious `-ENODEV` error= s and dropped cursor updates in normal operation. The commit message says "= consistent with the existing timeout pattern in the driver" but doesn't cit= e which existing timeout it refers to. A more robust approach might be to i= ntegrate with the device-level health checking (e.g., only bail out if the = device has actually been unplugged/reset, or use a much longer timeout). 2. **The `goto again` placement after timeout is correct but subtle.** If `= wait_event_timeout` returns non-zero (success), execution falls through to = `goto again`, which re-acquires the lock and re-checks the condition. This = is correct behavior =E2=80=94 matching the original `wait_event` + `goto ag= ain` flow =E2=80=94 but the diff placement makes it easy to misread. 3. **No logging on timeout.** When the timeout fires in `virtio_gpu_queue_c= trl_sgs`, the driver silently returns `-ENODEV`. There should be at least a= `drm_err()` or `drm_dbg()` message so the failure is diagnosable =E2=80=94= especially since this path could trigger spuriously. --- --- Generated by Claude Code Patch Reviewer