From mboxrd@z Thu Jan 1 00:00:00 1970 From: Claude Code Review Bot To: dri-devel-reviews@example.com Subject: Claude review: pci: check if VGA decoding was really activated Date: Mon, 09 Mar 2026 08:05:36 +1000 Message-ID: In-Reply-To: <20260307173538.763188-5-Simon.Richter@hogyros.de> References: <20260307173538.763188-1-Simon.Richter@hogyros.de> <20260307173538.763188-5-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: Has a coding style issue.** This is the core functional change =E2=80=94 reading back `PCI_BRIDGE_CONTR= OL` after writing to verify the bridge actually accepted the VGA enable. Th= e logic is correct: only check on `decode =3D=3D true` (enabling), since fa= ilure to disable is less critical. **Coding style bug** at `drivers/pci/pci.c:6203`: ```c if(!(cmd & PCI_BRIDGE_CTL_VGA)) ``` Missing space between `if` and `(`. Kernel coding style requires `if (condi= tion)`. This should be `if (!(cmd & PCI_BRIDGE_CTL_VGA))`. The choice of `-EIO` as the error code is reasonable for a hardware communi= cation issue. One consideration: if there are multiple bridges in the hierarchy and an in= termediate one fails, the bridges already configured earlier in the `while = (bus)` loop will have `PCI_BRIDGE_CTL_VGA` set but the function returns `-E= IO`. The caller (`__vga_tryget`) will propagate the error without undoing t= hose earlier bridge writes. This is similar to the state-consistency concer= n from patch 2 and is probably acceptable in practice, but worth noting. --- Generated by Claude Code Patch Reviewer