[PATCH v1] fbdev: savage: fix probe-path EDID cleanup leaks

[PATCH v1] fbdev: savage: fix probe-path EDID cleanup leaks

[Yuho Choi]

When CONFIG_FB_SAVAGE_I2C is enabled, savagefb_probe() can build both an
EDID-derived monspecs.modedb and a modelist from it before later
failing.

The normal success path frees monspecs.modedb after the initial mode
selection, but the probe error path only deletes the I2C busses and
misses the EDID-derived allocations.

Free both the modelist and monspecs.modedb on the failed: unwind path.

Co-developed-by: Myeonghun Pak <mhun512@gmail.com>
Signed-off-by: Myeonghun Pak <mhun512@gmail.com>
Co-developed-by: Ijae Kim <ae878000@gmail.com>
Signed-off-by: Ijae Kim <ae878000@gmail.com>
Co-developed-by: Taegyu Kim <tmk5904@psu.edu>
Signed-off-by: Taegyu Kim <tmk5904@psu.edu>
Signed-off-by: Yuho Choi <dbgh9129@gmail.com>
---
 drivers/video/fbdev/savage/savagefb_driver.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/video/fbdev/savage/savagefb_driver.c b/drivers/video/fbdev/savage/savagefb_driver.c
index ac41f8f37589f..c2f79357c8da0 100644
--- a/drivers/video/fbdev/savage/savagefb_driver.c
+++ b/drivers/video/fbdev/savage/savagefb_driver.c
@@ -2322,6 +2322,8 @@ static int savagefb_probe(struct pci_dev *dev, const struct pci_device_id *id)
  failed:
 #ifdef CONFIG_FB_SAVAGE_I2C
 	savagefb_delete_i2c_busses(info);
+	fb_destroy_modelist(&info->modelist);
+	fb_destroy_modedb(info->monspecs.modedb);
 #endif
 	fb_alloc_cmap(&info->cmap, 0, 0);
 	savage_unmap_video(info);
-- 
2.50.1 (Apple Git-155)

Claude review: fbdev: savage: fix probe-path EDID cleanup leaks

[Claude Code Review Bot]

Overall Series Review

Subject: fbdev: savage: fix probe-path EDID cleanup leaks
Author: Yuho Choi <dbgh9129@gmail.com>
Patches: 1
Reviewed: 2026-04-23T10:23:48.511522

---

This is a single-patch series that fixes a real memory leak in the savagefb probe error path. The analysis is correct: when `CONFIG_FB_SAVAGE_I2C` is enabled, `savagefb_probe()` allocates both `info->monspecs.modedb` (via `fb_edid_to_monspecs`) and modelist entries (via `fb_videomode_to_modelist`), but the `failed:` cleanup label only deleted the I2C busses without freeing these allocations.

The fix is minimal, correct, and safe against double-free. **Recommend accept**, with one minor observation.

---
Generated by Claude Code Patch Reviewer

Claude review: fbdev: savage: fix probe-path EDID cleanup leaks

[Claude Code Review Bot]

Patch Review

**Correctness: Good.** The patch adds the two missing cleanup calls in the right place:

```c
 failed:
#ifdef CONFIG_FB_SAVAGE_I2C
	savagefb_delete_i2c_busses(info);
+	fb_destroy_modelist(&info->modelist);
+	fb_destroy_modedb(info->monspecs.modedb);
#endif
```

Key points verified:

1. **All `goto failed` sites (lines 2265, 2281, 2310) occur after the EDID/I2C setup block (lines 2221–2228)**, so the allocations exist whenever we reach the `failed:` label. Earlier failures use distinct labels (`failed_video`, `failed_mmio`, `failed_init`, `failed_enable`).

2. **No double-free risk.** The one path that could cause trouble is line 2310 (`register_framebuffer` failure), which is reached after the success path already freed `monspecs.modedb` and set it to NULL at lines 2305–2306. But `fb_destroy_modedb` is just `kfree()` (line 697 of `fbmon.c`), which is a no-op on NULL. The modelist is NOT freed on the success path before `register_framebuffer`, so `fb_destroy_modelist` is also safe there.

3. **Placement inside `#ifdef CONFIG_FB_SAVAGE_I2C` is correct** since the allocations only happen under that same guard (lines 2221–2228).

**Minor observation (not a blocker):** The existing `savagefb_remove()` function (line 2342) also doesn't call `fb_destroy_modelist()`. On the success path, `monspecs.modedb` is freed at line 2305, but the modelist entries created at line 2226 are never freed during normal remove. This is a pre-existing leak unrelated to this patch — but the author may want to address it in a follow-up.

**Commit message:** Accurate and well-written. Clearly describes the problem, the root cause, and the fix.

---
Generated by Claude Code Patch Reviewer