[PATCH] fbcon: don't suspend/resume when vc is graphics mode

[PATCH] fbcon: don't suspend/resume when vc is graphics mode

[yaolu]

From: Lu Yao <yaolu@kylinos.cn>

Don't need to do suspend/resume for fbcon in graphic mode.

Doing this may cause error, eg:
  At the beginning, starting the Xorg with single screen and then an
  external screen was plugged in. After logging out in Xorg, fbdev
  info may using 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 found 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.

Signed-off-by: Lu Yao <yaolu@kylinos.cn>
---
 drivers/video/fbdev/core/fbcon.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/video/fbdev/core/fbcon.c b/drivers/video/fbdev/core/fbcon.c
index b0e3e765360d..450e690d0bd2 100644
--- a/drivers/video/fbdev/core/fbcon.c
+++ b/drivers/video/fbdev/core/fbcon.c
@@ -2602,8 +2602,9 @@ void fbcon_suspended(struct fb_info *info)
 		return;
 	vc = vc_cons[par->currcon].d;
 
-	/* Clear cursor, restore saved data */
-	fbcon_cursor(vc, false);
+	/* Don't need to clear cursor and restore saved data in graphic mode */
+	if (vc->vc_mode != KD_GRAPHICS)
+		fbcon_cursor(vc, false);
 }
 
 void fbcon_resumed(struct fb_info *info)
@@ -2615,7 +2616,9 @@ void fbcon_resumed(struct fb_info *info)
 		return;
 	vc = vc_cons[par->currcon].d;
 
-	update_screen(vc);
+	/* Graphics mode is managed by userspace */
+	if (vc->vc_mode != KD_GRAPHICS)
+		update_screen(vc);
 }
 
 static void fbcon_modechanged(struct fb_info *info)
-- 
2.25.1

Claude review: fbcon: don't suspend/resume when vc is graphics mode

[Claude Code Review Bot]

Overall Series Review

Subject: fbcon: don't suspend/resume when vc is graphics mode
Author: yaolu@kylinos.cn
Patches: 1
Reviewed: 2026-05-05T10:55:28.147234

---

This is a single patch that adds `KD_GRAPHICS` mode checks to `fbcon_suspended()` and `fbcon_resumed()`, skipping cursor clearing and screen updating when the VC is in graphics mode. The change is **reasonable in principle** — fbcon should not manipulate the framebuffer when a userspace graphics server (e.g. Xorg) owns the display. This is consistent with the existing pattern throughout fbcon.c, where nearly every display-modifying operation is gated on `KD_TEXT` mode.

However, the patch has issues with commit message quality and there's a question about whether it's fixing the right layer (this may be papering over a deeper problem in the DRM/fbdev handoff on hotplug + suspend).

**Verdict: Needs revision** — the concept is sound, but the commit message needs improvement.

---
Generated by Claude Code Patch Reviewer

Claude review: fbcon: don't suspend/resume when vc is graphics mode

[Claude Code Review Bot]

Patch Review

**Code change — 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 != KD_GRAPHICS)
	fbcon_cursor(vc, false);
```

```c
/* Graphics mode is managed by userspace */
if (vc->vc_mode != KD_GRAPHICS)
	update_screen(vc);
```

This is consistent with the existing pattern in fbcon.c. Looking at the rest of the file, essentially every operation that modifies screen content checks for `KD_TEXT` (or `!= KD_GRAPHICS`) before proceeding — see `fbcon_modechanged()` at line 2634, `fbcon_set_all_vcs()` at line 2674, `fbcon_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_lock`, 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 external screen was plugged in. After logging out in Xorg, fbdev info may using 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 found 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 ordering, `drm_setup_crtcs_fb` connector selection, suspend/resume, Xorg CRTC rebinding). The commit message should clearly describe: (a) the symptom (what goes wrong visually or functionally), (b) the root cause (fbcon_suspended/resumed unconditionally operate on the framebuffer even in KD_GRAPHICS mode), and (c) why the fix is correct (graphics mode means userspace owns the display, and fbcon should not touch it — consistent with all other fbcon code paths).

2. **The described scenario may indicate a deeper issue.** The commit message describes a problem with `drm_setup_crtcs_fb` picking the wrong connector after hotplug, which wouldn't be fixed by this patch — it would only be masked. If that's a real bug, it should be filed/fixed separately. This patch should stand on its own merits: fbcon should not manipulate the screen 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" — 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_GRAPHICS` checks in this file have no accompanying comment.

**Recommendation:** Request a v2 with a rewritten commit message that clearly states: "fbcon_suspended() and fbcon_resumed() unconditionally clear the cursor and update the screen. When the VC is in KD_GRAPHICS mode, userspace owns the display and fbcon should not touch it. Add KD_GRAPHICS guards consistent with other fbcon code paths." The code itself is fine.

---
Generated by Claude Code Patch Reviewer