From mboxrd@z Thu Jan  1 00:00:00 1970
From: Claude Code Review Bot <claude-review@example.com>
To: dri-devel-reviews@example.com
Subject: Claude review: drm/sysfb: efidrm: Fix framebuffer mapping error handling
Date: Mon, 09 Mar 2026 09:07:28 +1000
Message-ID: <review-patch1-20260306091927.1934860-1-nichen@iscas.ac.cn>
In-Reply-To: <20260306091927.1934860-1-nichen@iscas.ac.cn>
References: <20260306091927.1934860-1-nichen@iscas.ac.cn> <20260306091927.1934860-1-nichen@iscas.ac.cn>
X-Mailer: Claude Code Patch Reviewer
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0

Patch Review

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

```c
	} else if (mem_flags & EFI_MEMORY_WT) {
		sys_base =3D devm_memremap(&pdev->dev, mem->start, resource_size(mem),
					 MEMREMAP_WT);
		is_iomem =3D 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 p=
ointers), 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 ca=
ses (lines 264 and 270 in the patched file). The compiler may produce a war=
ning about this with strict settings.

**Error handling improvement in the `else` branch =E2=80=94 good:**

The original code would print an error for invalid `mem_flags` but then fal=
l through to `if (!screen_base) return ERR_PTR(-ENOMEM)`, which would retur=
n the wrong error code (`-ENOMEM` instead of something more appropriate). T=
he 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 =E2=80=94 good:**

The separation into `screen_base` (for ioremap paths) and `sys_base` (for m=
emremap paths) with the corresponding `iosys_map_set_vaddr_iomem()` vs `ios=
ys_map_set_vaddr()` is the right approach. The `iosys_map` abstraction spec=
ifically tracks whether the mapped region is I/O memory or system memory, a=
nd 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 t=
he type-mismatched error returns.

---
Generated by Claude Code Patch Reviewer