[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

[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

[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

[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

[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

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

[Ville Syrjälä]

On Sun, Mar 08, 2026 at 02:35:35AM +0900, Simon Richter wrote:
> 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);

I was thinking this one should never fail and maybe could warrant
a WARN, but uv_set_vga_state() involves some kind of BIOS call and
who knows what that does. So maybe a WARN isn't a good idea.

>  		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

-- 
Ville Syrjälä
Intel

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

[Ville Syrjälä]

On Sun, Mar 08, 2026 at 02:35:37AM +0900, Simon Richter wrote:
> 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;
> +			}

Maybe this should also have a comment or spec quote to explain
that it's legal behavior?

The slightly bigger concern I have is whether we need to unwind
the previous steps if this fails? Looks like we don't update
vgadev->owns on failure (even though we may have partially
enabled things). But since the bridge should never forward any
VGA accesses, leaving some extra PCI_COMMAND enable(s) set on
the device shouldn't matter in practice. So I guess this should
work even without unwinding. Well, not sure about 
arch_set_vga_state() since that's 100% magic...

>  		}
>  		bus = bus->parent;
>  	}
> -- 
> 2.47.3

-- 
Ville Syrjälä
Intel

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

[Simon Richter]

Hi,

On 3/10/26 8:37 PM, Ville Syrjälä wrote:

> Maybe this should also have a comment or spec quote to explain
> that it's legal behavior?

The PCI-to-PCI Bridge Specification version 1.1 has in chapter 4.5 "VGA 
Support":

     Bridges are not required to implement the VGA support mechanisms
     described in the following sections.

So this has always been optional.

What is a bit annoying is that I cannot find an explicit sentence 
allowing the VGAEnable bit to be hardwired to zero in that specification.

It is explicitly allowed to hardwire the VGASnoopEnable bit to zero if 
VGA palette snooping is not supported for downstream devices (Table 3-2 
"Command Register"):

     Implementation of VGA palette snooping by a bridge is optional. If
     VGA palette snooping is not supported, this bit must be implemented
     as read-only with a value of 0

That is the command register though, not the bridge control register.

The bridge control register documentation (Table 3-9) does not 
explicitly say what should be done if VGA support is not available, but 
the behaviour of the VGAEnable bit is a superset of the VGASnoopEnable 
bit, and the VGASnoopEnable bit becomes a Don't Care if VGAEnable is 
set, so "the bit must be hardwired to zero if unsupported" appears to be 
the only valid interpretation.

This seems to be the behaviour of all the devices I have available:

  - IBM PowerNV PCIe root bridge (POWER9)
  - SiFive VisionFive2 PCIe root bridge (PLDA XpressRich-AXI Ref Design)

Both read back this bit as zero when written via setpci:

# setpci -s 0000:00:00.0 BRIDGE_CONTROL
0002
# setpci -s 0000:00:00.0 BRIDGE_CONTROL=0xa
# setpci -s 0000:00:00.0 BRIDGE_CONTROL
0002

There is also a question in the Altera support forum for their PCIe Root 
Bridge IP at https://community.altera.com/kb/knowledge-base/-/345837 .

> The slightly bigger concern I have is whether we need to unwind
> the previous steps if this fails? Looks like we don't update
> vgadev->owns on failure (even though we may have partially
> enabled things). But since the bridge should never forward any
> VGA accesses, leaving some extra PCI_COMMAND enable(s) set on
> the device shouldn't matter in practice.

I was thinking the same, but because we're going upwards in the 
hierarchy, the bridges we leave enabled are downstream of the bridge 
that refused to forward.

I can add this for completeness though, that should be fairly easy.

> So I guess this should
> work even without unwinding. Well, not sure about
> arch_set_vga_state() since that's 100% magic...

It's not entirely magic, because after success from arch_set_vga_state() 
we still go through all the bridges. I believe uv uses it for some kind 
of paravirtualization.

    Simon

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

[Ville Syrjälä]

On Tue, Mar 10, 2026 at 11:08:40PM +0900, Simon Richter wrote:
> Hi,
> 
> On 3/10/26 8:37 PM, Ville Syrjälä wrote:
> 
> > Maybe this should also have a comment or spec quote to explain
> > that it's legal behavior?
> 
> The PCI-to-PCI Bridge Specification version 1.1 has in chapter 4.5 "VGA 
> Support":
> 
>      Bridges are not required to implement the VGA support mechanisms
>      described in the following sections.
> 
> So this has always been optional.
> 
> What is a bit annoying is that I cannot find an explicit sentence 
> allowing the VGAEnable bit to be hardwired to zero in that specification.
> 
> It is explicitly allowed to hardwire the VGASnoopEnable bit to zero if 
> VGA palette snooping is not supported for downstream devices (Table 3-2 
> "Command Register"):
> 
>      Implementation of VGA palette snooping by a bridge is optional. If
>      VGA palette snooping is not supported, this bit must be implemented
>      as read-only with a value of 0
> 
> That is the command register though, not the bridge control register.
> 
> The bridge control register documentation (Table 3-9) does not 
> explicitly say what should be done if VGA support is not available, but 
> the behaviour of the VGAEnable bit is a superset of the VGASnoopEnable 
> bit, and the VGASnoopEnable bit becomes a Don't Care if VGAEnable is 
> set, so "the bit must be hardwired to zero if unsupported" appears to be 
> the only valid interpretation.
> 
> This seems to be the behaviour of all the devices I have available:
> 
>   - IBM PowerNV PCIe root bridge (POWER9)
>   - SiFive VisionFive2 PCIe root bridge (PLDA XpressRich-AXI Ref Design)
> 
> Both read back this bit as zero when written via setpci:
> 
> # setpci -s 0000:00:00.0 BRIDGE_CONTROL
> 0002
> # setpci -s 0000:00:00.0 BRIDGE_CONTROL=0xa
> # setpci -s 0000:00:00.0 BRIDGE_CONTROL
> 0002
> 
> There is also a question in the Altera support forum for their PCIe Root 
> Bridge IP at https://community.altera.com/kb/knowledge-base/-/345837 .

Yeah, looks like the exact explanation was only added in the PCIe
bridge spec. Before that it was somewhat ambiguous.

> 
> > The slightly bigger concern I have is whether we need to unwind
> > the previous steps if this fails? Looks like we don't update
> > vgadev->owns on failure (even though we may have partially
> > enabled things). But since the bridge should never forward any
> > VGA accesses, leaving some extra PCI_COMMAND enable(s) set on
> > the device shouldn't matter in practice.
> 
> I was thinking the same, but because we're going upwards in the 
> hierarchy, the bridges we leave enabled are downstream of the bridge 
> that refused to forward.
> 
> I can add this for completeness though, that should be fairly easy.
> 
> > So I guess this should
> > work even without unwinding. Well, not sure about
> > arch_set_vga_state() since that's 100% magic...
> 
> It's not entirely magic, because after success from arch_set_vga_state() 
> we still go through all the bridges. I believe uv uses it for some kind 
> of paravirtualization.
> 
>     Simon

-- 
Ville Syrjälä
Intel

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

[Bjorn Helgaas]

On Sun, Mar 08, 2026 at 02:35:38AM +0900, Simon Richter wrote:
> This function can return an error, usually on non-x86 or virtualized
> environments.
> 
> The only caller so far is __vga_tryget() in vgaarb.

Is there any reason for pci_set_vga_state() to be in pci.c while the
only caller is in vgaarb.c?  If not, we could move it there, make it
static, and drop it from include/linux/pci.h.

> 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
> 

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

[Bjorn Helgaas]

On Sun, Mar 08, 2026 at 02:35:36AM +0900, Simon Richter wrote:
> 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);

vga_get() is implemented in vgaarb.c and is only used there.  Can we
drop this declaration and the stub below, make it static and
__must_check in vgaarb.c?

>  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;
>  }

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

[Bjorn Helgaas]

On Sun, Mar 08, 2026 at 02:35:37AM +0900, Simon Richter wrote:
> 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.

Based on this:
https://lore.kernel.org/all/9f297568-5616-40b6-b401-df1af57d5e14@hogyros.de

I *guess* this is the critical patch, and together with
https://patchwork.freedesktop.org/patch/709684/?series=161721&rev=4,
it fixes an issue?

Without these two patches, i915 assumes its
vga_get_uninterruptible(pdev, VGA_RSRC_LEGACY_IO) always succeeds, and
I guess the problem is that if it *didn't* succeed, i915's subsequent
ioport accesses may go to the wrong device or to no device at all,
which might corrupt another device or cause a PCI error?

We could include the i915 patch in this series.  Wouldn't be a
problem from the PCI side, I dunno about the DRM side.

> 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
> 

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

[Simon Richter]

Hi,

On 3/11/26 5:22 AM, Bjorn Helgaas wrote:

> Based on this:
> https://lore.kernel.org/all/9f297568-5616-40b6-b401-df1af57d5e14@hogyros.de

> I *guess* this is the critical patch, and together with
> https://patchwork.freedesktop.org/patch/709684/?series=161721&rev=4,
> it fixes an issue?

This patch to notice, and the patch to __vga_tryget() and vga_get() to 
forward the error to i915. Basically there are multiple layers of not 
handling errors in the current code.

rev=4 is the backported patch for 5.3 to 6.19, rev=3 is the relevant 
version, but technically it's doing the same thing.

> Without these two patches, i915 assumes its
> vga_get_uninterruptible(pdev, VGA_RSRC_LEGACY_IO) always succeeds, and
> I guess the problem is that if it *didn't* succeed, i915's subsequent
> ioport accesses may go to the wrong device or to no device at all,
> which might corrupt another device or cause a PCI error?

  - i915 assumes vga_get_uninterruptible() always succeeds.

  - This assumption currently holds because __vga_tryget() assumes that 
pci_set_vga_state() succeeds, and there is no error return to vga_get().

  - That assumption holds because pci_set_vga_state() only reports 
errors from the uv BIOS (or any other platform code) and assumes that 
setting VGAEnable in bridge control always succeeds.

The accesses should not go to the wrong device, because decoding on the 
previously active device has been disabled, either by disabling 
VGAEnable in a bridge above it or by disabling IO decoding for the VGA 
card altogether for the common case where onboard VGA is integrated into 
the mainboard chipset, and there are no bridges between the root and the 
onboard VGA that aren't also parents of the dGPU).

So the result is these go to no device at all.

> We could include the i915 patch in this series.  Wouldn't be a
> problem from the PCI side, I dunno about the DRM side.

The i915 patch (rev=3) is already in drm-intel-next, so it will 
percolate up into 7.0 from there.

If there is a desire to backport the PCI changes, i915 should get the 
backported patch for the older kernel versions as well.

    Simon

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

[Simon Richter]

Hi,

On 3/11/26 5:07 AM, Bjorn Helgaas wrote:

> vga_get() is implemented in vgaarb.c and is only used there.  Can we
> drop this declaration and the stub below, make it static and
> __must_check in vgaarb.c?

It's documented API, and exported. If we make it private, we need to 
export vga_get_interruptible() and vga_get_uninterruptible() instead.

    Simon

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

[Bjorn Helgaas]

On Thu, Mar 12, 2026 at 07:51:31AM +0900, Simon Richter wrote:
> On 3/11/26 5:07 AM, Bjorn Helgaas wrote:
> 
> > vga_get() is implemented in vgaarb.c and is only used there.  Can we
> > drop this declaration and the stub below, make it static and
> > __must_check in vgaarb.c?
> 
> It's documented API, and exported. If we make it private, we need to export
> vga_get_interruptible() and vga_get_uninterruptible() instead.

Yes.  vga_get_interruptible() and vga_get_uninterruptible() are
already exported by virtue of being defined in include/linux/vgaarb.h.
We would have to move those implementations to vgaarb.c and
EXPORT_SYMBOL them, leaving just the declaration in vgaarb.h.

Is there other API documentation I don't see?  Are you concerned about
out-of-tree drivers that might use vga_get()?  I don't know how much
to worry about that; usually I don't put too much effort into keeping
out-of-tree things working.

Bjorn

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

[Simon Richter]

Hi,

On 3/11/26 4:36 AM, Bjorn Helgaas wrote:

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

> Is there any reason for pci_set_vga_state() to be in pci.c while the
> only caller is in vgaarb.c?  If not, we could move it there, make it
> static, and drop it from include/linux/pci.h.

I have no strong feelings one way or the other. I believe that is

https://lore.kernel.org/linux-pci/20260220194943.3541702-1-bhelgaas@google.com/

Whichever goes first means the other needs to be rebased. Not a problem, 
unless backports are involved.

    Simon

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

[Simon Richter]

Hi,

On 3/12/26 8:14 AM, Bjorn Helgaas wrote:

> Is there other API documentation I don't see?

Only kernel docs, which would be annoying to edit because vga_get() 
documentation is lengthy, and the wrappers' documentation just refer to it.

    Simon

Re: [PATCH v3 0/5] Bridges without VGA support

[Bjorn Helgaas]

On Sun, Mar 08, 2026 at 02:35:33AM +0900, Simon Richter wrote:
> 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(-)

Applied to pci/vga for v7.1, thanks!

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

[Bjorn Helgaas]

On Sun, Mar 08, 2026 at 02:35:34AM +0900, Simon Richter wrote:
> 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.

I applied this, but I'm a little confused about the "don't call
vga_put() when userspace closes the handle" part.  Does that happen in
this patch and I'm just missing it?  This patch changes the "lock"
case, and the only vga_put() call I see is in the "unlock" case.

> 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
> 

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

[Simon Richter]

Hi Bjorn,

On 3/25/26 4:37 AM, Bjorn Helgaas wrote:

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

> I applied this, but I'm a little confused about the "don't call
> vga_put() when userspace closes the handle" part.  Does that happen in
> this patch and I'm just missing it?  This patch changes the "lock"
> case, and the only vga_put() call I see is in the "unlock" case.

By not locking, we're not incrementing uc->io_cnt and uc->mem_cnt, so 
the release will not call vga_put().

    Simon

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

[Bjorn Helgaas]

On Wed, Mar 25, 2026 at 03:52:37PM +0900, Simon Richter wrote:
> Hi Bjorn,
> 
> On 3/25/26 4:37 AM, Bjorn Helgaas wrote:
> 
> > > 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.
> 
> > I applied this, but I'm a little confused about the "don't call
> > vga_put() when userspace closes the handle" part.  Does that happen in
> > this patch and I'm just missing it?  This patch changes the "lock"
> > case, and the only vga_put() call I see is in the "unlock" case.
> 
> By not locking, we're not incrementing uc->io_cnt and uc->mem_cnt, so the
> release will not call vga_put().

Thanks, added this to the commit log:

  If VGA routing cannot be established, vga_get_uninterruptible() returns an
  error and does not increment the lock count. Return the error to the
  caller.

  Return before incrementing uc->io_cnt/mem_cnt so vga_arb_release() won't
  call vga_put() when userspace closes the handle.