[PATCH 0/1] Avoid oops on illegal VGA register access

[PATCH 0/1] Avoid oops on illegal VGA register access

[Simon Richter]

Hi,

this adds a way to avoid the illegal VGA accesses on non-x86 platforms by
letting these platforms report that VGA registers cannot be mapped.

This patch alone doesn't fully solve the problem, it also needs complementary
changes in vgaarb to actually report this, but since the interface is unchanged
and it is wrong to ignore the error code anyway, little coordination is needed
here.

Needs review from Intel whether this is the correct flow, and a decision on
whether this should be backported.

   Simon

Simon Richter (1):
  drm/i915: handle failure from vga_get

 drivers/gpu/drm/i915/display/intel_vga.c | 29 ++++++++++++++++++++----
 1 file changed, 24 insertions(+), 5 deletions(-)


base-commit: 15658979e64a7c97eaa65563e27a5a65e68a0188
-- 
2.47.3

[PATCH 1/1] drm/i915: handle failure from vga_get

[Simon Richter]

This function returns an error code. If that code is non-zero, VGA decoding
is undefined, and the lock counter has not been increased, so it is not valid
to access registers or call vga_put afterwards.

Signed-off-by: Simon Richter <Simon.Richter@hogyros.de>
Closes: https://gitlab.freedesktop.org/drm/xe/kernel/-/issues/1824
---
 drivers/gpu/drm/i915/display/intel_vga.c | 29 ++++++++++++++++++++----
 1 file changed, 24 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_vga.c b/drivers/gpu/drm/i915/display/intel_vga.c
index 6fc3e3702cb8..4118c451d53c 100644
--- a/drivers/gpu/drm/i915/display/intel_vga.c
+++ b/drivers/gpu/drm/i915/display/intel_vga.c
@@ -112,12 +112,17 @@ static bool intel_pci_bridge_set_vga(struct pci_dev *pdev, bool enable)
 	return old & PCI_BRIDGE_CTL_VGA;
 }
 
-static bool intel_vga_get(struct intel_display *display, bool mmio)
+static int __must_check intel_vga_get(struct intel_display *display,
+	bool mmio, bool *old_io_decode)
 {
 	struct pci_dev *pdev = to_pci_dev(display->drm->dev);
+	int err;
 
 	if (mmio)
-		return false;
+	{
+		*old_io_decode = false;
+		return 0;
+	}
 
 	/*
 	 * Bypass the VGA arbiter on the iGPU and just enable
@@ -131,9 +136,14 @@ static bool intel_vga_get(struct intel_display *display, bool mmio)
 	 * of how any other VGA routing bits are configured.
 	 */
 	if (display->platform.dgfx)
-		vga_get_uninterruptible(pdev, VGA_RSRC_LEGACY_IO);
+	{
+		err = vga_get_uninterruptible(pdev, VGA_RSRC_LEGACY_IO);
+		if (unlikely(err))
+			return err;
+	}
 
-	return intel_pci_set_io_decode(pdev, true);
+	*old_io_decode = intel_pci_set_io_decode(pdev, true);
+	return 0;
 }
 
 static void intel_vga_put(struct intel_display *display, bool io_decode, bool mmio)
@@ -175,6 +185,7 @@ void intel_vga_disable(struct intel_display *display)
 	bool io_decode;
 	u8 msr, sr1;
 	u32 tmp;
+	int err;
 
 	if (!intel_vga_decode_is_enabled(display)) {
 		drm_dbg_kms(display->drm, "VGA decode is disabled\n");
@@ -216,7 +227,15 @@ void intel_vga_disable(struct intel_display *display)
 			goto reset_vgacntr;
 	}
 
-	io_decode = intel_vga_get(display, mmio);
+	/* This should not fail, because vga_get will only report errors for
+	 * dGPUs that are unreachable via the bridge, and cannot be made
+	 * reachable either. We shouldn't even get here for this case, but if
+	 * we do, we assume that the bridge will also refuse future requests
+	 * to forward VGA accesses.
+	 */
+	err = intel_vga_get(display, mmio, &io_decode);
+	if (unlikely(err))
+		goto reset_vgacntr;
 
 	drm_WARN_ON(display->drm, !mmio && !intel_pci_has_vga_io_decode(pdev));
 
-- 
2.47.3

Claude review: drm/i915: handle failure from vga_get

[Claude Code Review Bot]

Patch Review

**Brace style**: The patch uses the wrong brace style for `if` statements in several places. Linux kernel coding style (documented in `Documentation/process/coding-style.rst`) requires the opening brace on the same line as the `if`, not on the next line:

> +	if (mmio)
> +	{
> +		*old_io_decode = false;
> +		return 0;
> +	}

and:

> +	if (display->platform.dgfx)
> +	{
> +		err = vga_get_uninterruptible(pdev, VGA_RSRC_LEGACY_IO);
> +		if (unlikely(err))
> +			return err;
> +	}

Both should have the opening brace on the `if` line: `if (mmio) {` and `if (display->platform.dgfx) {`. This would be caught by `checkpatch.pl`.

**Function signature change**: The change from returning `bool` to returning `int` via `__must_check`, with the old return value moved to an output parameter, is a reasonable approach. The `__must_check` annotation is good practice here.

> +static int __must_check intel_vga_get(struct intel_display *display,
> +	bool mmio, bool *old_io_decode)

Minor: the continuation line should align with the opening parenthesis per kernel style, i.e. it should be indented to align after `(`.

**Error handling in `intel_vga_disable()`**: The error path looks correct — if `intel_vga_get()` fails, jumping to `reset_vgacntr` skips the VGA register I/O (outb/inb) and the `intel_vga_put()` call, going directly to the MMIO-based VGA disable. Since the VGA arbiter lock was not acquired on failure, skipping `intel_vga_put()` is the right behavior.

> +	/* This should not fail, because vga_get will only report errors for
> +	 * dGPUs that are unreachable via the bridge, and cannot be made
> +	 * reachable either. We shouldn't even get here for this case, but if
> +	 * we do, we assume that the bridge will also refuse future requests
> +	 * to forward VGA accesses.
> +	 */

The multi-line comment style is slightly off. Kernel preferred style for multi-line comments puts the `/*` on its own line:
```
/*
 * This should not fail, because ...
 */
```

**Incomplete coverage**: The commit message says "it is not valid to access registers or call vga_put afterwards" when `vga_get` fails. This is correct for the `intel_vga_get()` path, but the same problem exists in `intel_vga_reset_io_mem()`, which also calls `vga_get_uninterruptible()` without checking the return value and then immediately does `outb(inb(VGA_MIS_R), VGA_MIS_W)`. If the concern is that VGA resources are unreachable on non-x86, this function has the same problem. Was this intentionally left for a follow-up, or is it an oversight?

**`io_decode` initialization**: When `intel_vga_get()` returns an error, the `io_decode` local variable in `intel_vga_disable()` is left uninitialized, but since the error path gotos past all uses of `io_decode`, this is fine.

**`unlikely()` usage**: The `unlikely(err)` annotations are reasonable given the comment that this "should not fail" in practice.

**Commit message**: The commit message accurately describes the problem, though it could be more specific about when this actually triggers (non-x86 platforms, referencing the linked issue). The `Closes:` tag referencing the Xe kernel tracker issue is appropriate.

**Base commit**: The patch declares `base-commit: 15658979e64a7c97eaa65563e27a5a65e68a0188` which is not in drm-next. The cover letter should explicitly state the dependency on the prerequisite refactoring series, since this patch does not apply to any public tree. The functions `intel_vga_get()`, `intel_vga_put()`, `intel_pci_set_io_decode()`, `intel_pci_bridge_set_vga()`, and `intel_vga_decode_is_enabled()` referenced in the diff context do not exist in the current drm-next tree.

---
Generated by Claude Code Patch Reviewer

Claude review: Avoid oops on illegal VGA register access

[Claude Code Review Bot]

Overall Series Review

Subject: Avoid oops on illegal VGA register access
Author: Simon Richter <Simon.Richter@hogyros.de>
Patches: 2
Reviewed: 2026-02-18T06:51:00.223518

---

This is a single-patch series that adds error handling for `vga_get_uninterruptible()` in the i915 VGA code. The stated motivation is to avoid oops on non-x86 platforms where VGA registers cannot be mapped, which would cause `vga_get()` to return `-ENODEV`. The patch modifies `intel_vga_get()` to propagate error codes, and `intel_vga_disable()` to handle them by skipping VGA register access and jumping to the MMIO-based VGA plane disable.

The patch is based on a tree that contains a refactoring of `intel_vga.c` (adding `intel_vga_get()`, `intel_vga_put()`, `intel_pci_set_io_decode()`, etc.) that has not yet landed in drm-next. The base commit `15658979e64a` is not present in drm-next. This means the patch cannot be applied on its own — it depends on a prerequisite refactoring series. The cover letter acknowledges this indirectly, mentioning that "complementary changes in vgaarb" are also needed, but the dependency on the i915 VGA refactoring series is not stated.

There are a few coding style issues and one incomplete aspect: the error handling is only added for `intel_vga_get()` within `intel_vga_disable()`, but if the underlying problem is that `vga_get_uninterruptible()` can fail on non-x86, this should also be considered for `intel_vga_reset_io_mem()` which similarly calls `vga_get_uninterruptible()` without checking the return value.

---
Generated by Claude Code Patch Reviewer