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

[Thomas Zimmermann <tzimmermann@suse.de>]

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)