Re: [PATCH 1/5] fbdev: aty128fb: Convert to managed PCI and ioremap API

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Incorrect check of pcim_request_region() return value treats success as failure.
- [High] The error path for a failed MMIO region request jumps to a cleanup label that calls `framebuffer_release()` with an uninitialized `info` pointer.
--

--- Patch [1]: [PATCH 1/5] fbdev: aty128fb: Convert to managed PCI and ioremap API ---
commit 1e12b2171d8c5fcb82cbff1de0aa1745b4062be0
Author: Shixiong Ou <[email protected]>

fbdev: aty128fb: Convert to managed PCI and ioremap API

Fix missing pci_disable_device() in probe and remove.
Use pcim_enable_device(), pcim_request_region(), devm_ioremap()
and devm_ioremap_wc() to replace manual resource management.

> diff --git a/drivers/video/fbdev/aty/aty128fb.c b/drivers/video/fbdev/aty/aty128fb.c
> index bcb10e66221c8..b76bfe014f41b 100644
> --- a/drivers/video/fbdev/aty/aty128fb.c
> +++ b/drivers/video/fbdev/aty/aty128fb.c

[ ... ]

>  	fb_addr = pci_resource_start(pdev, 0);
> -	if (!request_mem_region(fb_addr, pci_resource_len(pdev, 0),
> -				"aty128fb FB")) {
> +	if (!pcim_request_region(pdev, 0, "aty128fb FB")) {

[Severity: High]
Since pcim_request_region() returns 0 on success and a negative error code
on failure, does checking it with '!' cause the driver to fail probe when
the region is actually successfully requested?

>  		printk(KERN_ERR "aty128fb: cannot reserve frame "
>  				"buffer memory\n");
>  		return -ENODEV;
>  	}
>  
>  	reg_addr = pci_resource_start(pdev, 2);
> -	if (!request_mem_region(reg_addr, pci_resource_len(pdev, 2),
> -				"aty128fb MMIO")) {
> +	if (!pcim_request_region(pdev, 2, "aty128fb MMIO")) {

[Severity: High]
Does this have the same pcim_request_region() return value check issue as
the previous block?

>  		printk(KERN_ERR "aty128fb: cannot reserve MMIO region\n");
> -		goto err_free_fb;
> +		goto err_free_info;
>  	}
>  
>  	/* We have the resources. Now virtualize them */
>  	info = framebuffer_alloc(sizeof(struct aty128fb_par), &pdev->dev);

[Severity: High]
If the above goto is taken, the code jumps to err_free_info which calls
framebuffer_release(info). Since info hasn't been allocated or initialized
yet at that point, will this dereference an uninitialized pointer during
the cleanup path?

[ ... ]

> err_free_info:
>  	framebuffer_release(info);
> -err_free_mmio:
> -	release_mem_region(pci_resource_start(pdev, 2),
> -			pci_resource_len(pdev, 2));
> -err_free_fb:
> -	release_mem_region(pci_resource_start(pdev, 0),
> -			pci_resource_len(pdev, 0));
>  	return -ENODEV;
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1