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: Fix efidrm error handling and memory type mismatch
Date: Thu, 12 Mar 2026 07:16:10 +1000
Message-ID: <review-patch1-20260311064652.2903449-1-nichen@iscas.ac.cn>
In-Reply-To: <20260311064652.2903449-1-nichen@iscas.ac.cn>
References: <20260311064652.2903449-1-nichen@iscas.ac.cn> <20260311064652.2903449-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

**Correctness of the fix:**

The restructuring is sound. Each branch now:
- Declares `screen_base` with the correct type (`void __iomem *` for WC/UC,=
 `void *` for WT/WB)
- Uses the correct error check (`!screen_base` for `devm_ioremap*()` which =
returns NULL on failure, `IS_ERR()` for `devm_memremap()` which returns ERR=
_PTR)
- Calls the correct `iosys_map` setter (`iosys_map_set_vaddr_iomem` for iom=
em, `iosys_map_set_vaddr` for system memory)

**Minor issues:**

1. **Error code change for WC/UC paths**: The original code returned `-ENOM=
EM` when `devm_ioremap*()` failed. The patch changes this to `-ENXIO`:
```c
+		if (!screen_base)
+			return ERR_PTR(-ENXIO);
```
This is arguably fine (`-ENXIO` =3D "no such device or address" is reasonab=
le for a failed ioremap), but the commit message doesn't mention or justify=
 this change. `-ENOMEM` was also defensible since `ioremap` failures are ty=
pically due to resource exhaustion. This is a minor nit =E2=80=94 either er=
ror code is acceptable.

2. **Missing `else` fall-through error for invalid `mem_flags`**: In the or=
iginal code, when `mem_flags` matched none of the branches, it printed an e=
rror but then fell through to `if (!screen_base)` which returned `-ENOMEM`.=
 The patch now explicitly returns `-EINVAL` for this case:
```c
+	} else {
+ 		drm_err(dev, "invalid mem_flags: 0x%llx\n", mem_flags);
+		return ERR_PTR(-EINVAL);
+	}
```
This is a good improvement =E2=80=94 the error code is now more meaningful =
and the flow is explicit.

3. **Code duplication**: The WC and UC branches are nearly identical, as ar=
e the WT and WB branches. This could be slightly reduced, but given the sma=
ll number of lines and clarity of the current structure, it's acceptable.

**Commit message quality**: The commit message clearly explains both bugs a=
nd the fix approach. The `Fixes:` tag is present and correct.

**Overall**: This is a correct and well-structured fix for real bugs. The p=
atch looks good to merge. The only suggestion would be to briefly note in t=
he commit message why `-ENXIO` was chosen over `-ENOMEM` for the ioremap fa=
ilure paths, or simply keep `-ENOMEM` to minimize the behavioral change.

---
Generated by Claude Code Patch Reviewer