Claude review: drm/i915: handle failure from vga_get

[Claude Code Review Bot <claude-review@example.com>]

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