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 v2] fbdev/hga: Request memory region before ioremap Date: Thu, 12 Mar 2026 07:28:48 +1000 Message-ID: In-Reply-To: References: X-Mailer: Claude Code Patch Reviewer Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Patch Review **Issue 1 (Medium): Mixing devm-managed and manual resource management** The patch uses `devm_request_mem_region()` which will auto-release the regi= on when the device is removed, but the `ioremap()` on the very next line re= mains unmanaged. The `hgafb_remove()` function at line 609 manually calls `= iounmap(hga_vram)` but has no corresponding manual `release_region()` for t= he video memory (since devm handles it). This creates an inconsistency =E2= =80=94 if you're going devm for the mem region, you should also use `devm_i= oremap()` to replace the bare `ioremap()`. Otherwise the lifecycle manageme= nt is split confusingly between two models. ```c + if (!devm_request_mem_region(&pdev->dev, 0xb0000, hga_vram_len, "hgafb"))= { ... hga_vram =3D ioremap(0xb0000, hga_vram_len); ``` Consider using `devm_ioremap()` here as well, and then removing the manual = `iounmap()` calls in the error path, `hgafb_probe()`, and `hgafb_remove()`. **Issue 2 (Medium): I/O port regions still use non-devm request_region** The existing code at lines 291-294 uses bare `request_region()` for the I/O= port ranges `0x3b0` and `0x3bf`, with manual release in both the error pat= h and `hgafb_remove()`. If you're converting to devm for memory region mana= gement, it would be more consistent to also convert these to `devm_request_= region()` and drop the `release_io_ports`/`release_io_port` flags and manua= l release calls. This would be a good opportunity to clean up the entire re= source model in one go. **Issue 3 (Medium): Error path leak =E2=80=94 devm region not released on i= oremap failure** When `devm_request_mem_region()` succeeds but the subsequent `ioremap()` fa= ils, the function returns `-ENOMEM` directly. Since `devm_request_mem_regio= n()` is tied to the device lifecycle, the region will only be released when= the platform device is unbound. But the probe function returns an error, a= nd depending on the driver core behavior, the devm cleanup should fire on p= robe failure =E2=80=94 so this is likely okay. However, the error path at `= goto error` (line 350) does not account for the new mem region either. If t= he card detection memory/IO tests fail, the code jumps to `error:` which ca= lls `iounmap(hga_vram)` and releases IO ports, but the devm mem region is a= gain left to implicit cleanup. This works but is fragile and inconsistent w= ith the explicit cleanup style used throughout the rest of the function. **Issue 4 (Minor): pr_err prefix is redundant** ```c + pr_err("hgafb: cannot reserve video memory at 0xb0000\n"); ``` Using `pr_err` already doesn't add a prefix by default unless `pr_fmt` is d= efined. However, manually prefixing with `"hgafb: "` is inconsistent with t= he preferred approach. Since `pdev` is now available, consider using `dev_e= rr(&pdev->dev, ...)` instead, which automatically adds the device name. **Issue 5 (Minor): Should use devm_ioremap or convert ioremap to devm** Since the function signature was already changed to accept `struct platform= _device *pdev`, this would be the right time to replace: ```c hga_vram =3D ioremap(0xb0000, hga_vram_len); ``` with: ```c hga_vram =3D devm_ioremap(&pdev->dev, 0xb0000, hga_vram_len); ``` and remove the `iounmap()` calls in `hgafb_remove()` (line 621) and the err= or paths in `hgafb_probe()` (line 580, 600). **Summary**: The core idea is sound =E2=80=94 reserving the memory region b= efore mapping is the right thing to do. However, the patch introduces a hal= f-managed, half-manual resource model. A v3 should either go fully devm (pr= eferred =E2=80=94 convert ioremap and the IO port requests to devm as well,= removing the manual cleanup) or stay fully manual (use `request_mem_region= ` and add the corresponding `release_region` to the error and remove paths)= . Mixing the two models makes the code harder to reason about and maintain. --- Generated by Claude Code Patch Reviewer