[PATCH] fbdev: kyro: Add missing PCI memory region request

[PATCH] fbdev: kyro: Add missing PCI memory region request

[Giovanni Di Santi]

The kyro framebuffer driver did not request its PCI memory regions,
which could lead to conflicts with other drivers.  This change
addresses the task "Request memory regions in all fbdev drivers"
from the file Documentation/gpu/todo.rst.

pci_request_regions() is now called during probe. To ensure proper
cleanup, the corresponding pci_release_regions() and a missing
pci_disable_device() call are added to both the driver's remove
function and the probe's error handling path.

Signed-off-by: Giovanni Di Santi <giovanni.disanti.lkl@gmail.com>
---
 drivers/video/fbdev/kyro/fbdev.c | 23 ++++++++++++++++++-----
 1 file changed, 18 insertions(+), 5 deletions(-)

diff --git a/drivers/video/fbdev/kyro/fbdev.c b/drivers/video/fbdev/kyro/fbdev.c
index 08ee8baa79f8..80ac17337c1b 100644
--- a/drivers/video/fbdev/kyro/fbdev.c
+++ b/drivers/video/fbdev/kyro/fbdev.c
@@ -685,8 +685,14 @@ static int kyrofb_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
 	}
 
 	info = framebuffer_alloc(sizeof(struct kyrofb_info), &pdev->dev);
-	if (!info)
-		return -ENOMEM;
+	if (!info) {
+		err = -ENOMEM;
+		goto out_disable;
+	}
+
+	err = pci_request_regions(pdev, "kyrofb");
+	if (err)
+		goto out_free_fb;
 
 	currentpar = info->par;
 
@@ -695,10 +701,11 @@ static int kyrofb_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
 	kyro_fix.mmio_start = pci_resource_start(pdev, 1);
 	kyro_fix.mmio_len   = pci_resource_len(pdev, 1);
 
+	err = -EINVAL;
 	currentpar->regbase = deviceInfo.pSTGReg =
 		ioremap(kyro_fix.mmio_start, kyro_fix.mmio_len);
 	if (!currentpar->regbase)
-		goto out_free_fb;
+		goto out_release;
 
 	info->screen_base = pci_ioremap_wc_bar(pdev, 0);
 	if (!info->screen_base)
@@ -752,10 +759,13 @@ static int kyrofb_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
 	iounmap(info->screen_base);
 out_unmap_regs:
 	iounmap(currentpar->regbase);
+out_release:
+	pci_release_regions(pdev);
 out_free_fb:
 	framebuffer_release(info);
-
-	return -EINVAL;
+out_disable:
+	pci_disable_device(pdev);
+	return err;
 }
 
 static void kyrofb_remove(struct pci_dev *pdev)
@@ -780,6 +790,9 @@ static void kyrofb_remove(struct pci_dev *pdev)
 
 	unregister_framebuffer(info);
 	framebuffer_release(info);
+
+	pci_release_regions(pdev);
+	pci_disable_device(pdev);
 }
 
 static int __init kyrofb_init(void)
-- 
2.43.0

Re: [PATCH] fbdev: kyro: Add missing PCI memory region request

[Thomas Zimmermann]

Hi

Am 08.07.25 um 16:46 schrieb Giovanni Di Santi:
> The kyro framebuffer driver did not request its PCI memory regions,
> which could lead to conflicts with other drivers.  This change
> addresses the task "Request memory regions in all fbdev drivers"
> from the file Documentation/gpu/todo.rst.
>
> pci_request_regions() is now called during probe. To ensure proper
> cleanup, the corresponding pci_release_regions() and a missing
> pci_disable_device() call are added to both the driver's remove
> function and the probe's error handling path.
>
> Signed-off-by: Giovanni Di Santi <giovanni.disanti.lkl@gmail.com>
> ---
>   drivers/video/fbdev/kyro/fbdev.c | 23 ++++++++++++++++++-----
>   1 file changed, 18 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/video/fbdev/kyro/fbdev.c b/drivers/video/fbdev/kyro/fbdev.c
> index 08ee8baa79f8..80ac17337c1b 100644
> --- a/drivers/video/fbdev/kyro/fbdev.c
> +++ b/drivers/video/fbdev/kyro/fbdev.c
> @@ -685,8 +685,14 @@ static int kyrofb_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
>   	}
>   
>   	info = framebuffer_alloc(sizeof(struct kyrofb_info), &pdev->dev);
> -	if (!info)
> -		return -ENOMEM;
> +	if (!info) {
> +		err = -ENOMEM;
> +		goto out_disable;
> +	}
> +
> +	err = pci_request_regions(pdev, "kyrofb");
> +	if (err)
> +		goto out_free_fb;

Could this use pcim_request_all_regions() [1] instead? Cleanup and error 
rollback would be automatic.

[1] 
https://elixir.bootlin.com/linux/v6.15.5/source/drivers/pci/devres.c#L927


>   
>   	currentpar = info->par;
>   
> @@ -695,10 +701,11 @@ static int kyrofb_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
>   	kyro_fix.mmio_start = pci_resource_start(pdev, 1);
>   	kyro_fix.mmio_len   = pci_resource_len(pdev, 1);
>   
> +	err = -EINVAL;
>   	currentpar->regbase = deviceInfo.pSTGReg =
>   		ioremap(kyro_fix.mmio_start, kyro_fix.mmio_len);
>   	if (!currentpar->regbase)
> -		goto out_free_fb;
> +		goto out_release;
>   
>   	info->screen_base = pci_ioremap_wc_bar(pdev, 0);
>   	if (!info->screen_base)
> @@ -752,10 +759,13 @@ static int kyrofb_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
>   	iounmap(info->screen_base);
>   out_unmap_regs:
>   	iounmap(currentpar->regbase);
> +out_release:
> +	pci_release_regions(pdev);
>   out_free_fb:
>   	framebuffer_release(info);
> -
> -	return -EINVAL;
> +out_disable:
> +	pci_disable_device(pdev);
> +	return err;
>   }
>   
>   static void kyrofb_remove(struct pci_dev *pdev)
> @@ -780,6 +790,9 @@ static void kyrofb_remove(struct pci_dev *pdev)
>   
>   	unregister_framebuffer(info);
>   	framebuffer_release(info);
> +
> +	pci_release_regions(pdev);
> +	pci_disable_device(pdev);

Instead of manual cleanup, you're better off using pcim_enable_device() 
[2] in kyrofb_probe(). Cleaning up is automatic then.

Best regards
Thomas

[2] 
https://elixir.bootlin.com/linux/v6.15.5/source/drivers/pci/devres.c#L476

>   }
>   
>   static int __init kyrofb_init(void)

-- 
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)

[PATCH v2] fbdev: kyro: Add missing PCI memory region request

[Giovanni Di Santi]

Hi Thomas,

Thanks for the feedback.

On Tue, 8 Jul 2025 at 5:10, Thomas Zimmermann wrote:
> Could this use pcim_request_all_regions() [1] instead? Cleanup and error
> rollback would be automatic.
> ...
> Instead of manual cleanup, you're better off using pcim_enable_device()
> [2] in kyrofb_probe(). Cleaning up is automatic then.

I've applied the changes. I noticed that the driver still has manual
ioremap() and pci_ioremap_wc_bar() calls inside the probe function.
Should these also be converted to devm_ioremap() and devm_ioremap_wc() to
make the cleanup fully automatic? Or should I do it in another patch?

Changes in v2:
- Use pcim_enable_device() instead of pci_enable_device()
- Use pcim_request_all_regions() instead of pci_request_regions()
- Removed manual cleanup code as it's now automatic

---

The kyro framebuffer driver did not request its PCI memory regions,
which could lead to conflicts with other drivers.  This change
addresses the task "Request memory regions in all fbdev drivers"
from the file Documentation/gpu/todo.rst.

This is addressed by using the managed device functions pcim_enable_device()
and pcim_request_all_regions(). This simplifies the code by making error
handling and driver removal cleanup automatic for these resources.

Signed-off-by: Giovanni Di Santi <giovanni.disanti.lkl@gmail.com>
---
 drivers/video/fbdev/kyro/fbdev.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/video/fbdev/kyro/fbdev.c b/drivers/video/fbdev/kyro/fbdev.c
index 08ee8baa79f8..86e5d60ed0ff 100644
--- a/drivers/video/fbdev/kyro/fbdev.c
+++ b/drivers/video/fbdev/kyro/fbdev.c
@@ -679,7 +679,8 @@ static int kyrofb_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
 	if (err)
 		return err;
 
-	if ((err = pci_enable_device(pdev))) {
+	err = pcim_enable_device(pdev);
+	if (err) {
 		printk(KERN_WARNING "kyrofb: Can't enable pdev: %d\n", err);
 		return err;
 	}
@@ -688,6 +689,10 @@ static int kyrofb_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
 	if (!info)
 		return -ENOMEM;
 
+	err = pcim_request_all_regions(pdev, "kyrofb");
+	if (err)
+		goto out_free_fb;
+
 	currentpar = info->par;
 
 	kyro_fix.smem_start = pci_resource_start(pdev, 0);
-- 
2.43.0

Re: [PATCH v2] fbdev: kyro: Add missing PCI memory region request

[Thomas Zimmermann]

Hi

Am 08.07.25 um 20:57 schrieb Giovanni Di Santi:
> Hi Thomas,
>
> Thanks for the feedback.
>
> On Tue, 8 Jul 2025 at 5:10, Thomas Zimmermann wrote:
>> Could this use pcim_request_all_regions() [1] instead? Cleanup and error
>> rollback would be automatic.
>> ...
>> Instead of manual cleanup, you're better off using pcim_enable_device()
>> [2] in kyrofb_probe(). Cleaning up is automatic then.
> I've applied the changes. I noticed that the driver still has manual
> ioremap() and pci_ioremap_wc_bar() calls inside the probe function.
> Should these also be converted to devm_ioremap() and devm_ioremap_wc() to
> make the cleanup fully automatic? Or should I do it in another patch?

If you have the time and interest, you're welcome to change it to the 
devm_ functions. Ideally, you'd send a series, which each patch 
addressing a single function.

Best regards
Thomas

>
> Changes in v2:
> - Use pcim_enable_device() instead of pci_enable_device()
> - Use pcim_request_all_regions() instead of pci_request_regions()
> - Removed manual cleanup code as it's now automatic
>
> ---
>
> The kyro framebuffer driver did not request its PCI memory regions,
> which could lead to conflicts with other drivers.  This change
> addresses the task "Request memory regions in all fbdev drivers"
> from the file Documentation/gpu/todo.rst.
>
> This is addressed by using the managed device functions pcim_enable_device()
> and pcim_request_all_regions(). This simplifies the code by making error
> handling and driver removal cleanup automatic for these resources.
>
> Signed-off-by: Giovanni Di Santi <giovanni.disanti.lkl@gmail.com>
> ---
>   drivers/video/fbdev/kyro/fbdev.c | 7 ++++++-
>   1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/video/fbdev/kyro/fbdev.c b/drivers/video/fbdev/kyro/fbdev.c
> index 08ee8baa79f8..86e5d60ed0ff 100644
> --- a/drivers/video/fbdev/kyro/fbdev.c
> +++ b/drivers/video/fbdev/kyro/fbdev.c
> @@ -679,7 +679,8 @@ static int kyrofb_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
>   	if (err)
>   		return err;
>   
> -	if ((err = pci_enable_device(pdev))) {
> +	err = pcim_enable_device(pdev);
> +	if (err) {
>   		printk(KERN_WARNING "kyrofb: Can't enable pdev: %d\n", err);
>   		return err;
>   	}
> @@ -688,6 +689,10 @@ static int kyrofb_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
>   	if (!info)
>   		return -ENOMEM;
>   
> +	err = pcim_request_all_regions(pdev, "kyrofb");
> +	if (err)
> +		goto out_free_fb;
> +
>   	currentpar = info->par;
>   
>   	kyro_fix.smem_start = pci_resource_start(pdev, 0);

-- 
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)