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: fbdev/hga: Request memory region before ioremap
Date: Wed, 11 Mar 2026 13:36:34 +1000
Message-ID: <review-patch1-20260310064124.602848-1-hardik.phalet@pm.me>
In-Reply-To: <20260310064124.602848-1-hardik.phalet@pm.me>
References: <20260310064124.602848-1-hardik.phalet@pm.me> <20260310064124.602848-1-hardik.phalet@pm.me>
X-Mailer: Claude Code Patch Reviewer
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0

Patch Review

**Positive aspects:**
- Correctly identifies that `ioremap()` at `0xb0000` is called without a pr=
eceding `request_mem_region()`.
- Properly adds `release_mem_region()` in the `ioremap()` failure path, the=
 `error:` goto label, and in `hgafb_remove()`.

**Issue: Ordering in `error:` cleanup path**

In the `error:` label, the patch adds `release_mem_region()` *before* the e=
xisting `iounmap(hga_vram)` call at line 356 of the current source. Looking=
 at the existing error path (lines 350-361):

```c
error:
	release_mem_region(0xb0000, hga_vram_len);  // added by patch
	if (release_io_ports)
		release_region(0x3b0, 12);
	if (release_io_port)
		release_region(0x3bf, 1);

	iounmap(hga_vram);
```

The `iounmap()` should happen *before* `release_mem_region()` =E2=80=94 you=
 should unmap the virtual mapping before releasing the underlying physical =
region. While in practice this is unlikely to cause a problem on x86 ISA ha=
rdware, the correct teardown order is the reverse of the setup order: `ioun=
map()` first, then `release_mem_region()`. The patch should place `release_=
mem_region()` after the `iounmap()` call, or better yet, after all existing=
 cleanup in the error path (mirroring the order in `hgafb_remove()`).

Looking at `hgafb_remove()` (line 621-627 in current source), the patch cor=
rectly places `release_mem_region()` after `iounmap(hga_vram)`, which is th=
e right order. The `error:` path should match this ordering.

**Minor style nit:**

The `pr_err` format string uses a `"hgafb: "` prefix manually. The driver c=
ould use `pr_fmt` or `dev_err` for consistent prefixing, but this is consis=
tent with the existing style in this driver (see line 358: `pr_err("hgafb: =
HGA card not detected.\n")`), so it's acceptable.

**Summary:** The patch is a valid improvement but the teardown ordering in =
the `error:` goto path should be fixed so that `release_mem_region()` comes=
 after `iounmap()`, consistent with the ordering in `hgafb_remove()`.

---
Generated by Claude Code Patch Reviewer