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

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)

On Tue Mar 10, 2026 at 6:38 PM IST, Thomas Zimmermann wrote:
> Hi,
>
> thanks for the patch. Let's hope there are no conflicts with other
> hardware.  IDK if anyone still uses this driver.
Hi Thomas,

Thanks for reviewing this.

Since I currently do not have access to the hardware needed to test the
change properly, I will drop this patch for now. I may revisit it once I
can validate the behavior on real hardware. 

Thanks again for your feedback.

Best regards,
Hardik
>
> 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)

Hi

Am 12.03.26 um 16:04 schrieb Hardik Phalet:
> On Tue Mar 10, 2026 at 6:38 PM IST, Thomas Zimmermann wrote:
>> Hi,
>>
>> thanks for the patch. Let's hope there are no conflicts with other
>> hardware.  IDK if anyone still uses this driver.
> Hi Thomas,
>
> Thanks for reviewing this.
>
> Since I currently do not have access to the hardware needed to test the
> change properly, I will drop this patch for now. I may revisit it once I
> can validate the behavior on real hardware.

Good luck. That's the Hercules framebuffer driver. Finding such ancient 
hardware that can run modern Linux is nigh impossible.

But we can merge the patch. If it breaks anyone's setup, they will send 
a bug report.

Helge will pick up the fix if he's ok with it.

Best regards
Thomas

>
> Thanks again for your feedback.
>
> Best regards,
> Hardik
>> 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)
>
>

-- 
--
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)

On 3/12/26 16:10, Thomas Zimmermann wrote:
> Am 12.03.26 um 16:04 schrieb Hardik Phalet:
>> On Tue Mar 10, 2026 at 6:38 PM IST, Thomas Zimmermann wrote:
>>> Hi,
>>>
>>> thanks for the patch. Let's hope there are no conflicts with other
>>> hardware.  IDK if anyone still uses this driver.
>> Hi Thomas,
>>
>> Thanks for reviewing this.
>>
>> Since I currently do not have access to the hardware needed to test the
>> change properly, I will drop this patch for now. I may revisit it once I
>> can validate the behavior on real hardware.
> 
> Good luck. That's the Hercules framebuffer driver. Finding such
> ancient hardware that can run modern Linux is nigh impossible.
> 
> But we can merge the patch. If it breaks anyone's setup, they will send a bug report.
> 
> Helge will pick up the fix if he's ok with it.

No, I don't want to merge such patches any longer without any testing
on real hardware. There is no actual problem (else someone would have reported),
as such I don't see a benefit to apply it. Applying it just brings the risk
that we break it for someone.
So, NAK.

I believe I wrote about my opinion already in another patch?
I think we should rephrase that specific TODO item (which mentions the memory
region allocation) that only patches which have been tested are accepted.

Helge

Hi

Am 12.03.26 um 20:47 schrieb Helge Deller:
> On 3/12/26 16:10, Thomas Zimmermann wrote:
>> Am 12.03.26 um 16:04 schrieb Hardik Phalet:
>>> On Tue Mar 10, 2026 at 6:38 PM IST, Thomas Zimmermann wrote:
>>>> Hi,
>>>>
>>>> thanks for the patch. Let's hope there are no conflicts with other
>>>> hardware.  IDK if anyone still uses this driver.
>>> Hi Thomas,
>>>
>>> Thanks for reviewing this.
>>>
>>> Since I currently do not have access to the hardware needed to test the
>>> change properly, I will drop this patch for now. I may revisit it 
>>> once I
>>> can validate the behavior on real hardware.
>>
>> Good luck. That's the Hercules framebuffer driver. Finding such
>> ancient hardware that can run modern Linux is nigh impossible.
>>
>> But we can merge the patch. If it breaks anyone's setup, they will 
>> send a bug report.
>>
>> Helge will pick up the fix if he's ok with it.
>
> No, I don't want to merge such patches any longer without any testing
> on real hardware. There is no actual problem (else someone would have 
> reported),
> as such I don't see a benefit to apply it. Applying it just brings the 
> risk
> that we break it for someone.
> So, NAK.
>
> I believe I wrote about my opinion already in another patch?

Sorry, I wasn't aware.

> I think we should rephrase that specific TODO item (which mentions the 
> memory
> region allocation) that only patches which have been tested are accepted.

There will likely no one show up here for testing unless it breaks there 
system. Which you won't know until you merge the patch.

If only pre-tested patches can go in, we should make a serious effort to 
remove drivers from fbdev. Because otherwise the driver code just sits 
around as liability.

Aggressively moving fbdev drivers into staging would be a good start IMHO.

Best regards
Thomas

>
> Helge

-- 
--
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)

Hi Thomas,

On 3/13/26 09:05, Thomas Zimmermann wrote:
> Am 12.03.26 um 20:47 schrieb Helge Deller:
>> On 3/12/26 16:10, Thomas Zimmermann wrote:
>>> Am 12.03.26 um 16:04 schrieb Hardik Phalet:
>>>> On Tue Mar 10, 2026 at 6:38 PM IST, Thomas Zimmermann wrote:
>>>>> Hi,
>>>>>
>>>>> thanks for the patch. Let's hope there are no conflicts with other
>>>>> hardware.  IDK if anyone still uses this driver.
>>>> Hi Thomas,
>>>>
>>>> Thanks for reviewing this.
>>>>
>>>> Since I currently do not have access to the hardware needed to test the
>>>> change properly, I will drop this patch for now. I may revisit it once I
>>>> can validate the behavior on real hardware.
>>>
>>> Good luck. That's the Hercules framebuffer driver. Finding such
>>> ancient hardware that can run modern Linux is nigh impossible.
>>>
>>> But we can merge the patch. If it breaks anyone's setup, they will send a bug report.
>>>
>>> Helge will pick up the fix if he's ok with it.
>>
>> No, I don't want to merge such patches any longer without any testing
>> on real hardware. There is no actual problem (else someone would have reported),
>> as such I don't see a benefit to apply it. Applying it just brings the risk
>> that we break it for someone.
>> So, NAK.
>>
>> I believe I wrote about my opinion already in another patch?
> 
> Sorry, I wasn't aware.
> 
>> I think we should rephrase that specific TODO item (which mentions the memory
>> region allocation) that only patches which have been tested are accepted.
> 
> There will likely no one show up here for testing unless it breaks
> there system. Which you won't know until you merge the patch.

No-one likes to merge unnecessary patches which highly potentially
introduce malfunctioning and haven't been tested at all.
  
> If only pre-tested patches can go in,

You misunderstand.
I'm still happy to take *any* patches for fbdev.
Even untested ones if they
a) seem necessary (e.g. bugfix), or
b) seem beneficial (code cleanup)
as long as they don't break the driver. This patch may break the driver.

Helge

Hi

Am 13.03.26 um 13:50 schrieb Helge Deller:
> Hi Thomas,
>
> On 3/13/26 09:05, Thomas Zimmermann wrote:
>> Am 12.03.26 um 20:47 schrieb Helge Deller:
>>> On 3/12/26 16:10, Thomas Zimmermann wrote:
>>>> Am 12.03.26 um 16:04 schrieb Hardik Phalet:
>>>>> On Tue Mar 10, 2026 at 6:38 PM IST, Thomas Zimmermann wrote:
>>>>>> Hi,
>>>>>>
>>>>>> thanks for the patch. Let's hope there are no conflicts with other
>>>>>> hardware.  IDK if anyone still uses this driver.
>>>>> Hi Thomas,
>>>>>
>>>>> Thanks for reviewing this.
>>>>>
>>>>> Since I currently do not have access to the hardware needed to 
>>>>> test the
>>>>> change properly, I will drop this patch for now. I may revisit it 
>>>>> once I
>>>>> can validate the behavior on real hardware.
>>>>
>>>> Good luck. That's the Hercules framebuffer driver. Finding such
>>>> ancient hardware that can run modern Linux is nigh impossible.
>>>>
>>>> But we can merge the patch. If it breaks anyone's setup, they will 
>>>> send a bug report.
>>>>
>>>> Helge will pick up the fix if he's ok with it.
>>>
>>> No, I don't want to merge such patches any longer without any testing
>>> on real hardware. There is no actual problem (else someone would 
>>> have reported),
>>> as such I don't see a benefit to apply it. Applying it just brings 
>>> the risk
>>> that we break it for someone.
>>> So, NAK.
>>>
>>> I believe I wrote about my opinion already in another patch?
>>
>> Sorry, I wasn't aware.
>>
>>> I think we should rephrase that specific TODO item (which mentions 
>>> the memory
>>> region allocation) that only patches which have been tested are 
>>> accepted.
>>
>> There will likely no one show up here for testing unless it breaks
>> there system. Which you won't know until you merge the patch.
>
> No-one likes to merge unnecessary patches which highly potentially
> introduce malfunctioning and haven't been tested at all.
>
>> If only pre-tested patches can go in,
>
> You misunderstand.
> I'm still happy to take *any* patches for fbdev.
> Even untested ones if they
> a) seem necessary (e.g. bugfix), or
> b) seem beneficial (code cleanup)
> as long as they don't break the driver. This patch may break the driver.

Any patch falling under a) or b) may break a driver. The patch at hand 
isn't even particularly fragile. People keep posting clean-up patches 
for some of the fbdev drivers and I doubt that most of them have seen 
any testing.

My point here is that if a patch breaks the driver then someone will 
show up and report the regression. If you operate under the (implicit) 
assumption that the driver breaks without anyone reporting the 
regression, the corollary is that the driver is unused.  And so should 
be removed. I suspect this is the case for a lot of fbdev drivers. Hence 
we should make a push to get drivers removed.

Best regards
Thomas

>
> Helge

-- 
--
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)