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

[PATCH v2] 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 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/
[1]: https://lore.kernel.org/all/5f9749ba-18a8-4b6b-a6e7-a011a3871bfb@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..fffd34141f8d 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")) {
+		pr_err("hgafb: 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 v2] fbdev/hga: Request memory region before ioremap

[Thomas Zimmermann]

Hi

Am 10.03.26 um 12:38 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>
> ---
> 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/
> [1]: https://lore.kernel.org/all/5f9749ba-18a8-4b6b-a6e7-a011a3871bfb@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..fffd34141f8d 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")) {
> +		pr_err("hgafb: cannot reserve video memory at 0xb0000\n");

dev_err() with pdev->dev please.

The rest looks good.

Best regards
Thomas

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

[Claude Code Review Bot]

Overall Series Review

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

---

This is a single patch (v2) that adds a `devm_request_mem_region()` call before `ioremap()` in the HGA framebuffer driver to properly reserve the video memory region at `0xb0000`. The intent is correct — claiming the memory region prevents silent conflicts with other drivers. However, the patch has several issues related to mixing managed (`devm_`) and unmanaged resource APIs, an incomplete conversion, and a missing cleanup path.

---
Generated by Claude Code Patch Reviewer

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

[Claude Code Review Bot]

Patch Review

**Issue 1 (Medium): Mixing devm-managed and manual resource management**

The patch uses `devm_request_mem_region()` which will auto-release the region when the device is removed, but the `ioremap()` on the very next line remains unmanaged. The `hgafb_remove()` function at line 609 manually calls `iounmap(hga_vram)` but has no corresponding manual `release_region()` for the video memory (since devm handles it). This creates an inconsistency — if you're going devm for the mem region, you should also use `devm_ioremap()` to replace the bare `ioremap()`. Otherwise the lifecycle management is split confusingly between two models.

```c
+	if (!devm_request_mem_region(&pdev->dev, 0xb0000, hga_vram_len, "hgafb")) {
...
 	hga_vram = 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 path and `hgafb_remove()`. If you're converting to devm for memory region management, it would be more consistent to also convert these to `devm_request_region()` and drop the `release_io_ports`/`release_io_port` flags and manual release calls. This would be a good opportunity to clean up the entire resource model in one go.

**Issue 3 (Medium): Error path leak — devm region not released on ioremap failure**

When `devm_request_mem_region()` succeeds but the subsequent `ioremap()` fails, the function returns `-ENOMEM` directly. Since `devm_request_mem_region()` 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, and depending on the driver core behavior, the devm cleanup should fire on probe failure — so this is likely okay. However, the error path at `goto error` (line 350) does not account for the new mem region either. If the card detection memory/IO tests fail, the code jumps to `error:` which calls `iounmap(hga_vram)` and releases IO ports, but the devm mem region is again left to implicit cleanup. This works but is fragile and inconsistent with 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 defined. However, manually prefixing with `"hgafb: "` is inconsistent with the preferred approach. Since `pdev` is now available, consider using `dev_err(&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 = ioremap(0xb0000, hga_vram_len);
```
with:
```c
hga_vram = devm_ioremap(&pdev->dev, 0xb0000, hga_vram_len);
```
and remove the `iounmap()` calls in `hgafb_remove()` (line 621) and the error paths in `hgafb_probe()` (line 580, 600).

**Summary**: The core idea is sound — reserving the memory region before mapping is the right thing to do. However, the patch introduces a half-managed, half-manual resource model. A v3 should either go fully devm (preferred — 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