From mboxrd@z Thu Jan 1 00:00:00 1970 From: Claude Code Review Bot To: dri-devel-reviews@example.com Subject: Claude review: Re: [PATCH v3] fbdev/hga: Request memory region before ioremap Date: Thu, 12 Mar 2026 07:23:24 +1000 Message-ID: In-Reply-To: <3d58e520-0308-44c7-a43a-e438548e9c40@suse.de> References: <3d58e520-0308-44c7-a43a-e438548e9c40@suse.de> <3d58e520-0308-44c7-a43a-e438548e9c40@suse.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 **Concept: Good, but implementation has issues.** The intent is sound: `ioremap()` without a preceding `request_mem_region()`= leaves the resource tree incomplete. However: **1. `devm_request_mem_region` without `devm_ioremap` creates a resource ma= nagement mismatch.** The patch uses `devm_request_mem_region()` (automatically released on devic= e removal) but keeps the plain `ioremap()` call. This means the memory regi= on is managed by devres, but the ioremap mapping is manually managed. In `h= gafb_remove()` (line 621), `iounmap(hga_vram)` is called explicitly, which = is fine =E2=80=94 but now there's no explicit `release_region()` for the vi= deo memory range because `devm_request_mem_region` handles it automatically= . This *works* but is an inconsistent pattern. Consider also converting `io= remap()` to `devm_ioremap()` for consistency, or use non-devm `request_mem_= region()` and add the matching `release_region()` in the existing cleanup p= aths. **2. Missing cleanup of the memory region in the `error` path within `hga_c= ard_detect()`.** Looking at the existing code at lines 350=E2=80=93361 of hgafb.c, when card= detection fails, the function jumps to `error:` which calls `iounmap(hga_v= ram)` and releases I/O ports. Since `devm_request_mem_region()` is managed,= it won't be released until the device is unbound =E2=80=94 but in this err= or path, the probe function returns an error, so devres *will* clean it up = on probe failure. This is technically correct, but only because probe failu= re triggers devres cleanup. **3. The `ioremap()` should also be converted to `devm_ioremap()`.** If you're passing `pdev` into `hga_card_detect()` specifically to use devre= s, you should go all the way and also convert the `ioremap()` to `devm_iore= map()`. Then remove the manual `iounmap()` calls in the `error` label (line= 356), in `hgafb_probe()` error paths (lines 580, 600), and in `hgafb_remov= e()` (line 621). This would be a more complete and consistent cleanup. Howe= ver, that may be better suited as a follow-up patch. **4. The I/O port regions at `0x3b0` and `0x3bf` have the same pattern issu= e.** The existing code at lines 291=E2=80=93294 uses plain `request_region()` fo= r the I/O ports, and the `error` path and `hgafb_remove()` manually release= them. If you're fixing resource reservation for VRAM, it might be worth no= ting these should also be converted for consistency (again, possibly as a f= ollow-up). **5. Minor: return value semantics.** ```c + if (!devm_request_mem_region(&pdev->dev, 0xb0000, hga_vram_len, "hgafb"))= { + dev_err(&pdev->dev, "cannot reserve video memory at 0xb0000\n"); + return -EBUSY; + } ``` Returning `-EBUSY` is reasonable here. The `dev_err()` usage (replacing the= older `pr_err` style used elsewhere in this driver) is good and was reques= ted by the reviewer in v2. **Summary:** The patch is a step in the right direction but creates an inco= nsistent resource management model =E2=80=94 `devm_request_mem_region` pair= ed with manual `ioremap`/`iounmap`. At minimum this should be called out in= the commit message as an incremental improvement, or better yet, the `iore= map` should be converted to `devm_ioremap` in the same patch. As-is, the pa= tch is functionally correct but could be cleaner. --- Generated by Claude Code Patch Reviewer