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: vgaarb: pass errors from pci_set_vga_state() up
Date: Mon, 09 Mar 2026 08:05:36 +1000
Message-ID: <review-patch2-20260307173538.763188-3-Simon.Richter@hogyros.de>
In-Reply-To: <20260307173538.763188-3-Simon.Richter@hogyros.de>
References: <20260307173538.763188-1-Simon.Richter@hogyros.de> <20260307173538.763188-3-Simon.Richter@hogyros.de>
X-Mailer: Claude Code Patch Reviewer
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0

Patch Review

**Verdict: Correct but has a concern.**

The approach of wrapping the error in `ERR_PTR()` to fit the existing point=
er return type of `__vga_tryget()` is reasonable. The `vga_get()` caller is=
 properly updated with `IS_ERR()`/`PTR_ERR()` checks.

However, there is a missed caller: `vga_tryget()` at `drivers/pci/vgaarb.c:=
529`:

```c
if (__vga_tryget(vgadev, rsrc))
    rc =3D -EBUSY;
```

After this patch, `__vga_tryget()` can return `ERR_PTR(-EIO)` (or other err=
or pointers). Since `ERR_PTR()` returns a non-NULL pointer, this truthiness=
 check won't crash =E2=80=94 it correctly enters the error branch. But it w=
ill unconditionally report `-EBUSY` instead of the actual error code (e.g.,=
 `-EIO`). This masks the real failure reason. It should be updated to somet=
hing like:

```c
conflict =3D __vga_tryget(vgadev, rsrc);
if (IS_ERR(conflict))
    rc =3D PTR_ERR(conflict);
else if (conflict)
    rc =3D -EBUSY;
```

**State inconsistency concern:** When `pci_set_vga_state()` fails while dis=
abling a conflicting device (line 311-313), the function returns immediatel=
y. At this point, earlier conflicting devices in the loop iteration may hav=
e already had their VGA state disabled and their `owns` bits cleared (line =
314). When the error propagates up and `vga_get()` breaks out of its loop, =
no rollback occurs =E2=80=94 those previously-disabled conflicts remain dis=
abled with no owner. This is arguably acceptable since the alternative (com=
plex rollback logic) is fragile, and the VGA arbiter state may self-correct=
 on the next successful `vga_get()`. But it's worth the author considering =
whether a `dev_warn()` or similar diagnostic would be appropriate when this=
 happens.

---
Generated by Claude Code Patch Reviewer