[PATCH] drm/sysfb: efidrm: Fix framebuffer mapping error handling

[PATCH] drm/sysfb: efidrm: Fix framebuffer mapping error handling

[Chen Ni]

devm_memremap() returns an error pointer on failure, not NULL. Using
!screen_base to check for errors will miss error codes and treat invalid
pointers as valid addresses.

devm_memremap() returns system memory pointers (void *), while
devm_ioremap() returns I/O memory pointers (void __iomem *). The current
code incorrectly passes system memory pointers to
iosys_map_set_vaddr_iomem(), which expects I/O memory.

Fix these issues by:
- Properly checking IS_ERR() for devm_memremap() return values.
- Using iosys_map_set_vaddr_iomem() for I/O memory mappings (WC/UC).
- Using iosys_map_set_vaddr() for system memory mappings (WT/WB).

Fixes: 32ae90c66fb6 ("drm/sysfb: Add efidrm for EFI displays")
Signed-off-by: Chen Ni <nichen@iscas.ac.cn>
---
 drivers/gpu/drm/sysfb/efidrm.c | 42 ++++++++++++++++++++++++----------
 1 file changed, 30 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/sysfb/efidrm.c b/drivers/gpu/drm/sysfb/efidrm.c
index 1114359a1e62..a1da841a8dd4 100644
--- a/drivers/gpu/drm/sysfb/efidrm.c
+++ b/drivers/gpu/drm/sysfb/efidrm.c
@@ -158,6 +158,7 @@ static struct efidrm_device *efidrm_device_create(struct drm_driver *drv,
 	struct drm_device *dev;
 	struct resource *mem = NULL;
 	void __iomem *screen_base = NULL;
+	void *sys_base = NULL;
 	struct drm_plane *primary_plane;
 	struct drm_crtc *crtc;
 	struct drm_encoder *encoder;
@@ -165,6 +166,7 @@ static struct efidrm_device *efidrm_device_create(struct drm_driver *drv,
 	unsigned long max_width, max_height;
 	size_t nformats;
 	int ret;
+	bool is_iomem = false;
 
 	dpy = dev_get_platdata(&pdev->dev);
 	if (!dpy)
@@ -244,21 +246,37 @@ static struct efidrm_device *efidrm_device_create(struct drm_driver *drv,
 
 	mem_flags = efidrm_get_mem_flags(dev, res->start, vsize);
 
-	if (mem_flags & EFI_MEMORY_WC)
+	if (mem_flags & EFI_MEMORY_WC) {
 		screen_base = devm_ioremap_wc(&pdev->dev, mem->start, resource_size(mem));
-	else if (mem_flags & EFI_MEMORY_UC)
+		is_iomem = true;
+		if (!screen_base)
+			return ERR_PTR(-ENOMEM);
+	} else if (mem_flags & EFI_MEMORY_UC) {
 		screen_base = devm_ioremap(&pdev->dev, mem->start, resource_size(mem));
-	else if (mem_flags & EFI_MEMORY_WT)
-		screen_base = devm_memremap(&pdev->dev, mem->start, resource_size(mem),
-					    MEMREMAP_WT);
-	else if (mem_flags & EFI_MEMORY_WB)
-		screen_base = devm_memremap(&pdev->dev, mem->start, resource_size(mem),
-					    MEMREMAP_WB);
-	else
+		is_iomem = true;
+		if (!screen_base)
+			return ERR_PTR(-ENOMEM);
+	} else if (mem_flags & EFI_MEMORY_WT) {
+		sys_base = devm_memremap(&pdev->dev, mem->start, resource_size(mem),
+					 MEMREMAP_WT);
+		is_iomem = false;
+		if (IS_ERR(sys_base))
+			return sys_base;
+	} else if (mem_flags & EFI_MEMORY_WB) {
+		sys_base = devm_memremap(&pdev->dev, mem->start, resource_size(mem),
+					 MEMREMAP_WB);
+		is_iomem = false;
+		if (IS_ERR(sys_base))
+			return sys_base;
+	} else {
 		drm_err(dev, "invalid mem_flags: 0x%llx\n", mem_flags);
-	if (!screen_base)
-		return ERR_PTR(-ENOMEM);
-	iosys_map_set_vaddr_iomem(&sysfb->fb_addr, screen_base);
+		return ERR_PTR(-EINVAL);
+	}
+
+	if (is_iomem)
+		iosys_map_set_vaddr_iomem(&sysfb->fb_addr, screen_base);
+	else
+		iosys_map_set_vaddr(&sysfb->fb_addr, sys_base);
 
 	/*
 	 * Modesetting
-- 
2.25.1

Claude review: drm/sysfb: efidrm: Fix framebuffer mapping error handling

[Claude Code Review Bot]

Overall Series Review

Subject: drm/sysfb: efidrm: Fix framebuffer mapping error handling
Author: Chen Ni <nichen@iscas.ac.cn>
Patches: 1
Reviewed: 2026-03-09T09:07:28.386548

---

This is a single-patch fix for the `efidrm` driver addressing two real bugs in framebuffer mapping error handling:

1. **Incorrect error check for `devm_memremap()`**: The original code used `!screen_base` (NULL check) but `devm_memremap()` returns an `ERR_PTR` on failure, not NULL. This is a genuine bug.
2. **Wrong `iosys_map` type for system memory**: The original code passed `devm_memremap()` results (plain `void *`) to `iosys_map_set_vaddr_iomem()`, which expects `void __iomem *`. The fix correctly distinguishes between I/O memory and system memory mappings.

The fix also improves the `else` branch (invalid mem_flags) by returning an error instead of falling through to a NULL check.

Overall the patch is correct and addresses real issues. There is one type-safety concern worth noting.

---
Generated by Claude Code Patch Reviewer

Claude review: drm/sysfb: efidrm: Fix framebuffer mapping error handling

[Claude Code Review Bot]

Patch Review

**Type mismatch on error return path (minor issue):**

```c
	} else if (mem_flags & EFI_MEMORY_WT) {
		sys_base = devm_memremap(&pdev->dev, mem->start, resource_size(mem),
					 MEMREMAP_WT);
		is_iomem = false;
		if (IS_ERR(sys_base))
			return sys_base;
```

The function returns `struct efidrm_device *`, but `sys_base` is `void *`. While `ERR_PTR` values will work in practice (they're just error-encoding pointers), the correct pattern would be `return ERR_CAST(sys_base)` to avoid an implicit pointer type conversion. This applies to both the WT and WB cases (lines 264 and 270 in the patched file). The compiler may produce a warning about this with strict settings.

**Error handling improvement in the `else` branch — good:**

The original code would print an error for invalid `mem_flags` but then fall through to `if (!screen_base) return ERR_PTR(-ENOMEM)`, which would return the wrong error code (`-ENOMEM` instead of something more appropriate). The patch now correctly returns `-EINVAL` immediately:

```c
	} else {
		drm_err(dev, "invalid mem_flags: 0x%llx\n", mem_flags);
		return ERR_PTR(-EINVAL);
	}
```

**Correctness of `iosys_map` usage — good:**

The separation into `screen_base` (for ioremap paths) and `sys_base` (for memremap paths) with the corresponding `iosys_map_set_vaddr_iomem()` vs `iosys_map_set_vaddr()` is the right approach. The `iosys_map` abstraction specifically tracks whether the mapped region is I/O memory or system memory, and the original code was setting everything as I/O memory incorrectly.

**Summary:** The patch fixes real bugs and the approach is sound. The only nit is using `return sys_base` instead of `return ERR_CAST(sys_base)` for the type-mismatched error returns.

---
Generated by Claude Code Patch Reviewer