[PATCH v3] fbdev/hga: Request memory region before ioremap

[PATCH v3] fbdev/hga: Request memory region before ioremap

[Hardik Phalet]

The driver calls ioremap() on the HGA video memory at 0xb0000 without
first reserving the physical address range. This leaves the kernel
resource tree incomplete and can cause silent conflicts with other
drivers claiming the same range.

Add a devm_request_mem_region() call before ioremap() in
hga_card_detect() to reserve the memory region.

Signed-off-by: Hardik Phalet <hardik.phalet@pm.me>
---
Changes in v3:
- Used dev_err() to log memory region request, based on another review
  comment by Thomas [2].
Changes in v2:
- Used devm_request_mem_region instead of request_mem_region, based on a
	review comment by Thomas [1].

v1: https://lore.kernel.org/all/20260310064124.602848-1-hardik.phalet@pm.me/
v2: https://lore.kernel.org/all/20260310113810.789575-1-hardik.phalet@pm.me/
[1]: https://lore.kernel.org/all/5f9749ba-18a8-4b6b-a6e7-a011a3871bfb@suse.de/
[2]: https://lore.kernel.org/all/ec635591-c861-4aa8-a259-718690ddaa4e@suse.de/

 drivers/video/fbdev/hgafb.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/video/fbdev/hgafb.c b/drivers/video/fbdev/hgafb.c
index 14418aa3791a..d32fd1c5217c 100644
--- a/drivers/video/fbdev/hgafb.c
+++ b/drivers/video/fbdev/hgafb.c
@@ -276,7 +276,7 @@ static void hga_blank(int blank_mode)
 	spin_unlock_irqrestore(&hga_reg_lock, flags);
 }
 
-static int hga_card_detect(void)
+static int hga_card_detect(struct platform_device *pdev)
 {
 	int count = 0;
 	void __iomem *p, *q;
@@ -284,6 +284,11 @@ static int hga_card_detect(void)
 
 	hga_vram_len  = 0x08000;
 
+	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;
+	}
+
 	hga_vram = ioremap(0xb0000, hga_vram_len);
 	if (!hga_vram)
 		return -ENOMEM;
@@ -568,7 +573,7 @@ static int hgafb_probe(struct platform_device *pdev)
 	struct fb_info *info;
 	int ret;
 
-	ret = hga_card_detect();
+	ret = hga_card_detect(pdev);
 	if (ret)
 		return ret;
 
-- 
2.53.0

Re: [PATCH v3] fbdev/hga: Request memory region before ioremap

[Thomas Zimmermann]

Hi,

thanks for the patch. Let's hope there are no conflicts with other 
hardware.  IDK if anyone still uses this driver.

Am 10.03.26 um 13:30 schrieb Hardik Phalet:
> The driver calls ioremap() on the HGA video memory at 0xb0000 without
> first reserving the physical address range. This leaves the kernel
> resource tree incomplete and can cause silent conflicts with other
> drivers claiming the same range.
>
> Add a devm_request_mem_region() call before ioremap() in
> hga_card_detect() to reserve the memory region.
>
> Signed-off-by: Hardik Phalet <hardik.phalet@pm.me>

Reviewed-by: Thomas Zimmermann <tzimmermann@suse.de>

Best regards
Thomas

> ---
> Changes in v3:
> - Used dev_err() to log memory region request, based on another review
>    comment by Thomas [2].
> Changes in v2:
> - Used devm_request_mem_region instead of request_mem_region, based on a
> 	review comment by Thomas [1].
>
> v1: https://lore.kernel.org/all/20260310064124.602848-1-hardik.phalet@pm.me/
> v2: https://lore.kernel.org/all/20260310113810.789575-1-hardik.phalet@pm.me/
> [1]: https://lore.kernel.org/all/5f9749ba-18a8-4b6b-a6e7-a011a3871bfb@suse.de/
> [2]: https://lore.kernel.org/all/ec635591-c861-4aa8-a259-718690ddaa4e@suse.de/
>
>   drivers/video/fbdev/hgafb.c | 9 +++++++--
>   1 file changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/video/fbdev/hgafb.c b/drivers/video/fbdev/hgafb.c
> index 14418aa3791a..d32fd1c5217c 100644
> --- a/drivers/video/fbdev/hgafb.c
> +++ b/drivers/video/fbdev/hgafb.c
> @@ -276,7 +276,7 @@ static void hga_blank(int blank_mode)
>   	spin_unlock_irqrestore(&hga_reg_lock, flags);
>   }
>   
> -static int hga_card_detect(void)
> +static int hga_card_detect(struct platform_device *pdev)
>   {
>   	int count = 0;
>   	void __iomem *p, *q;
> @@ -284,6 +284,11 @@ static int hga_card_detect(void)
>   
>   	hga_vram_len  = 0x08000;
>   
> +	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;
> +	}
> +
>   	hga_vram = ioremap(0xb0000, hga_vram_len);
>   	if (!hga_vram)
>   		return -ENOMEM;
> @@ -568,7 +573,7 @@ static int hgafb_probe(struct platform_device *pdev)
>   	struct fb_info *info;
>   	int ret;
>   
> -	ret = hga_card_detect();
> +	ret = hga_card_detect(pdev);
>   	if (ret)
>   		return ret;
>   

-- 
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstr. 146, 90461 Nürnberg, Germany, www.suse.com
GF: Jochen Jaser, Andrew McDonald, Werner Knoblich, (HRB 36809, AG Nürnberg)

Claude review: Re: [PATCH v3] fbdev/hga: Request memory region before ioremap

[Claude Code Review Bot]

Overall Series Review

Subject: Re: [PATCH v3] fbdev/hga: Request memory region before ioremap
Author: Thomas Zimmermann <tzimmermann@suse.de>
Patches: 2
Reviewed: 2026-03-12T07:23:23.926808

---

This is a single-patch series (v3) that adds a `devm_request_mem_region()` call before `ioremap()` in the HGA framebuffer driver to properly reserve the video memory range at `0xb0000`. The goal is correct — the driver should reserve the physical address range to prevent conflicts. However, there are several issues with the current implementation that need to be addressed.

---
Generated by Claude Code Patch Reviewer

Claude review: Re: [PATCH v3] fbdev/hga: Request memory region before ioremap

[Claude Code Review Bot]

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 management mismatch.**

The patch uses `devm_request_mem_region()` (automatically released on device removal) but keeps the plain `ioremap()` call. This means the memory region is managed by devres, but the ioremap mapping is manually managed. In `hgafb_remove()` (line 621), `iounmap(hga_vram)` is called explicitly, which is fine — but now there's no explicit `release_region()` for the video memory range because `devm_request_mem_region` handles it automatically. This *works* but is an inconsistent pattern. Consider also converting `ioremap()` to `devm_ioremap()` for consistency, or use non-devm `request_mem_region()` and add the matching `release_region()` in the existing cleanup paths.

**2. Missing cleanup of the memory region in the `error` path within `hga_card_detect()`.**

Looking at the existing code at lines 350–361 of hgafb.c, when card detection fails, the function jumps to `error:` which calls `iounmap(hga_vram)` and releases I/O ports. Since `devm_request_mem_region()` is managed, it won't be released until the device is unbound — but in this error path, the probe function returns an error, so devres *will* clean it up on probe failure. This is technically correct, but only because probe failure 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 devres, you should go all the way and also convert the `ioremap()` to `devm_ioremap()`. Then remove the manual `iounmap()` calls in the `error` label (line 356), in `hgafb_probe()` error paths (lines 580, 600), and in `hgafb_remove()` (line 621). This would be a more complete and consistent cleanup. However, that may be better suited as a follow-up patch.

**4. The I/O port regions at `0x3b0` and `0x3bf` have the same pattern issue.**

The existing code at lines 291–294 uses plain `request_region()` for 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 noting these should also be converted for consistency (again, possibly as a follow-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 requested by the reviewer in v2.

**Summary:** The patch is a step in the right direction but creates an inconsistent resource management model — `devm_request_mem_region` paired with manual `ioremap`/`iounmap`. At minimum this should be called out in the commit message as an incremental improvement, or better yet, the `ioremap` should be converted to `devm_ioremap` in the same patch. As-is, the patch is functionally correct but could be cleaner.

---
Generated by Claude Code Patch Reviewer