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

[PATCH] 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 via request_mem_region().
This leaves the kernel resource tree incomplete and can cause silent
conflicts with other drivers claiming the same range.

Add a request_mem_region() call before ioremap() in hga_card_detect()
and release the region in all error paths and in hgafb_remove().

Signed-off-by: Hardik Phalet <hardik.phalet@pm.me>
---
 drivers/video/fbdev/hgafb.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/drivers/video/fbdev/hgafb.c b/drivers/video/fbdev/hgafb.c
index 14418aa3791a..ceca6cc2c928 100644
--- a/drivers/video/fbdev/hgafb.c
+++ b/drivers/video/fbdev/hgafb.c
@@ -284,9 +284,16 @@ static int hga_card_detect(void)
 
 	hga_vram_len  = 0x08000;
 
+	if (!request_mem_region(0xb0000, hga_vram_len, "hgafb")) {
+		pr_err("hgafb: cannot reserve video memory at 0xb0000\n");
+		return -EBUSY;
+	}
+
 	hga_vram = ioremap(0xb0000, hga_vram_len);
-	if (!hga_vram)
+	if (!hga_vram) {
+		release_mem_region(0xb0000, hga_vram_len);
 		return -ENOMEM;
+	}
 
 	if (request_region(0x3b0, 12, "hgafb"))
 		release_io_ports = 1;
@@ -348,6 +355,7 @@ static int hga_card_detect(void)
 	}
 	return 0;
 error:
+	release_mem_region(0xb0000, hga_vram_len);
 	if (release_io_ports)
 		release_region(0x3b0, 12);
 	if (release_io_port)
@@ -619,6 +627,7 @@ static void hgafb_remove(struct platform_device *pdev)
 	}
 
 	iounmap(hga_vram);
+	release_mem_region(0xb0000, hga_vram_len);
 
 	if (release_io_ports)
 		release_region(0x3b0, 12);
-- 
2.53.0

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

[Thomas Zimmermann]

Hi

Am 10.03.26 um 07:41 schrieb Hardik Phalet:
> The driver calls ioremap() on the HGA video memory at 0xb0000 without
> first reserving the physical address range via request_mem_region().
> This leaves the kernel resource tree incomplete and can cause silent
> conflicts with other drivers claiming the same range.
>
> Add a request_mem_region() call before ioremap() in hga_card_detect()
> and release the region in all error paths and in hgafb_remove().
>
> Signed-off-by: Hardik Phalet <hardik.phalet@pm.me>
> ---
>   drivers/video/fbdev/hgafb.c | 11 ++++++++++-
>   1 file changed, 10 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/video/fbdev/hgafb.c b/drivers/video/fbdev/hgafb.c
> index 14418aa3791a..ceca6cc2c928 100644
> --- a/drivers/video/fbdev/hgafb.c
> +++ b/drivers/video/fbdev/hgafb.c
> @@ -284,9 +284,16 @@ static int hga_card_detect(void)

You can pass the platform device from hga_probe() and ...

>   
>   	hga_vram_len  = 0x08000;
>   
> +	if (!request_mem_region(0xb0000, hga_vram_len, "hgafb")) {

... use devm_request_mem_region() here.  That would avoid all the 
clean-up calls below AFAICT.

Best regards
Thomas


> +		pr_err("hgafb: cannot reserve video memory at 0xb0000\n");
> +		return -EBUSY;
> +	}
> +
>   	hga_vram = ioremap(0xb0000, hga_vram_len);
> -	if (!hga_vram)
> +	if (!hga_vram) {
> +		release_mem_region(0xb0000, hga_vram_len);
>   		return -ENOMEM;
> +	}
>   
>   	if (request_region(0x3b0, 12, "hgafb"))
>   		release_io_ports = 1;
> @@ -348,6 +355,7 @@ static int hga_card_detect(void)
>   	}
>   	return 0;
>   error:
> +	release_mem_region(0xb0000, hga_vram_len);
>   	if (release_io_ports)
>   		release_region(0x3b0, 12);
>   	if (release_io_port)
> @@ -619,6 +627,7 @@ static void hgafb_remove(struct platform_device *pdev)
>   	}
>   
>   	iounmap(hga_vram);
> +	release_mem_region(0xb0000, hga_vram_len);
>   
>   	if (release_io_ports)
>   		release_region(0x3b0, 12);

-- 
--
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: fbdev/hga: Request memory region before ioremap

[Claude Code Review Bot]

Overall Series Review

Subject: fbdev/hga: Request memory region before ioremap
Author: Hardik Phalet <hardik.phalet@pm.me>
Patches: 2
Reviewed: 2026-03-11T13:36:34.577207

---

This is a single patch adding a `request_mem_region()` call before `ioremap()` in the HGA framebuffer driver. The change is reasonable in principle — it's good practice to claim a memory region before mapping it, to prevent conflicts with other drivers. The implementation is mostly correct but has one ordering issue in the error path.

**Verdict:** Needs minor fix.

---
Generated by Claude Code Patch Reviewer

Claude review: fbdev/hga: Request memory region before ioremap

[Claude Code Review Bot]

Patch Review

**Positive aspects:**
- Correctly identifies that `ioremap()` at `0xb0000` is called without a preceding `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 existing `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()` — 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 hardware, the correct teardown order is the reverse of the setup order: `iounmap()` 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 correctly places `release_mem_region()` after `iounmap(hga_vram)`, which is the right order. The `error:` path should match this ordering.

**Minor style nit:**

The `pr_err` format string uses a `"hgafb: "` prefix manually. The driver could use `pr_fmt` or `dev_err` for consistent prefixing, but this is consistent 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