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: Wed, 18 Feb 2026 06:47:58 +1000 Message-ID: In-Reply-To: <20260217170419.236739-5-Simon.Richter@hogyros.de> References: <20260217170419.236739-1-Simon.Richter@hogyros.de> <20260217170419.236739-5-Simon.Richter@hogyros.de> X-Mailer: Claude Code Patch Reviewer Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 7bit MIME-Version: 1.0 Patch Review This is the core patch that detects bridges refusing VGA forwarding by reading back `PCI_BRIDGE_CTL_VGA` after writing it. > + if (decode) { > + pci_read_config_word(bridge, PCI_BRIDGE_CONTROL, > + &cmd); > + if(!(cmd & PCI_BRIDGE_CTL_VGA)) > + return -EIO; > + } Missing space between `if` and `(` -- should be `if (!(cmd & PCI_BRIDGE_CTL_VGA))`. More importantly: the `while (bus)` loop walks from the device's immediate parent bridge up toward the root. If bridge N successfully sets `PCI_BRIDGE_CTL_VGA` but bridge N+1 (closer to root) refuses, the function returns `-EIO` immediately, leaving bridge N (and any earlier bridges) with `PCI_BRIDGE_CTL_VGA` still set. The commit message says "I think it should be fine to leave VGA forwarding enabled on lower bridges if a bridge closer to the root refused to enable forwarding, because no accesses can reach there anyway." This reasoning is correct for the *forwarding* direction -- VGA cycles from the CPU won't reach the lower bridge. But is it possible for this stale bit to cause issues if a different device later successfully claims VGA? The next `pci_set_vga_state(true)` call for another device on a different branch would walk a different bridge chain and wouldn't clean up these stale bits. A subsequent `pci_set_vga_state(false)` for the same device *would* clear them (since `decode=false` doesn't do the readback check), so the bits would be cleaned up on the disable path. This seems acceptable but worth noting. The choice of `-EIO` is reasonable for "hardware refused the write." --- Generated by Claude Code Patch Reviewer