Claude review: Avoid oops on illegal VGA register access

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

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

[Simon Richter]

Hi,

this is a backport of https://patchwork.freedesktop.org/series/161721/ that
should apply to anything between 5.3-rc4 and 6.19.

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

   Simon

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

 drivers/gpu/drm/i915/display/intel_vga.c | 21 +++++++++++++++++++--
 1 file changed, 19 insertions(+), 2 deletions(-)


base-commit: 6a753907865e35ae986b7b2ad48daa1eab4bcf3a
-- 
2.47.3

[PATCH 1/1] drm/i915: handle failure from vga_get_uninterruptible()

[Simon Richter]

The vga_get_uninterruptible() function can return an error if it fails to
set up VGA decoding for the requested device.

If VGA decoding is unavailable, we don't need to be careful to leave the
VGA emulation in a usable state, as vgacon will also be unable to get
access later on, so just skip over the VGA accesses and the vga_put() call
matching the failed vga_get_uninterruptible().

Signed-off-by: Simon Richter <Simon.Richter@hogyros.de>
---
 drivers/gpu/drm/i915/display/intel_vga.c | 21 +++++++++++++++++++--
 1 file changed, 19 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_vga.c b/drivers/gpu/drm/i915/display/intel_vga.c
index 6e125564db34..dd581f20e962 100644
--- a/drivers/gpu/drm/i915/display/intel_vga.c
+++ b/drivers/gpu/drm/i915/display/intel_vga.c
@@ -49,6 +49,7 @@ void intel_vga_disable(struct intel_display *display)
 	enum pipe pipe;
 	u32 tmp;
 	u8 sr1;
+	int err;
 
 	tmp = intel_de_read(display, vga_reg);
 	if (tmp & VGA_DISP_DISABLE)
@@ -65,13 +66,22 @@ void intel_vga_disable(struct intel_display *display)
 		    pipe_name(pipe));
 
 	/* WaEnableVGAAccessThroughIOPort:ctg,elk,ilk,snb,ivb,vlv,hsw */
-	vga_get_uninterruptible(pdev, VGA_RSRC_LEGACY_IO);
+	/*
+	 * if the VGA ports are inaccessible now, the chance that vgacon
+	 * will be able to access them later is low enough that we don't
+	 * have to care about the emulation state machines being in sync
+	 */
+	err = vga_get_uninterruptible(pdev, VGA_RSRC_LEGACY_IO);
+	if (err)
+		goto skip;
+
 	outb(0x01, VGA_SEQ_I);
 	sr1 = inb(VGA_SEQ_D);
 	outb(sr1 | VGA_SR01_SCREEN_OFF, VGA_SEQ_D);
 	vga_put(pdev, VGA_RSRC_LEGACY_IO);
 	udelay(300);
 
+skip:
 	intel_de_write(display, vga_reg, VGA_DISP_DISABLE);
 	intel_de_posting_read(display, vga_reg);
 }
@@ -79,6 +89,7 @@ void intel_vga_disable(struct intel_display *display)
 void intel_vga_reset_io_mem(struct intel_display *display)
 {
 	struct pci_dev *pdev = to_pci_dev(display->drm->dev);
+	int err;
 
 	/*
 	 * After we re-enable the power well, if we touch VGA register 0x3d5
@@ -89,8 +100,14 @@ void intel_vga_reset_io_mem(struct intel_display *display)
 	 * console_unlock(). So make here we touch the VGA MSR register, making
 	 * sure vgacon can keep working normally without triggering interrupts
 	 * and error messages.
+	 *
+	 * We ignore this issue if we can't access the VGA registers, because
+	 * neither can vgacon.
 	 */
-	vga_get_uninterruptible(pdev, VGA_RSRC_LEGACY_IO);
+	err = vga_get_uninterruptible(pdev, VGA_RSRC_LEGACY_IO);
+	if (err)
+		return;
+
 	outb(inb(VGA_MIS_R), VGA_MIS_W);
 	vga_put(pdev, VGA_RSRC_LEGACY_IO);
 }
-- 
2.47.3

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-03-09T08:44:57.272701

---

This is a simple, well-reasoned single-patch series that adds error handling for `vga_get_uninterruptible()` calls in the i915 VGA helper code. The motivation is clear: on non-x86 platforms, VGA legacy I/O resources may not be mappable, and ignoring the error return from `vga_get_uninterruptible()` leads to an oops when subsequent `outb()`/`inb()` calls are reached. The fix correctly skips the VGA I/O port accesses when VGA arbitration fails, while still performing the necessary MMIO register writes (like `VGA_DISP_DISABLE`).

The patch is straightforward, correct, and low-risk. The approach of "if we can't get VGA access, neither can vgacon, so there's nothing to synchronize" is sound logic.

**No major issues found.**

---
Generated by Claude Code Patch Reviewer

Claude review: drm/i915: handle failure from vga_get_uninterruptible()

[Claude Code Review Bot]

Patch Review

**Note:** The cover letter subject says "handle failure from vga_get()" but the patch subject says "vga_get_uninterruptible()" — the patch subject is more accurate.

**Changes in `intel_vga_disable()`:**

The `goto skip` pattern is used correctly here. When `vga_get_uninterruptible()` fails, the code skips the `outb()`/`inb()` VGA register accesses and the matching `vga_put()`, but still performs the MMIO write to `VGA_DISP_DISABLE`:

```c
err = vga_get_uninterruptible(pdev, VGA_RSRC_LEGACY_IO);
if (err)
    goto skip;

outb(0x01, VGA_SEQ_I);
sr1 = inb(VGA_SEQ_D);
outb(sr1 | VGA_SR01_SCREEN_OFF, VGA_SEQ_D);
vga_put(pdev, VGA_RSRC_LEGACY_IO);
udelay(300);

skip:
intel_de_write(display, vga_reg, VGA_DISP_DISABLE);
```

This is correct — the `VGA_DISP_DISABLE` write goes through MMIO (not legacy I/O), so it must still happen regardless of VGA arbitration status. The `udelay(300)` is also correctly skipped since it's only needed after the VGA sequencer register write.

**Changes in `intel_vga_reset_io_mem()`:**

The early `return` on error is clean and appropriate here, since the entire function body is about touching VGA legacy I/O registers:

```c
err = vga_get_uninterruptible(pdev, VGA_RSRC_LEGACY_IO);
if (err)
    return;
```

The added comment explaining the rationale is helpful:
```c
 * We ignore this issue if we can't access the VGA registers, because
 * neither can vgacon.
```

**Minor nits (non-blocking):**

- The `err` variable is declared but only used for a single check-and-branch. This is fine and follows kernel convention.
- The comment in `intel_vga_disable()` could start with a capital letter ("If the VGA ports...") per kernel comment style, but this is truly trivial.

**Verdict:** Patch looks correct and ready to apply. The error handling logic is sound, the skip/return patterns are appropriate for each call site, and the comments explain the reasoning well.

---
Generated by Claude Code Patch Reviewer