* [PATCH 2/5] fbdev: nvidia: Convert to managed PCI and ioremap API
2026-08-13 12:50 [PATCH 1/5] fbdev: aty128fb: Convert to managed PCI and ioremap API oushixiong1025
@ 2026-08-13 12:50 ` oushixiong1025
2026-08-13 12:50 ` [PATCH 3/5] fbdev: savage: " oushixiong1025
` (4 subsequent siblings)
5 siblings, 0 replies; 9+ messages in thread
From: oushixiong1025 @ 2026-08-13 12:50 UTC (permalink / raw)
To: Helge Deller
Cc: Thomas Zimmermann, Abdun Nihaal, u.kleine-koenig, linux-fbdev,
dri-devel, linux-kernel, Shixiong Ou
From: Shixiong Ou <oushixiong@kylinos.cn>
Fix missing pci_disable_device() in probe and remove.
Use pcim_enable_device(), pcim_request_all_regions(),
devm_ioremap() and devm_ioremap_wc() to replace manual resource
management. Remove all pci_release_regions() and iounmap() calls.
Signed-off-by: Shixiong Ou <oushixiong@kylinos.cn>
---
drivers/video/fbdev/nvidia/nvidia.c | 19 ++++++-------------
1 file changed, 6 insertions(+), 13 deletions(-)
diff --git a/drivers/video/fbdev/nvidia/nvidia.c b/drivers/video/fbdev/nvidia/nvidia.c
index 7d20c4087aeb..4ef7a5ba43cc 100644
--- a/drivers/video/fbdev/nvidia/nvidia.c
+++ b/drivers/video/fbdev/nvidia/nvidia.c
@@ -1292,7 +1292,7 @@ static int nvidiafb_probe(struct pci_dev *pd, const struct pci_device_id *ent)
NVTRACE_ENTER();
assert(pd != NULL);
- if (pci_enable_device(pd)) {
+ if (pcim_enable_device(pd)) {
printk(KERN_ERR PFX "cannot enable PCI device\n");
return -ENODEV;
}
@@ -1305,7 +1305,7 @@ static int nvidiafb_probe(struct pci_dev *pd, const struct pci_device_id *ent)
nvidiafb_fix.mmio_start = pci_resource_start(pd, 0);
nvidiafb_fix.mmio_len = pci_resource_len(pd, 0);
- REGS = ioremap(nvidiafb_fix.mmio_start, nvidiafb_fix.mmio_len);
+ REGS = devm_ioremap(&pd->dev, nvidiafb_fix.mmio_start, nvidiafb_fix.mmio_len);
if (!REGS) {
printk(KERN_ERR PFX "cannot ioremap MMIO base\n");
return -ENODEV;
@@ -1333,7 +1333,7 @@ static int nvidiafb_probe(struct pci_dev *pd, const struct pci_device_id *ent)
if (info->pixmap.addr == NULL)
goto err_out_kfree;
- if (pci_request_regions(pd, "nvidiafb")) {
+ if (pcim_request_all_regions(pd, "nvidiafb")) {
printk(KERN_ERR PFX "cannot request PCI regions\n");
goto err_out_enable;
}
@@ -1358,7 +1358,7 @@ static int nvidiafb_probe(struct pci_dev *pd, const struct pci_device_id *ent)
sprintf(nvidiafb_fix.id, "NV%x", (pd->device & 0x0ff0) >> 4);
if (NVCommonSetup(info))
- goto err_out_free_base0;
+ goto err_out_enable;
par->FbAddress = nvidiafb_fix.smem_start;
par->FbMapSize = par->RamAmountKBytes * 1024;
@@ -1378,8 +1378,8 @@ static int nvidiafb_probe(struct pci_dev *pd, const struct pci_device_id *ent)
par->ScratchBufferStart = par->FbUsableSize - par->ScratchBufferSize;
par->CursorStart = par->FbUsableSize + (32 * 1024);
- info->screen_base = ioremap_wc(nvidiafb_fix.smem_start,
- par->FbMapSize);
+ info->screen_base = devm_ioremap_wc(&pd->dev, nvidiafb_fix.smem_start,
+ par->FbMapSize);
info->screen_size = par->FbUsableSize;
nvidiafb_fix.smem_len = par->RamAmountKBytes * 1024;
@@ -1423,19 +1423,15 @@ static int nvidiafb_probe(struct pci_dev *pd, const struct pci_device_id *ent)
return 0;
err_out_iounmap_fb:
- iounmap(info->screen_base);
fb_destroy_modelist(&info->modelist);
err_out_free_base1:
fb_destroy_modedb(info->monspecs.modedb);
nvidia_delete_i2c_busses(par);
-err_out_free_base0:
- pci_release_regions(pd);
err_out_enable:
kfree(info->pixmap.addr);
err_out_kfree:
framebuffer_release(info);
err_out:
- iounmap(REGS);
return -ENODEV;
}
@@ -1450,11 +1446,8 @@ static void nvidiafb_remove(struct pci_dev *pd)
unregister_framebuffer(info);
arch_phys_wc_del(par->wc_cookie);
- iounmap(info->screen_base);
fb_destroy_modedb(info->monspecs.modedb);
nvidia_delete_i2c_busses(par);
- iounmap(par->REGS);
- pci_release_regions(pd);
kfree(info->pixmap.addr);
framebuffer_release(info);
NVTRACE_LEAVE();
--
2.25.1
No virus found
Checked by Hillstone Network AntiVirus
^ permalink raw reply related [flat|nested] 9+ messages in thread* [PATCH 3/5] fbdev: savage: Convert to managed PCI and ioremap API
2026-08-13 12:50 [PATCH 1/5] fbdev: aty128fb: Convert to managed PCI and ioremap API oushixiong1025
2026-08-13 12:50 ` [PATCH 2/5] fbdev: nvidia: " oushixiong1025
@ 2026-08-13 12:50 ` oushixiong1025
2026-08-13 12:50 ` [PATCH 4/5] fbdev: matrox: " oushixiong1025
` (3 subsequent siblings)
5 siblings, 0 replies; 9+ messages in thread
From: oushixiong1025 @ 2026-08-13 12:50 UTC (permalink / raw)
To: Helge Deller
Cc: Thomas Zimmermann, Abdun Nihaal, u.kleine-koenig, linux-fbdev,
dri-devel, linux-kernel, Shixiong Ou
From: Shixiong Ou <oushixiong@kylinos.cn>
Fix missing pci_disable_device() in probe and remove.
Use pcim_enable_device(), pcim_request_all_regions(),
devm_ioremap() and devm_ioremap_wc() to replace manual resource
management. Remove all pci_release_regions() and iounmap() calls.
Merge failed_init label into failed_enable.
Signed-off-by: Shixiong Ou <oushixiong@kylinos.cn>
---
drivers/video/fbdev/savage/savagefb_driver.c | 16 ++++++----------
1 file changed, 6 insertions(+), 10 deletions(-)
diff --git a/drivers/video/fbdev/savage/savagefb_driver.c b/drivers/video/fbdev/savage/savagefb_driver.c
index 7789196d2eb5..56ff3c50cd29 100644
--- a/drivers/video/fbdev/savage/savagefb_driver.c
+++ b/drivers/video/fbdev/savage/savagefb_driver.c
@@ -1728,7 +1728,7 @@ static int savage_map_mmio(struct fb_info *info)
par->mmio.len = SAVAGE_NEWMMIO_REGSIZE;
- par->mmio.vbase = ioremap(par->mmio.pbase, par->mmio.len);
+ par->mmio.vbase = devm_ioremap(&par->pcidev->dev, par->mmio.pbase, par->mmio.len);
if (!par->mmio.vbase) {
printk("savagefb: unable to map memory mapped IO\n");
return -ENOMEM;
@@ -1755,7 +1755,6 @@ static void savage_unmap_mmio(struct fb_info *info)
savage_disable_mmio(par);
if (par->mmio.vbase) {
- iounmap(par->mmio.vbase);
par->mmio.vbase = NULL;
}
}
@@ -1774,7 +1773,7 @@ static int savage_map_video(struct fb_info *info, int video_len)
par->video.pbase = pci_resource_start(par->pcidev, resource);
par->video.len = video_len;
- par->video.vbase = ioremap_wc(par->video.pbase, par->video.len);
+ par->video.vbase = devm_ioremap_wc(&par->pcidev->dev, par->video.pbase, par->video.len);
if (!par->video.vbase) {
printk("savagefb: unable to map screen memory\n");
@@ -1802,7 +1801,6 @@ static void savage_unmap_video(struct fb_info *info)
if (par->video.vbase) {
arch_phys_wc_del(par->video.wc_cookie);
- iounmap(par->video.vbase);
par->video.vbase = NULL;
info->screen_base = NULL;
}
@@ -2188,11 +2186,12 @@ static int savagefb_probe(struct pci_dev *dev, const struct pci_device_id *id)
return -ENOMEM;
par = info->par;
mutex_init(&par->open_lock);
- err = pci_enable_device(dev);
+ err = pcim_enable_device(dev);
if (err)
goto failed_enable;
- if ((err = pci_request_regions(dev, "savagefb"))) {
+ err = pcim_request_all_regions(dev, "savagefb");
+ if (err) {
printk(KERN_ERR "cannot request PCI regions\n");
goto failed_enable;
}
@@ -2200,7 +2199,7 @@ static int savagefb_probe(struct pci_dev *dev, const struct pci_device_id *id)
err = -ENOMEM;
if ((err = savage_init_fb_info(info, dev, id)))
- goto failed_init;
+ goto failed_enable;
err = savage_map_mmio(info);
if (err)
@@ -2331,8 +2330,6 @@ static int savagefb_probe(struct pci_dev *dev, const struct pci_device_id *id)
savage_unmap_mmio(info);
failed_mmio:
kfree(info->pixmap.addr);
- failed_init:
- pci_release_regions(dev);
failed_enable:
framebuffer_release(info);
@@ -2355,7 +2352,6 @@ static void savagefb_remove(struct pci_dev *dev)
savage_unmap_video(info);
savage_unmap_mmio(info);
kfree(info->pixmap.addr);
- pci_release_regions(dev);
framebuffer_release(info);
}
}
--
2.25.1
No virus found
Checked by Hillstone Network AntiVirus
^ permalink raw reply related [flat|nested] 9+ messages in thread* [PATCH 4/5] fbdev: matrox: Convert to managed PCI and ioremap API
2026-08-13 12:50 [PATCH 1/5] fbdev: aty128fb: Convert to managed PCI and ioremap API oushixiong1025
2026-08-13 12:50 ` [PATCH 2/5] fbdev: nvidia: " oushixiong1025
2026-08-13 12:50 ` [PATCH 3/5] fbdev: savage: " oushixiong1025
@ 2026-08-13 12:50 ` oushixiong1025
2026-08-13 13:02 ` sashiko-bot
2026-08-13 12:50 ` [PATCH 5/5] fbdev: atyfb: " oushixiong1025
` (2 subsequent siblings)
5 siblings, 1 reply; 9+ messages in thread
From: oushixiong1025 @ 2026-08-13 12:50 UTC (permalink / raw)
To: Helge Deller
Cc: Thomas Zimmermann, Abdun Nihaal, u.kleine-koenig, linux-fbdev,
dri-devel, linux-kernel, Shixiong Ou
From: Shixiong Ou <oushixiong@kylinos.cn>
Fix missing pci_disable_device() in probe and remove.
Use pcim_enable_device(), devm_request_mem_region(),
devm_ioremap() and devm_ioremap_wc() to replace manual resource
management. Remove all release_mem_region() and iounmap() calls.
Use devm_request_mem_region() instead of pcim_request_region()
because the requested sizes (16384 for MMIO, maxvram for FB) do
not match the full PCI BAR sizes.
Signed-off-by: Shixiong Ou <oushixiong@kylinos.cn>
---
drivers/video/fbdev/matrox/matroxfb_base.c | 30 ++++++++--------------
1 file changed, 10 insertions(+), 20 deletions(-)
diff --git a/drivers/video/fbdev/matrox/matroxfb_base.c b/drivers/video/fbdev/matrox/matroxfb_base.c
index ac04a19b6849..24d312c29fb6 100644
--- a/drivers/video/fbdev/matrox/matroxfb_base.c
+++ b/drivers/video/fbdev/matrox/matroxfb_base.c
@@ -374,10 +374,6 @@ static void matroxfb_remove(struct matrox_fb_info *minfo, int dummy)
unregister_framebuffer(&minfo->fbcon);
matroxfb_g450_shutdown(minfo);
arch_phys_wc_del(minfo->wc_cookie);
- iounmap(minfo->mmio.vbase.vaddr);
- iounmap(minfo->video.vbase.vaddr);
- release_mem_region(minfo->video.base, minfo->video.len_maximum);
- release_mem_region(minfo->mmio.base, 16384);
kfree(minfo);
}
@@ -1712,11 +1708,13 @@ static int initMatrox2(struct matrox_fb_info *minfo, struct board *b)
goto fail;
}
memsize = b->base->maxvram;
- if (!request_mem_region(ctrlptr_phys, 16384, "matroxfb MMIO")) {
+ if (!devm_request_mem_region(&minfo->pcidev->dev, ctrlptr_phys, 16384,
+ "matroxfb MMIO")) {
goto fail;
}
- if (!request_mem_region(video_base_phys, memsize, "matroxfb FB")) {
- goto failCtrlMR;
+ if (!devm_request_mem_region(&minfo->pcidev->dev, video_base_phys,
+ memsize, "matroxfb FB")) {
+ goto fail;
}
minfo->video.len_maximum = memsize;
/* convert mem (autodetect k, M) */
@@ -1727,19 +1725,19 @@ static int initMatrox2(struct matrox_fb_info *minfo, struct board *b)
memsize = mem;
err = -ENOMEM;
- minfo->mmio.vbase.vaddr = ioremap(ctrlptr_phys, 16384);
+ minfo->mmio.vbase.vaddr = devm_ioremap(&minfo->pcidev->dev, ctrlptr_phys, 16384);
if (!minfo->mmio.vbase.vaddr) {
printk(KERN_ERR "matroxfb: cannot ioremap(%lX, 16384), matroxfb disabled\n", ctrlptr_phys);
- goto failVideoMR;
+ goto fail;
}
minfo->mmio.base = ctrlptr_phys;
minfo->mmio.len = 16384;
minfo->video.base = video_base_phys;
- minfo->video.vbase.vaddr = ioremap_wc(video_base_phys, memsize);
+ minfo->video.vbase.vaddr = devm_ioremap_wc(&minfo->pcidev->dev, video_base_phys, memsize);
if (!minfo->video.vbase.vaddr) {
printk(KERN_ERR "matroxfb: cannot ioremap(%lX, %d), matroxfb disabled\n",
video_base_phys, memsize);
- goto failCtrlIO;
+ goto fail;
}
{
u_int32_t cmd;
@@ -1954,13 +1952,6 @@ static int initMatrox2(struct matrox_fb_info *minfo, struct board *b)
return 0;
failVideoIO:;
matroxfb_g450_shutdown(minfo);
- iounmap(minfo->video.vbase.vaddr);
-failCtrlIO:;
- iounmap(minfo->mmio.vbase.vaddr);
-failVideoMR:;
- release_mem_region(video_base_phys, minfo->video.len_maximum);
-failCtrlMR:;
- release_mem_region(ctrlptr_phys, 16384);
fail:;
return err;
}
@@ -2069,9 +2060,8 @@ static int matroxfb_probe(struct pci_dev* pdev, const struct pci_device_id* dumm
return -ENODEV;
}
pci_read_config_dword(pdev, PCI_COMMAND, &cmd);
- if (pci_enable_device(pdev)) {
+ if (pcim_enable_device(pdev))
return -1;
- }
minfo = kzalloc_obj(*minfo);
if (!minfo)
--
2.25.1
No virus found
Checked by Hillstone Network AntiVirus
^ permalink raw reply related [flat|nested] 9+ messages in thread* Re: [PATCH 4/5] fbdev: matrox: Convert to managed PCI and ioremap API
2026-08-13 12:50 ` [PATCH 4/5] fbdev: matrox: " oushixiong1025
@ 2026-08-13 13:02 ` sashiko-bot
0 siblings, 0 replies; 9+ messages in thread
From: sashiko-bot @ 2026-08-13 13:02 UTC (permalink / raw)
To: oushixiong1025; +Cc: dri-devel
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Crash/UAF due to premature devres cleanup during deferred driver teardown.
--
--- Patch [4]: [PATCH 4/5] fbdev: matrox: Convert to managed PCI and ioremap API ---
commit 3f4d09fdf1478a2c901bd635a21ab3e200899c5a
Author: Shixiong Ou <oushixiong@kylinos.cn>
fbdev: matrox: Convert to managed PCI and ioremap API
This commit converts the matroxfb driver to use managed device resource
APIs such as pcim_enable_device, devm_request_mem_region, and devm_ioremap.
It removes the manual release_mem_region and iounmap calls from the remove
path and error handling.
> diff --git a/drivers/video/fbdev/matrox/matroxfb_base.c b/drivers/video/fbdev/matrox/matroxfb_base.c
> index ac04a19b68492..24d312c29fb64 100644
> --- a/drivers/video/fbdev/matrox/matroxfb_base.c
> +++ b/drivers/video/fbdev/matrox/matroxfb_base.c
> @@ -374,10 +374,6 @@ static void matroxfb_remove(struct matrox_fb_info *minfo, int dummy)
> unregister_framebuffer(&minfo->fbcon);
> matroxfb_g450_shutdown(minfo);
> arch_phys_wc_del(minfo->wc_cookie);
> - iounmap(minfo->mmio.vbase.vaddr);
> - iounmap(minfo->video.vbase.vaddr);
> - release_mem_region(minfo->video.base, minfo->video.len_maximum);
> - release_mem_region(minfo->mmio.base, 16384);
> kfree(minfo);
> }
[Severity: High]
Will this cause a crash if the device is removed while still in use?
Looking earlier in matroxfb_remove(), there is an early return that defers
cleanup if the device is busy:
static void matroxfb_remove(struct matrox_fb_info *minfo, int dummy)
{
...
minfo->dead = 1;
if (minfo->usecount) {
/* destroy it later */
return;
}
...
}
By converting the allocations to the devm_ and pcim_ APIs, the resources
like the MMIO mappings and the PCI enable state will be automatically freed
as soon as matroxfb_remove() returns to the driver core.
If minfo->usecount is non-zero, won't this unmap and disable the device
while userspace or other subsystems are still actively interacting with the
framebuffer, leading to a memory fault?
[ ... ]
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260813125015.1097301-1-oushixiong1025@163.com?part=4
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH 5/5] fbdev: atyfb: Convert to managed PCI and ioremap API
2026-08-13 12:50 [PATCH 1/5] fbdev: aty128fb: Convert to managed PCI and ioremap API oushixiong1025
` (2 preceding siblings ...)
2026-08-13 12:50 ` [PATCH 4/5] fbdev: matrox: " oushixiong1025
@ 2026-08-13 12:50 ` oushixiong1025
2026-08-13 13:13 ` sashiko-bot
2026-08-13 12:57 ` [PATCH 1/5] fbdev: aty128fb: " sashiko-bot
2026-08-13 15:45 ` Helge Deller
5 siblings, 1 reply; 9+ messages in thread
From: oushixiong1025 @ 2026-08-13 12:50 UTC (permalink / raw)
To: Helge Deller
Cc: Thomas Zimmermann, Abdun Nihaal, u.kleine-koenig, linux-fbdev,
dri-devel, linux-kernel, Shixiong Ou
From: Shixiong Ou <oushixiong@kylinos.cn>
Fix missing pci_disable_device() in probe and remove.
Use pcim_enable_device(), pcim_request_region(), devm_ioremap(),
devm_ioremap_uc() and devm_ioremap_wc() for the PCI path. Convert
aux_start to devm_request_mem_region(). Guard atyfb_remove() to
only unmap/release for non-PCI (Atari) devices. Keep iounmap for
sprite.addr outside the guard since it uses raw ioremap().
Signed-off-by: Shixiong Ou <oushixiong@kylinos.cn>
---
drivers/video/fbdev/aty/atyfb_base.c | 47 +++++++++++-----------------
1 file changed, 18 insertions(+), 29 deletions(-)
diff --git a/drivers/video/fbdev/aty/atyfb_base.c b/drivers/video/fbdev/aty/atyfb_base.c
index 9fc5af09f86c..97cc8b6a9361 100644
--- a/drivers/video/fbdev/aty/atyfb_base.c
+++ b/drivers/video/fbdev/aty/atyfb_base.c
@@ -3435,7 +3435,7 @@ static int atyfb_setup_generic(struct pci_dev *pdev, struct fb_info *info,
raddr = addr + 0x7ff000UL;
rrp = &pdev->resource[2];
if ((rrp->flags & IORESOURCE_MEM) &&
- request_mem_region(rrp->start, resource_size(rrp), "atyfb")) {
+ devm_request_mem_region(&pdev->dev, rrp->start, resource_size(rrp), "atyfb")) {
par->aux_start = rrp->start;
par->aux_size = resource_size(rrp);
raddr = rrp->start;
@@ -3448,9 +3448,9 @@ static int atyfb_setup_generic(struct pci_dev *pdev, struct fb_info *info,
* By using strong UC we force the MTRR to never have an
* effect on the MMIO region on both non-PAT and PAT systems.
*/
- par->ati_regbase = ioremap_uc(info->fix.mmio_start, 0x1000);
+ par->ati_regbase = devm_ioremap_uc(&pdev->dev, info->fix.mmio_start, 0x1000);
#else
- par->ati_regbase = ioremap(info->fix.mmio_start, 0x1000);
+ par->ati_regbase = devm_ioremap(&pdev->dev, info->fix.mmio_start, 0x1000);
#endif
if (par->ati_regbase == NULL)
return -ENOMEM;
@@ -3490,8 +3490,8 @@ static int atyfb_setup_generic(struct pci_dev *pdev, struct fb_info *info,
aty_fudge_framebuffer_len(info);
- info->screen_base = ioremap_wc(info->fix.smem_start,
- info->fix.smem_len);
+ info->screen_base = devm_ioremap_wc(&pdev->dev, info->fix.smem_start,
+ info->fix.smem_len);
if (info->screen_base == NULL) {
ret = -ENOMEM;
goto atyfb_setup_generic_fail;
@@ -3511,12 +3511,9 @@ static int atyfb_setup_generic(struct pci_dev *pdev, struct fb_info *info,
return 0;
atyfb_setup_generic_fail:
- iounmap(par->ati_regbase);
+ /* devm handles cleanup automatically on probe failure */
par->ati_regbase = NULL;
- if (info->screen_base) {
- iounmap(info->screen_base);
- info->screen_base = NULL;
- }
+ info->screen_base = NULL;
return ret;
}
@@ -3536,7 +3533,7 @@ static int atyfb_pci_probe(struct pci_dev *pdev,
return rc;
/* Enable device in PCI config */
- if (pci_enable_device(pdev)) {
+ if (pcim_enable_device(pdev)) {
PRINTKE("Cannot enable PCI device\n");
return -ENXIO;
}
@@ -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;
/* Allocate framebuffer */
@@ -3612,17 +3609,9 @@ static int atyfb_pci_probe(struct pci_dev *pdev,
err_release_io:
#ifdef __sparc__
kfree(par->mmap_map);
-#else
- if (par->ati_regbase)
- iounmap(par->ati_regbase);
- if (info->screen_base)
- iounmap(info->screen_base);
#endif
+ /* devm handles cleanup automatically for non-sparc PCI devices */
err_release_mem:
- if (par->aux_start)
- release_mem_region(par->aux_start, par->aux_size);
-
- release_mem_region(par->res_start, par->res_size);
framebuffer_release(info);
return rc;
@@ -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);
+ if (info->screen_base)
+ iounmap(info->screen_base);
+ }
#ifdef __BIG_ENDIAN
if (info->sprite.addr)
iounmap(info->sprite.addr);
@@ -3745,10 +3737,7 @@ static void atyfb_remove(struct fb_info *info)
#ifdef __sparc__
kfree(par->mmap_map);
#endif
- if (par->aux_start)
- release_mem_region(par->aux_start, par->aux_size);
-
- if (par->res_start)
+ if (par->res_start && par->bus_type != PCI)
release_mem_region(par->res_start, par->res_size);
framebuffer_release(info);
--
2.25.1
No virus found
Checked by Hillstone Network AntiVirus
^ permalink raw reply related [flat|nested] 9+ messages in thread* Re: [PATCH 5/5] fbdev: atyfb: Convert to managed PCI and ioremap API
2026-08-13 12:50 ` [PATCH 5/5] fbdev: atyfb: " oushixiong1025
@ 2026-08-13 13:13 ` sashiko-bot
0 siblings, 0 replies; 9+ messages in thread
From: sashiko-bot @ 2026-08-13 13:13 UTC (permalink / raw)
To: oushixiong1025; +Cc: dri-devel
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 <oushixiong@kylinos.cn>
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/20260813125015.1097301-1-oushixiong1025@163.com?part=5
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/5] fbdev: aty128fb: Convert to managed PCI and ioremap API
2026-08-13 12:50 [PATCH 1/5] fbdev: aty128fb: Convert to managed PCI and ioremap API oushixiong1025
` (3 preceding siblings ...)
2026-08-13 12:50 ` [PATCH 5/5] fbdev: atyfb: " oushixiong1025
@ 2026-08-13 12:57 ` sashiko-bot
2026-08-13 15:45 ` Helge Deller
5 siblings, 0 replies; 9+ messages in thread
From: sashiko-bot @ 2026-08-13 12:57 UTC (permalink / raw)
To: oushixiong1025; +Cc: dri-devel
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 <oushixiong@kylinos.cn>
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/20260813125015.1097301-1-oushixiong1025@163.com?part=1
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH 1/5] fbdev: aty128fb: Convert to managed PCI and ioremap API
2026-08-13 12:50 [PATCH 1/5] fbdev: aty128fb: Convert to managed PCI and ioremap API oushixiong1025
` (4 preceding siblings ...)
2026-08-13 12:57 ` [PATCH 1/5] fbdev: aty128fb: " sashiko-bot
@ 2026-08-13 15:45 ` Helge Deller
5 siblings, 0 replies; 9+ messages in thread
From: Helge Deller @ 2026-08-13 15:45 UTC (permalink / raw)
To: oushixiong1025; +Cc: linux-fbdev, dri-devel, linux-kernel
On 8/13/26 14:50, oushixiong1025@163.com wrote:
> From: Shixiong Ou <oushixiong@kylinos.cn>
>
> 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.
> Remove all release_mem_region() and iounmap() calls.
>
> Signed-off-by: Shixiong Ou <oushixiong@kylinos.cn>
> ---
> drivers/video/fbdev/aty/aty128fb.c | 40 +++++++++---------------------
> 1 file changed, 12 insertions(+), 28 deletions(-)
Whole series applied to fbdev for-next git tree.
Thanks!
Helge
^ permalink raw reply [flat|nested] 9+ messages in thread