From mboxrd@z Thu Jan 1 00:00:00 1970 From: Claude Code Review Bot To: dri-devel-reviews@example.com Subject: Claude review: fbcon: don't suspend/resume when vc is graphics mode Date: Tue, 05 May 2026 10:55:28 +1000 Message-ID: In-Reply-To: <20260430060137.131107-1-yaolu@kylinos.cn> References: <20260430060137.131107-1-yaolu@kylinos.cn> <20260430060137.131107-1-yaolu@kylinos.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 **Code change =E2=80=94 looks correct:** The two hunks add appropriate guards: ```c /* Don't need to clear cursor and restore saved data in graphic mode */ if (vc->vc_mode !=3D KD_GRAPHICS) fbcon_cursor(vc, false); ``` ```c /* Graphics mode is managed by userspace */ if (vc->vc_mode !=3D KD_GRAPHICS) update_screen(vc); ``` This is consistent with the existing pattern in fbcon.c. Looking at the res= t of the file, essentially every operation that modifies screen content che= cks for `KD_TEXT` (or `!=3D KD_GRAPHICS`) before proceeding =E2=80=94 see `= fbcon_modechanged()` at line 2634, `fbcon_set_all_vcs()` at line 2674, `fbc= on_registered_fb_set_buf()` at line 3021, `fbcon_init()` at line 1193, etc.= The suspend/resume paths were missing this check, which is arguably a pre-= existing bug. The caller `fb_set_suspend()` in `fbmem.c` (line 652) does hold `console_lo= ck`, so there's no TOCTOU concern with reading `vc->vc_mode`. **Issues:** 1. **Commit message is poorly written and hard to follow.** The description: > "At the beginning, starting the Xorg with single screen and then an ex= ternal screen was plugged in. After logging out in Xorg, fbdev info may usi= ng screen which is connected later on for info always using first connected= connector in list in func 'drm_setup_crtcs_fb'. Then, S3 executed, fbcon f= ound that the information did not match and do atomic to switch fb. However= , Xorg will not re-bind the crtc fb but continues doing ioctl. At this time= , the fb is incorrect." This scenario is confusing and mixes several unrelated issues (hotplug o= rdering, `drm_setup_crtcs_fb` connector selection, suspend/resume, Xorg CRT= C rebinding). The commit message should clearly describe: (a) the symptom (= what goes wrong visually or functionally), (b) the root cause (fbcon_suspen= ded/resumed unconditionally operate on the framebuffer even in KD_GRAPHICS = mode), and (c) why the fix is correct (graphics mode means userspace owns t= he display, and fbcon should not touch it =E2=80=94 consistent with all oth= er fbcon code paths). 2. **The described scenario may indicate a deeper issue.** The commit messa= ge describes a problem with `drm_setup_crtcs_fb` picking the wrong connecto= r after hotplug, which wouldn't be fixed by this patch =E2=80=94 it would o= nly be masked. If that's a real bug, it should be filed/fixed separately. T= his patch should stand on its own merits: fbcon should not manipulate the s= creen during suspend/resume when in graphics mode, period. 3. **Minor style nit:** The first comment says "Don't need to clear cursor = and restore saved data in graphic mode" =E2=80=94 it would be slightly more= consistent to say "in graphics mode" (with the 's'), and honestly neither = comment adds much value since the `KD_GRAPHICS` check is self-explanatory. = Consider dropping the comments entirely, as most other `KD_TEXT`/`KD_GRAPHI= CS` checks in this file have no accompanying comment. **Recommendation:** Request a v2 with a rewritten commit message that clear= ly states: "fbcon_suspended() and fbcon_resumed() unconditionally clear the= cursor and update the screen. When the VC is in KD_GRAPHICS mode, userspac= e owns the display and fbcon should not touch it. Add KD_GRAPHICS guards co= nsistent with other fbcon code paths." The code itself is fine. --- Generated by Claude Code Patch Reviewer