[PATCH 0/5] bridges without VGA support

[PATCH 0/5] bridges without VGA support

[Simon Richter]

Hi,

this allows bridges to refuse forwarding VGA, and reports this upwards as an
error, because we cannot set up valid decoding for the requested device in this
case.

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.

   Simon

Simon Richter (5):
  vgaarb: pass vga_get errors to userspace
  vgaarb: pass errors from pci_set_vga_state up
  vgaarb: mark vga_get family as __must_check
  pci: check if VGA decoding was really activated
  pci: mark return value of pci_set_vga_state as __must_check

 drivers/pci/pci.c      |  6 ++++++
 drivers/pci/vgaarb.c   | 22 +++++++++++++++++++---
 include/linux/pci.h    |  2 +-
 include/linux/vgaarb.h |  9 +++++----
 4 files changed, 31 insertions(+), 8 deletions(-)


base-commit: 9702969978695d9a699a1f34771580cdbb153b33
-- 
2.47.3

[PATCH 1/5] vgaarb: pass vga_get errors to userspace

[Simon Richter]

If vga_get fails, return the error code via the write syscall.

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

diff --git a/drivers/pci/vgaarb.c b/drivers/pci/vgaarb.c
index 87143e235033..5c2719cb1bfa 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,12 @@ 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 (unlikely(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 errors to userspace

[Claude Code Review Bot]

Patch Review

This patch makes the `vga_arb_write` handler check the return value of `vga_get_uninterruptible` and return the error to userspace instead of silently proceeding.

> +	int err;
>  	int ret_val;
>  	int i;

Minor style note: the blank line between `err` and `ret_val` is odd -- these are related local variables and would normally be declared together.

> +		err = vga_get_uninterruptible(pdev, io_state);
> +		if (unlikely(err))
> +		{
> +			ret_val = err;
> +			goto done;
> +		}

The opening brace should be on the same line as the `if`, per kernel coding style (`if (unlikely(err)) {`). This pattern appears in multiple patches in the series.

Functionally this patch is correct -- prior to this series `vga_get_uninterruptible` could already return `-ENODEV` or `-ERESTARTSYS`, so checking the return value here fixes a pre-existing bug where the "lock" command would report success to userspace even when `vga_get` failed, and would then call `vga_put` on a resource that was never acquired (via the lock counter update that follows). So this is a good fix.

---
Generated by Claude Code Patch Reviewer

[PATCH 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. Pass
this code through __vga_tryget (via ERR_PTR).

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

diff --git a/drivers/pci/vgaarb.c b/drivers/pci/vgaarb.c
index 5c2719cb1bfa..322c028539f0 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 = 0;
 
 	/*
 	 * 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 (unlikely(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 (unlikely(err))
+		return ERR_PTR(err);
 
 	vgadev->owns |= wants;
 lock_them:
@@ -455,6 +460,11 @@ 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

This patch converts `__vga_tryget` to propagate `pci_set_vga_state` errors via `ERR_PTR`, and updates `vga_get` to handle the new error pointer returns.

> +		err = pci_set_vga_state(conflict->pdev, false, pci_bits, flags);
> +		if (unlikely(err))
> +			return ERR_PTR(err);
>  		conflict->owns &= ~match;

When `pci_set_vga_state(conflict->pdev, false, ...)` fails, the function returns immediately. At this point, VGA state for the *requesting* device has not been enabled yet, and the conflict device's `owns` bits have not been updated, so the data structures remain consistent. However, if this is not the first iteration of the `list_for_each_entry` loop, earlier iterations may have already successfully disabled VGA on other conflicting devices (via `pci_set_vga_state(false)`) and cleared their `owns` bits. Those devices have lost VGA ownership in the software state and in hardware, but the requesting device never got it. This leaves the system in a state where nobody owns VGA. Is this an acceptable outcome, or should the series attempt to roll back the changes to previously-processed conflicts?

> +		if (IS_ERR(conflict))
> +		{
> +			rc = PTR_ERR(conflict);
> +			break;
> +		}

Same brace style issue. The `IS_ERR` check in `vga_get` is correct.

However, `vga_tryget()` also calls `__vga_tryget` and is **not updated** by this patch:

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

After this patch, `__vga_tryget` can return `ERR_PTR(-EIO)` (or similar). Since `ERR_PTR` values are non-NULL and non-zero, this `if` evaluates to true and `vga_tryget` returns `-EBUSY` instead of the actual error code. This is a bug -- `vga_tryget` should be updated to distinguish between a real conflict (non-NULL, non-ERR pointer) and an error (ERR_PTR). Something like:

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

---
Generated by Claude Code Patch Reviewer

[PATCH 3/5] vgaarb: mark vga_get family as __must_check

[Simon Richter]

These functions can return an error, but some callers expect they don't, and
unconditionally access VGA registers afterwards and call vga_put.

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

diff --git a/include/linux/vgaarb.h b/include/linux/vgaarb.h
index 97129a1bbb7d..de683d499dfa 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,7 +40,7 @@ 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,
+static inline int __must_check vga_get(struct pci_dev *pdev, unsigned int rsrc,
 		int interruptible)
 {
 	return 0;
@@ -74,7 +75,7 @@ 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,
+static inline int __must_check vga_get_interruptible(struct pci_dev *pdev,
 					unsigned int rsrc)
 {
 	return vga_get(pdev, rsrc, 1);
@@ -89,7 +90,7 @@ 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,
+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 family as __must_check

[Claude Code Review Bot]

Patch Review

This patch adds `__must_check` to `vga_get`, `vga_get_interruptible`, and `vga_get_uninterruptible`.

The change is reasonable in principle, but it will cause new compiler warnings for existing callers that don't check the return value. Specifically, `drivers/gpu/drm/i915/display/intel_vga.c` has two call sites:

```c
vga_get_uninterruptible(pdev, VGA_RSRC_LEGACY_IO);
```

at lines 68 and 93, neither of which checks the return value. Adding `__must_check` without fixing these callers will introduce build warnings (or errors with `-Werror`). These callers should be fixed in this series, or at minimum the commit message should acknowledge the known warnings.

---
Generated by Claude Code Patch Reviewer

[PATCH 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.

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 f3244630bfd0..0984a0aefb88 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -6198,6 +6198,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

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

[PATCH 5/5] pci: mark return value of pci_set_vga_state as __must_check

[Simon Richter]

This function can return an error, which should be checked.

The only caller so far is __vga_tryget in vgaarb, which did not check.

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

diff --git a/include/linux/pci.h b/include/linux/pci.h
index 1c270f1d5123..08d7c1ae33b5 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -1720,7 +1720,7 @@ 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,
+int __must_check pci_set_vga_state(struct pci_dev *pdev, bool decode,
 		      unsigned int command_bits, u32 flags);
 
 /*
-- 
2.47.3

Claude review: pci: mark return value of pci_set_vga_state as __must_check

[Claude Code Review Bot]

Patch Review

> -int pci_set_vga_state(struct pci_dev *pdev, bool decode,
> +int __must_check pci_set_vga_state(struct pci_dev *pdev, bool decode,

The commit message says "The only caller so far is __vga_tryget in vgaarb, which did not check." But patch 2 already added error checking to `__vga_tryget`, so at this point in the series the only caller *does* check. The commit message is slightly misleading -- it describes the pre-series state but is applied after patch 2 has already fixed it. Consider rewording to say the caller now checks (as of patch 2), so this annotation is safe to add.

No functional issues with this patch.

---
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-02-18T06:47:57.209916

---

This series by Simon Richter adds error propagation for VGA arbitration failures, motivated by PCI bridges (such as the root bridge in IBM PowerNV) that silently refuse to enable VGA forwarding. The approach is: (1) detect bridge refusal by reading back the VGA control bit after writing it, (2) propagate the error through `pci_set_vga_state` -> `__vga_tryget` -> `vga_get` -> userspace, and (3) add `__must_check` annotations to encourage callers to handle the new failure mode.

The series has a logical structure and addresses a real problem. However, there are two concrete bugs: `vga_tryget()` is not updated to handle the new `ERR_PTR` returns from `__vga_tryget()`, which means it will misreport errors as `-EBUSY`; and the early return from `pci_set_vga_state` when a bridge refuses VGA decode leaves lower bridges with VGA forwarding enabled, creating an inconsistent hardware state. The series also introduces several coding style violations (missing space before `(`, opening brace placement on `if` blocks) that are inconsistent with kernel conventions. The `__must_check` annotations in patches 3 and 5 will generate new compiler warnings for the i915 driver, which doesn't check return values of `vga_get_uninterruptible`.

---
Generated by Claude Code Patch Reviewer