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

[Thomas Zimmermann <tzimmermann@suse.de>]

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)