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

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)