From mboxrd@z Thu Jan 1 00:00:00 1970 From: Claude Code Review Bot To: dri-devel-reviews@example.com Subject: Claude review: video/fbdev/via: check ioremap return value in viafb_lcd_get_mobile_state Date: Wed, 11 Mar 2026 13:49:46 +1000 Message-ID: In-Reply-To: References: X-Mailer: Claude Code Patch Reviewer Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Patch Review **The bug is real.** At `lcd.c:956`, `ioremap()` is called and the result i= s used immediately at line 957 via `readw(biosptr)` without any NULL check.= If `ioremap()` fails, this would cause a NULL pointer dereference. **Issues:** 1. **Commit message says `-ENOMEM` but the code returns `false`.** The comm= it message states: > This patch adds a proper NULL check after ioremap() and returns -ENOME= M in case of failure. =20 But the actual code returns `false`, which is consistent with the functi= on's `bool` return type. The commit message is wrong and should be correcte= d to say it returns `false`. 2. **DEBUG_MSG format string has a leading space:** ```c DEBUG_MSG(KERN_ERR " Failed to remap BIOS memory\n"); ``` The space after `KERN_ERR` before `"Failed"` looks like a typo. It shoul= d be: ```c DEBUG_MSG(KERN_ERR "Failed to remap BIOS memory\n"); ``` 3. **Use of `DEBUG_MSG` with `KERN_ERR`:** Looking at existing error paths = in this function, neither of the existing `return false` paths (line 988) l= ogs anything. Using `DEBUG_MSG` (which is typically compiled out) with `KER= N_ERR` is a bit contradictory =E2=80=94 if this is truly an error worth rep= orting, `pr_err()` would be more appropriate and wouldn't be compiled away.= That said, using the existing codebase conventions is also reasonable; the= author should clarify intent. 4. **Minor: the `*mobile` output parameter is left uninitialized on the err= or path.** This matches the existing behavior when the BIOS signature doesn= 't match (the `else` branch at line 986-988 also returns `false` without se= tting `*mobile`), so this is a pre-existing issue, not something this patch= introduces. **The fix itself is correct in substance** =E2=80=94 the NULL check is plac= ed in the right location, the return value is appropriate for the function = signature, and the existing `iounmap()` calls in later paths don't need adj= ustment since we never successfully mapped in this error case. --- Generated by Claude Code Patch Reviewer