From mboxrd@z Thu Jan  1 00:00:00 1970
From: Claude Code Review Bot <claude-review@example.com>
To: dri-devel-reviews@example.com
Subject: Claude review: drm/i915: handle failure from vga_get
Date: Wed, 18 Feb 2026 06:51:00 +1000
Message-ID: <review-patch1-20260217165214.236482-2-Simon.Richter@hogyros.de>
In-Reply-To: <20260217165214.236482-2-Simon.Richter@hogyros.de>
References: <20260217165214.236482-1-Simon.Richter@hogyros.de> <20260217165214.236482-2-Simon.Richter@hogyros.de>
X-Mailer: Claude Code Patch Reviewer
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0

Patch Review

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

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

and:

> +	if (display->platform.dgfx)
> +	{
> +		err =3D 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 returnin=
g `int` via `__must_check`, with the old return value moved to an output pa=
rameter, is a reasonable approach. The `__must_check` annotation is good pr=
actice 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 =
=E2=80=94 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 mu=
lti-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 correc=
t for the `intel_vga_get()` path, but the same problem exists in `intel_vga=
_reset_io_mem()`, which also calls `vga_get_uninterruptible()` without chec=
king the return value and then immediately does `outb(inb(VGA_MIS_R), VGA_M=
IS_W)`. If the concern is that VGA resources are unreachable on non-x86, th=
is 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, th=
e `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 give=
n the comment that this "should not fail" in practice.

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

**Base commit**: The patch declares `base-commit: 15658979e64a7c97eaa65563e=
27a5a65e68a0188` which is not in drm-next. The cover letter should explicit=
ly 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