[PATCH v3 0/5] Bridges without VGA support

[PATCH v3 0/5] Bridges without VGA support

[Simon Richter]

Hi,

reformatted the descriptions to follow the style guide, removed unlikely()
from return code checks.

Companion patch for i915/xe is v3 of

  https://patchwork.freedesktop.org/series/161721/

which can be applied independently and should probably go in before the
__must_check patches at least.

Not sure if these should have a Cc stable. If yes, there is a backport of
the i915/xe patch as well (what patchwork calls "v4").

Simon Richter (5):
  vgaarb: pass vga_get_uninterruptible() errors to userspace
  vgaarb: pass errors from pci_set_vga_state() up
  vgaarb: mark vga_get() and wrappers as __must_check
  pci: check if VGA decoding was really activated
  pci: mark pci_set_vga_state() as __must_check

 drivers/pci/pci.c      |  6 ++++++
 drivers/pci/vgaarb.c   | 20 +++++++++++++++++---
 include/linux/pci.h    |  4 ++--
 include/linux/vgaarb.h | 15 ++++++++-------
 4 files changed, 33 insertions(+), 12 deletions(-)

-- 
2.47.3

[PATCH v3 1/5] vgaarb: pass vga_get_uninterruptible() errors to userspace

[Simon Richter]

If VGA routing cannot be established, vga_get_uninterruptible() returns an
error and does not increment the lock count. Pass the error on, and don't
call vga_put() when userspace closes the handle.

Signed-off-by: Simon Richter <Simon.Richter@hogyros.de>
---
 drivers/pci/vgaarb.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/vgaarb.c b/drivers/pci/vgaarb.c
index d9383bf541e7..22b2b6ebdefd 100644
--- a/drivers/pci/vgaarb.c
+++ b/drivers/pci/vgaarb.c
@@ -1134,6 +1134,7 @@ static ssize_t vga_arb_write(struct file *file, const char __user *buf,
 	char kbuf[64], *curr_pos;
 	size_t remaining = count;
 
+	int err;
 	int ret_val;
 	int i;
 
@@ -1165,7 +1166,11 @@ static ssize_t vga_arb_write(struct file *file, const char __user *buf,
 			goto done;
 		}
 
-		vga_get_uninterruptible(pdev, io_state);
+		err = vga_get_uninterruptible(pdev, io_state);
+		if (err) {
+			ret_val = err;
+			goto done;
+		}
 
 		/* Update the client's locks lists */
 		for (i = 0; i < MAX_USER_CARDS; i++) {
-- 
2.47.3

Claude review: vgaarb: pass vga_get_uninterruptible() errors to userspace

[Claude Code Review Bot]

Patch Review

**Verdict: Looks good.**

This patch correctly checks the return value of `vga_get_uninterruptible()` in the `vga_arb_write()` "lock" command handler and returns the error to userspace instead of silently proceeding. The error path via `goto done` is the right existing pattern.

Minor style nit: the new `int err;` declaration is placed before the existing `int ret_val;` with a blank line separating them at lines 1146-1147. Kernel style generally groups declarations together without blank lines in between, so `err` could simply be on the same line or adjacent to `ret_val`. Very minor.

---
Generated by Claude Code Patch Reviewer

[PATCH v3 2/5] vgaarb: pass errors from pci_set_vga_state() up

[Simon Richter]

pci_set_vga_state() returns an error code, which so far has been ignored by
the only caller, __vga_tryget(), so forward it to the caller. As the return
type of __vga_tryget() is a pointer, wrap the error in ERR_PTR().

Signed-off-by: Simon Richter <Simon.Richter@hogyros.de>
---
 drivers/pci/vgaarb.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/drivers/pci/vgaarb.c b/drivers/pci/vgaarb.c
index 22b2b6ebdefd..c360eee11dd9 100644
--- a/drivers/pci/vgaarb.c
+++ b/drivers/pci/vgaarb.c
@@ -215,6 +215,7 @@ static struct vga_device *__vga_tryget(struct vga_device *vgadev,
 	struct vga_device *conflict;
 	unsigned int pci_bits;
 	u32 flags = 0;
+	int err;
 
 	/*
 	 * Account for "normal" resources to lock. If we decode the legacy,
@@ -307,7 +308,9 @@ static struct vga_device *__vga_tryget(struct vga_device *vgadev,
 		if (change_bridge)
 			flags |= PCI_VGA_STATE_CHANGE_BRIDGE;
 
-		pci_set_vga_state(conflict->pdev, false, pci_bits, flags);
+		err = pci_set_vga_state(conflict->pdev, false, pci_bits, flags);
+		if (err)
+			return ERR_PTR(err);
 		conflict->owns &= ~match;
 
 		/* If we disabled normal decoding, reflect it in owns */
@@ -337,7 +340,9 @@ static struct vga_device *__vga_tryget(struct vga_device *vgadev,
 	if (wants & VGA_RSRC_LEGACY_MASK)
 		flags |= PCI_VGA_STATE_CHANGE_BRIDGE;
 
-	pci_set_vga_state(vgadev->pdev, true, pci_bits, flags);
+	err = pci_set_vga_state(vgadev->pdev, true, pci_bits, flags);
+	if (err)
+		return ERR_PTR(err);
 
 	vgadev->owns |= wants;
 lock_them:
@@ -455,6 +460,10 @@ int vga_get(struct pci_dev *pdev, unsigned int rsrc, int interruptible)
 		}
 		conflict = __vga_tryget(vgadev, rsrc);
 		spin_unlock_irqrestore(&vga_lock, flags);
+		if (IS_ERR(conflict)) {
+			rc = PTR_ERR(conflict);
+			break;
+		}
 		if (conflict == NULL)
 			break;
 
-- 
2.47.3

Claude review: vgaarb: pass errors from pci_set_vga_state() up

[Claude Code Review Bot]

Patch Review

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

The approach of wrapping the error in `ERR_PTR()` to fit the existing pointer 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 = -EBUSY;
```

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

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

**State inconsistency concern:** When `pci_set_vga_state()` fails while disabling a conflicting device (line 311-313), the function returns immediately. At this point, earlier conflicting devices in the loop iteration may have 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 — those previously-disabled conflicts remain disabled with no owner. This is arguably acceptable since the alternative (complex 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

[PATCH v3 3/5] vgaarb: mark vga_get() and wrappers as __must_check

[Simon Richter]

The vga_get() function and the two wrappers vga_get_interruptible() and
vga_get_uninterruptible() can return errors. As these are paired with
vga_put(), which must only be called after vga_get() returned success, all
callers need to check the return code.

Signed-off-by: Simon Richter <Simon.Richter@hogyros.de>
---
 include/linux/vgaarb.h | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

diff --git a/include/linux/vgaarb.h b/include/linux/vgaarb.h
index 97129a1bbb7d..eed524c67c22 100644
--- a/include/linux/vgaarb.h
+++ b/include/linux/vgaarb.h
@@ -27,7 +27,8 @@ struct pci_dev;
 
 #ifdef CONFIG_VGA_ARB
 void vga_set_legacy_decoding(struct pci_dev *pdev, unsigned int decodes);
-int vga_get(struct pci_dev *pdev, unsigned int rsrc, int interruptible);
+int __must_check vga_get(struct pci_dev *pdev, unsigned int rsrc,
+			 int interruptible);
 void vga_put(struct pci_dev *pdev, unsigned int rsrc);
 struct pci_dev *vga_default_device(void);
 void vga_set_default_device(struct pci_dev *pdev);
@@ -39,8 +40,8 @@ static inline void vga_set_legacy_decoding(struct pci_dev *pdev,
 		unsigned int decodes)
 {
 };
-static inline int vga_get(struct pci_dev *pdev, unsigned int rsrc,
-		int interruptible)
+static inline int __must_check vga_get(struct pci_dev *pdev, unsigned int rsrc,
+				       int interruptible)
 {
 	return 0;
 }
@@ -74,8 +75,8 @@ static inline int vga_client_register(struct pci_dev *pdev,
  *
  * On success, release the VGA resource again with vga_put().
  */
-static inline int vga_get_interruptible(struct pci_dev *pdev,
-					unsigned int rsrc)
+static inline int __must_check vga_get_interruptible(struct pci_dev *pdev,
+						     unsigned int rsrc)
 {
 	return vga_get(pdev, rsrc, 1);
 }
@@ -89,8 +90,8 @@ static inline int vga_get_interruptible(struct pci_dev *pdev,
  *
  * On success, release the VGA resource again with vga_put().
  */
-static inline int vga_get_uninterruptible(struct pci_dev *pdev,
-					  unsigned int rsrc)
+static inline int __must_check vga_get_uninterruptible(struct pci_dev *pdev,
+						       unsigned int rsrc)
 {
 	return vga_get(pdev, rsrc, 0);
 }
-- 
2.47.3

Claude review: vgaarb: mark vga_get() and wrappers as __must_check

[Claude Code Review Bot]

Patch Review

**Verdict: Looks good.**

Straightforward annotation change. All three functions (`vga_get()`, `vga_get_interruptible()`, `vga_get_uninterruptible()`) and their `!CONFIG_VGA_ARB` stubs are annotated. This will cause compile warnings for any in-tree callers that ignore the return value, which is the right approach given patches 1 and 2 now make these errors meaningful.

The cover letter mentions companion i915/xe patches to handle the new warnings, which makes sense since those drivers call `vga_get_uninterruptible()`.

---
Generated by Claude Code Patch Reviewer

[PATCH v3 4/5] pci: check if VGA decoding was really activated

[Simon Richter]

PCI bridges are allowed to refuse activating VGA decoding, by simply
ignoring attempts to set the bit that enables it, so after setting the bit,
read it back to verify.

One example of such a bridge is the root bridge in IBM PowerNV, but this is
also useful for GPU passthrough into virtual machines, where it is
difficult to set up routing for legacy IO through IOMMU.

Signed-off-by: Simon Richter <Simon.Richter@hogyros.de>
---
 drivers/pci/pci.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 8479c2e1f74f..e60b948f8576 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -6197,6 +6197,12 @@ int pci_set_vga_state(struct pci_dev *dev, bool decode,
 				cmd &= ~PCI_BRIDGE_CTL_VGA;
 			pci_write_config_word(bridge, PCI_BRIDGE_CONTROL,
 					      cmd);
+			if (decode) {
+				pci_read_config_word(bridge, PCI_BRIDGE_CONTROL,
+						     &cmd);
+				if(!(cmd & PCI_BRIDGE_CTL_VGA))
+					return -EIO;
+			}
 		}
 		bus = bus->parent;
 	}
-- 
2.47.3

Claude review: pci: check if VGA decoding was really activated

[Claude Code Review Bot]

Patch Review

**Verdict: Has a coding style issue.**

This is the core functional change — reading back `PCI_BRIDGE_CONTROL` after writing to verify the bridge actually accepted the VGA enable. The logic is correct: only check on `decode == true` (enabling), since failure 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 (condition)`. This should be `if (!(cmd & PCI_BRIDGE_CTL_VGA))`.

The choice of `-EIO` as the error code is reasonable for a hardware communication issue.

One consideration: if there are multiple bridges in the hierarchy and an intermediate one fails, the bridges already configured earlier in the `while (bus)` loop will have `PCI_BRIDGE_CTL_VGA` set but the function returns `-EIO`. The caller (`__vga_tryget`) will propagate the error without undoing those earlier bridge writes. This is similar to the state-consistency concern from patch 2 and is probably acceptable in practice, but worth noting.

---
Generated by Claude Code Patch Reviewer

[PATCH v3 5/5] pci: mark pci_set_vga_state() as __must_check

[Simon Richter]

This function can return an error, usually on non-x86 or virtualized
environments.

The only caller so far is __vga_tryget() in vgaarb.

Signed-off-by: Simon Richter <Simon.Richter@hogyros.de>
---
 include/linux/pci.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/include/linux/pci.h b/include/linux/pci.h
index 1c270f1d5123..aa1451d402d1 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -1720,8 +1720,8 @@ resource_size_t pcibios_window_alignment(struct pci_bus *bus,
 #define PCI_VGA_STATE_CHANGE_BRIDGE (1 << 0)
 #define PCI_VGA_STATE_CHANGE_DECODES (1 << 1)
 
-int pci_set_vga_state(struct pci_dev *pdev, bool decode,
-		      unsigned int command_bits, u32 flags);
+int __must_check pci_set_vga_state(struct pci_dev *pdev, bool decode,
+				   unsigned int command_bits, u32 flags);
 
 /*
  * Virtual interrupts allow for more interrupts to be allocated
-- 
2.47.3

Claude review: pci: mark pci_set_vga_state() as __must_check

[Claude Code Review Bot]

Patch Review

**Verdict: Looks good.**

Straightforward `__must_check` annotation. Since patch 2 already converted the only caller (`__vga_tryget`) to check the return value, this won't introduce new warnings. Good defensive measure to catch any future callers that might ignore the error.

---
Generated by Claude Code Patch Reviewer

Claude review: Bridges without VGA support

[Claude Code Review Bot]

Overall Series Review

Subject: Bridges without VGA support
Author: Simon Richter <Simon.Richter@hogyros.de>
Patches: 6
Reviewed: 2026-03-09T08:05:36.022574

---

This is a well-motivated 5-patch series that adds proper error propagation for VGA arbitration failures, targeting environments where PCI bridges cannot activate VGA decoding (IBM PowerNV root bridges, GPU passthrough VMs). The series follows a logical progression: first fix error handling, then propagate errors upward, then add `__must_check` annotations, then add the actual hardware readback check, and finally annotate the lower-level function.

The series is generally clean and correct, but there are two notable issues: a coding style problem in patch 4, and a subtle semantic concern with `vga_tryget()` masking the real error code after patch 2.

---

---
Generated by Claude Code Patch Reviewer