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

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

New issues:
- [High] Using a hardware-derived `bus_type` field to conditionally unmap devm-managed resources leads to double-frees and double-unmaps on PCI devices.
- [High] Incorrect error check for `pcim_request_region` causes `atyfb_pci_probe` to always fail with `-EBUSY`.

Pre-existing issues:
- [High] `iounmap` is called on a memory pointer that was offset from its base allocation address for non-PCI (Atari) devices.
--

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

fbdev: atyfb: Convert to managed PCI and ioremap API

This commit converts the atyfb driver's PCI initialization path to use
devres-managed APIs like pcim_request_region() and devm_ioremap(). It also
restricts manual cleanup during driver removal to non-PCI devices only.

> diff --git a/drivers/video/fbdev/aty/atyfb_base.c b/drivers/video/fbdev/aty/atyfb_base.c
> index 9fc5af09f86c4..97cc8b6a93614 100644
> --- a/drivers/video/fbdev/aty/atyfb_base.c
> +++ b/drivers/video/fbdev/aty/atyfb_base.c

[ ... ]

> @@ -3552,7 +3549,7 @@ static int atyfb_pci_probe(struct pci_dev *pdev,
>  	/* Reserve space */
>  	res_start = rp->start;
>  	res_size = resource_size(rp);
> -	if (!request_mem_region(res_start, res_size, "atyfb"))
> +	if (!pcim_request_region(pdev, rp - pdev->resource, "atyfb"))
>  		return -EBUSY;

[Severity: High]
Since pcim_request_region() returns 0 on success and a negative error code
on failure, will this condition cause the probe to fail unconditionally with
-EBUSY when the region is successfully requested?

[ ... ]

> @@ -3733,10 +3722,13 @@ static void atyfb_remove(struct fb_info *info)
>  	arch_phys_wc_del(par->wc_cookie);
>  
>  #ifndef __sparc__
> -	if (par->ati_regbase)
> -		iounmap(par->ati_regbase);
> -	if (info->screen_base)
> -		iounmap(info->screen_base);
> +	/* For PCI devices, devm handles unmapping automatically */
> +	if (par->bus_type != PCI) {
> +		if (par->ati_regbase)
> +			iounmap(par->ati_regbase);

[Severity: High]
Does par->bus_type reliably identify PCI devices at this point?

During device initialization in aty_init(), par->bus_type is overwritten with
a value read from a hardware register:

    par->bus_type = (stat0 >> 0) & 0x07;

If a PCI device reports a non-PCI bus type (like ISA) from the hardware
register, could this cause par->bus_type != PCI to evaluate to true,
resulting in a double-unmap of the devm-managed ati_regbase here, as well as
a double-free of the memory region later in this function?

[Severity: High]
This is a pre-existing issue, but for non-PCI devices (like Atari),
par->ati_regbase can be initialized with an offset:

    par->ati_regbase = ioremap(phys_guiregbase[m64_num], 0x10000) +
                        0xFC00ul;

Since par->ati_regbase is an offset pointer in those cases, will passing it
directly to iounmap() here cause a kernel panic?

> +		if (info->screen_base)
> +			iounmap(info->screen_base);
> +	}

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