* [PATCH] vt: Fix non-blinking cursor regression @ 2020-01-22 14:17 Lukas Wunner 2020-01-22 16:40 ` Nicolas Pitre 0 siblings, 1 reply; 6+ messages in thread From: Lukas Wunner @ 2020-01-22 14:17 UTC (permalink / raw) To: Greg Kroah-Hartman, Jiri Slaby, Nicolas Pitre Cc: Mikulas Patocka, Matthew Whitehead, Daniel Vetter, Arvind Sankar, linux-kernel Since commit a6dbe4427559 ("vt: perform safe console erase in the right order"), when userspace clears both the scrollback buffer and the screen by writing "\e[3J" to an fbdev virtual console, the cursor stops blinking if that virtual console is not in the foreground. I'm witnessing this on every boot of Raspbian since updating to v4.19.37+ because agetty writes the sequence to /dev/tty6 while the console is still switched to /dev/tty1. Switching consoles once makes the cursor blink again. The commit added an invocation of ->con_switch() to flush_scrollback(). Normally this is only invoked from switch_screen() to switch consoles. switch_screen() updates *vc->vc_display_fg to the new console and fbcon_switch() updates ops->currcon. Because the commit only invokes fbcon_switch() but doesn't update *vc->vc_display_fg, it performs an incomplete console switch. When fb_flashcursor() subsequently blinks the cursor, it retrieves the foreground console from ops->currcon. Because *vc->vc_display_fg wasn't updated, con_is_visible() incorrectly returns false and as a result, fb_flashcursor() bails out without blinking the cursor. The invocation of ->con_switch() appears to have been erroneous. After all, why should a console switch be performed when clearing the screen? The commit message doesn't provide a rationale either. So delete it. Fixes: a6dbe4427559 ("vt: perform safe console erase in the right order") Signed-off-by: Lukas Wunner <lukas@wunner.de> Cc: stable@vger.kernel.org # v5.1+ Cc: Nicolas Pitre <nico@fluxnic.net> Cc: Mikulas Patocka <mpatocka@redhat.com> Cc: Matthew Whitehead <tedheadster@gmail.com> --- drivers/tty/vt/vt.c | 2 -- 1 file changed, 2 deletions(-) diff --git a/drivers/tty/vt/vt.c b/drivers/tty/vt/vt.c index 35d21cdb60d0..eb25fb4ca05f 100644 --- a/drivers/tty/vt/vt.c +++ b/drivers/tty/vt/vt.c @@ -938,8 +938,6 @@ static void flush_scrollback(struct vc_data *vc) set_origin(vc); if (vc->vc_sw->con_flush_scrollback) vc->vc_sw->con_flush_scrollback(vc); - else - vc->vc_sw->con_switch(vc); } /* -- 2.24.0 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] vt: Fix non-blinking cursor regression 2020-01-22 14:17 [PATCH] vt: Fix non-blinking cursor regression Lukas Wunner @ 2020-01-22 16:40 ` Nicolas Pitre 2020-01-26 12:48 ` Lukas Wunner 0 siblings, 1 reply; 6+ messages in thread From: Nicolas Pitre @ 2020-01-22 16:40 UTC (permalink / raw) To: Lukas Wunner Cc: Greg Kroah-Hartman, Jiri Slaby, Mikulas Patocka, Matthew Whitehead, Daniel Vetter, Arvind Sankar, linux-kernel On Wed, 22 Jan 2020, Lukas Wunner wrote: > Since commit a6dbe4427559 ("vt: perform safe console erase in the right > order"), when userspace clears both the scrollback buffer and the screen > by writing "\e[3J" to an fbdev virtual console, the cursor stops blinking > if that virtual console is not in the foreground. I'm witnessing this > on every boot of Raspbian since updating to v4.19.37+ because agetty > writes the sequence to /dev/tty6 while the console is still switched to > /dev/tty1. Switching consoles once makes the cursor blink again. > > The commit added an invocation of ->con_switch() to flush_scrollback(). > Normally this is only invoked from switch_screen() to switch consoles. > switch_screen() updates *vc->vc_display_fg to the new console and > fbcon_switch() updates ops->currcon. Because the commit only invokes > fbcon_switch() but doesn't update *vc->vc_display_fg, it performs an > incomplete console switch. > > When fb_flashcursor() subsequently blinks the cursor, it retrieves the > foreground console from ops->currcon. Because *vc->vc_display_fg wasn't > updated, con_is_visible() incorrectly returns false and as a result, > fb_flashcursor() bails out without blinking the cursor. > > The invocation of ->con_switch() appears to have been erroneous. After > all, why should a console switch be performed when clearing the screen? > The commit message doesn't provide a rationale either. So delete it. The problem here is that only vgacon provides a con_flush_scrollback method. When not provided, the only way to flush the scrollback buffer is to invoke the switch method. If you remove it the scrollback buffer of the foreground console won't be flushed in the fb case and possibly others. Originally the con_switch method was invoked via update_screen(). The code rationalization in commit a6dbe4427559 failed to carry over a few details though. So I think the actual fix should instead be like this: diff --git a/drivers/tty/vt/vt.c b/drivers/tty/vt/vt.c index 34aa39d1ae..93314a2f26 100644 --- a/drivers/tty/vt/vt.c +++ b/drivers/tty/vt/vt.c @@ -936,10 +936,13 @@ static void flush_scrollback(struct vc_data *vc) WARN_CONSOLE_UNLOCKED(); set_origin(vc); - if (vc->vc_sw->con_flush_scrollback) + if (vc->vc_sw->con_flush_scrollback) { vc->vc_sw->con_flush_scrollback(vc); - else + } else if (con_is_visible(vc)) { + hide_cursor(vc); vc->vc_sw->con_switch(vc); + set_cursor(vc); + } } /* Nicolas ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] vt: Fix non-blinking cursor regression 2020-01-22 16:40 ` Nicolas Pitre @ 2020-01-26 12:48 ` Lukas Wunner 2020-01-26 17:32 ` Nicolas Pitre 0 siblings, 1 reply; 6+ messages in thread From: Lukas Wunner @ 2020-01-26 12:48 UTC (permalink / raw) To: Nicolas Pitre Cc: Greg Kroah-Hartman, Jiri Slaby, Mikulas Patocka, Matthew Whitehead, Daniel Vetter, Arvind Sankar, linux-kernel On Wed, Jan 22, 2020 at 11:40:38AM -0500, Nicolas Pitre wrote: > On Wed, 22 Jan 2020, Lukas Wunner wrote: > > Since commit a6dbe4427559 ("vt: perform safe console erase in the right > > order"), when userspace clears both the scrollback buffer and the screen > > by writing "\e[3J" to an fbdev virtual console, the cursor stops blinking > > if that virtual console is not in the foreground. I'm witnessing this > > on every boot of Raspbian since updating to v4.19.37+ because agetty > > writes the sequence to /dev/tty6 while the console is still switched to > > /dev/tty1. Switching consoles once makes the cursor blink again. > > > > The commit added an invocation of ->con_switch() to flush_scrollback(). > > Normally this is only invoked from switch_screen() to switch consoles. > > switch_screen() updates *vc->vc_display_fg to the new console and > > fbcon_switch() updates ops->currcon. Because the commit only invokes > > fbcon_switch() but doesn't update *vc->vc_display_fg, it performs an > > incomplete console switch. > > > > When fb_flashcursor() subsequently blinks the cursor, it retrieves the > > foreground console from ops->currcon. Because *vc->vc_display_fg wasn't > > updated, con_is_visible() incorrectly returns false and as a result, > > fb_flashcursor() bails out without blinking the cursor. > > > > The invocation of ->con_switch() appears to have been erroneous. After > > all, why should a console switch be performed when clearing the screen? > > The commit message doesn't provide a rationale either. So delete it. > > The problem here is that only vgacon provides a con_flush_scrollback > method. When not provided, the only way to flush the scrollback buffer > is to invoke the switch method. If you remove it the scrollback buffer > of the foreground console won't be flushed in the fb case and possibly > others. Okay. I guess it's somewhat counter-intuitive that ->con_switch() is called only because it has the side effect of flushing scrollback. In particular, this approach doesn't work for nonvisible consoles. So the proper solution might be to amend the fb_con struct with a ->con_flush_scrollback() hook. Which portions of fbcon_switch() would have to be duplicated in that hook? The softback code at the top of the function would seem like an obvious candidate. What about the invocation of fb_set_var() (which in turn calls fb_pan_display())? Anything else? FWIW, even without any call to ->con_switch, writing "\e[3J" does flush scrollback. Works both on the foreground console as well as nonvisible consoles. I've tested this with bcm2708_fb which isn't upstream yet, it's in the Raspberry Pi Foundation's downstream tree: https://github.com/raspberrypi/linux/blob/rpi-4.19.y/drivers/video/fbdev/bcm2708_fb.c > @@ -936,10 +936,13 @@ static void flush_scrollback(struct vc_data *vc) > WARN_CONSOLE_UNLOCKED(); > > set_origin(vc); > - if (vc->vc_sw->con_flush_scrollback) > + if (vc->vc_sw->con_flush_scrollback) { > vc->vc_sw->con_flush_scrollback(vc); > - else > + } else if (con_is_visible(vc)) { > + hide_cursor(vc); > vc->vc_sw->con_switch(vc); > + set_cursor(vc); > + } A dumb question perhaps, but why is it necessary to hide the cursor? Thanks, Lukas ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] vt: Fix non-blinking cursor regression 2020-01-26 12:48 ` Lukas Wunner @ 2020-01-26 17:32 ` Nicolas Pitre 2020-01-28 11:11 ` Lukas Wunner 0 siblings, 1 reply; 6+ messages in thread From: Nicolas Pitre @ 2020-01-26 17:32 UTC (permalink / raw) To: Lukas Wunner Cc: Greg Kroah-Hartman, Jiri Slaby, Mikulas Patocka, Matthew Whitehead, Daniel Vetter, Arvind Sankar, linux-kernel On Sun, 26 Jan 2020, Lukas Wunner wrote: > On Wed, Jan 22, 2020 at 11:40:38AM -0500, Nicolas Pitre wrote: > > On Wed, 22 Jan 2020, Lukas Wunner wrote: > > > Since commit a6dbe4427559 ("vt: perform safe console erase in the right > > > order"), when userspace clears both the scrollback buffer and the screen > > > by writing "\e[3J" to an fbdev virtual console, the cursor stops blinking > > > if that virtual console is not in the foreground. I'm witnessing this > > > on every boot of Raspbian since updating to v4.19.37+ because agetty > > > writes the sequence to /dev/tty6 while the console is still switched to > > > /dev/tty1. Switching consoles once makes the cursor blink again. > > > > > > The commit added an invocation of ->con_switch() to flush_scrollback(). > > > Normally this is only invoked from switch_screen() to switch consoles. > > > switch_screen() updates *vc->vc_display_fg to the new console and > > > fbcon_switch() updates ops->currcon. Because the commit only invokes > > > fbcon_switch() but doesn't update *vc->vc_display_fg, it performs an > > > incomplete console switch. > > > > > > When fb_flashcursor() subsequently blinks the cursor, it retrieves the > > > foreground console from ops->currcon. Because *vc->vc_display_fg wasn't > > > updated, con_is_visible() incorrectly returns false and as a result, > > > fb_flashcursor() bails out without blinking the cursor. > > > > > > The invocation of ->con_switch() appears to have been erroneous. After > > > all, why should a console switch be performed when clearing the screen? > > > The commit message doesn't provide a rationale either. So delete it. > > > > The problem here is that only vgacon provides a con_flush_scrollback > > method. When not provided, the only way to flush the scrollback buffer > > is to invoke the switch method. If you remove it the scrollback buffer > > of the foreground console won't be flushed in the fb case and possibly > > others. > > Okay. I guess it's somewhat counter-intuitive that ->con_switch() > is called only because it has the side effect of flushing scrollback. > In particular, this approach doesn't work for nonvisible consoles. Normally, only the foreground console has a scrollback, so by not being visible it therefore has no scrollback to flush. When the scrollback is persistent (only vgacon implements that) then there must be a con_flush_scrollback method. > So the proper solution might be to amend the fb_con struct with a > ->con_flush_scrollback() hook. Yes, and every other console drivers as well. > Which portions of fbcon_switch() > would have to be duplicated in that hook? The softback code at the > top of the function would seem like an obvious candidate. Most likely, yes, after making sure the scrollback does correspond to the vc argument. > What about the invocation of fb_set_var() (which in turn calls > fb_pan_display())? Anything else? I don't know enough about the fbcon code to be sure of all the implications here. Still, I'd prefer to get back to the same functional state from before commit a6dbe44275 with the switch method first. Can you confirm that the patch I propose does fix it for you? > > @@ -936,10 +936,13 @@ static void flush_scrollback(struct vc_data *vc) > > WARN_CONSOLE_UNLOCKED(); > > > > set_origin(vc); > > - if (vc->vc_sw->con_flush_scrollback) > > + if (vc->vc_sw->con_flush_scrollback) { > > vc->vc_sw->con_flush_scrollback(vc); > > - else > > + } else if (con_is_visible(vc)) { > > + hide_cursor(vc); > > vc->vc_sw->con_switch(vc); > > + set_cursor(vc); > > + } > > A dumb question perhaps, but why is it necessary to hide the cursor? Many console implements the cursor by changing the background color of the cursor position. If the switch occurs while the cursor is in its visible period, the rest of the code will assume that the cursor is the actual background color, effectively leaving the drawn cursor there after it moved. Nicolas ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] vt: Fix non-blinking cursor regression 2020-01-26 17:32 ` Nicolas Pitre @ 2020-01-28 11:11 ` Lukas Wunner 2020-01-28 16:23 ` Nicolas Pitre 0 siblings, 1 reply; 6+ messages in thread From: Lukas Wunner @ 2020-01-28 11:11 UTC (permalink / raw) To: Nicolas Pitre Cc: Greg Kroah-Hartman, Jiri Slaby, Mikulas Patocka, Matthew Whitehead, Daniel Vetter, Arvind Sankar, linux-kernel On Sun, Jan 26, 2020 at 12:32:10PM -0500, Nicolas Pitre wrote: > On Sun, 26 Jan 2020, Lukas Wunner wrote: > > On Wed, Jan 22, 2020 at 11:40:38AM -0500, Nicolas Pitre wrote: > > > On Wed, 22 Jan 2020, Lukas Wunner wrote: > > > > Since commit a6dbe4427559 ("vt: perform safe console erase in the right > > > > order"), when userspace clears both the scrollback buffer and the screen > > > > by writing "\e[3J" to an fbdev virtual console, the cursor stops blinking > > > > if that virtual console is not in the foreground. I'm witnessing this > > > > on every boot of Raspbian since updating to v4.19.37+ because agetty > > > > writes the sequence to /dev/tty6 while the console is still switched to > > > > /dev/tty1. Switching consoles once makes the cursor blink again. > > > > > > > > The commit added an invocation of ->con_switch() to flush_scrollback(). > > > > Normally this is only invoked from switch_screen() to switch consoles. > > > > switch_screen() updates *vc->vc_display_fg to the new console and > > > > fbcon_switch() updates ops->currcon. Because the commit only invokes > > > > fbcon_switch() but doesn't update *vc->vc_display_fg, it performs an > > > > incomplete console switch. > > > > > > > > When fb_flashcursor() subsequently blinks the cursor, it retrieves the > > > > foreground console from ops->currcon. Because *vc->vc_display_fg wasn't > > > > updated, con_is_visible() incorrectly returns false and as a result, > > > > fb_flashcursor() bails out without blinking the cursor. > > > > > > > > The invocation of ->con_switch() appears to have been erroneous. After > > > > all, why should a console switch be performed when clearing the screen? > > > > The commit message doesn't provide a rationale either. So delete it. > > > > > > The problem here is that only vgacon provides a con_flush_scrollback > > > method. When not provided, the only way to flush the scrollback buffer > > > is to invoke the switch method. If you remove it the scrollback buffer > > > of the foreground console won't be flushed in the fb case and possibly > > > others. [...] > Still, I'd prefer to get back to the same functional state from before > commit a6dbe44275 with the switch method first. Can you confirm that the > patch I propose does fix it for you? Yes, your patch is Reported-and-tested-by: Lukas Wunner <lukas@wunner.de> I'm withdrawing my own patch because further testing has shown that while it fixes the non-blinking cursor issue, it doesn't flush scrollback if "\e[3J" is written to the foreground console. Would you prefer me to submit your patch with your Signed-off-by or rather submit it yourself with my Tested-by? If the latter, please include a code comment explaining that ->con_switch() has the side effect of flushing scrollback. That seems quite non-obvious to me. > > A dumb question perhaps, but why is it necessary to hide the cursor? > > Many console implements the cursor by changing the background color of > the cursor position. If the switch occurs while the cursor is in its > visible period, the rest of the code will assume that the cursor is the > actual background color, effectively leaving the drawn cursor there > after it moved. I see, thanks for the explanation. Lukas ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] vt: Fix non-blinking cursor regression 2020-01-28 11:11 ` Lukas Wunner @ 2020-01-28 16:23 ` Nicolas Pitre 0 siblings, 0 replies; 6+ messages in thread From: Nicolas Pitre @ 2020-01-28 16:23 UTC (permalink / raw) To: Lukas Wunner Cc: Greg Kroah-Hartman, Jiri Slaby, Mikulas Patocka, Matthew Whitehead, Daniel Vetter, Arvind Sankar, linux-kernel On Tue, 28 Jan 2020, Lukas Wunner wrote: > On Sun, Jan 26, 2020 at 12:32:10PM -0500, Nicolas Pitre wrote: > > On Sun, 26 Jan 2020, Lukas Wunner wrote: > > > On Wed, Jan 22, 2020 at 11:40:38AM -0500, Nicolas Pitre wrote: > > > > On Wed, 22 Jan 2020, Lukas Wunner wrote: > > > > > Since commit a6dbe4427559 ("vt: perform safe console erase in the right > > > > > order"), when userspace clears both the scrollback buffer and the screen > > > > > by writing "\e[3J" to an fbdev virtual console, the cursor stops blinking > > > > > if that virtual console is not in the foreground. I'm witnessing this > > > > > on every boot of Raspbian since updating to v4.19.37+ because agetty > > > > > writes the sequence to /dev/tty6 while the console is still switched to > > > > > /dev/tty1. Switching consoles once makes the cursor blink again. > > > > > > > > > > The commit added an invocation of ->con_switch() to flush_scrollback(). > > > > > Normally this is only invoked from switch_screen() to switch consoles. > > > > > switch_screen() updates *vc->vc_display_fg to the new console and > > > > > fbcon_switch() updates ops->currcon. Because the commit only invokes > > > > > fbcon_switch() but doesn't update *vc->vc_display_fg, it performs an > > > > > incomplete console switch. > > > > > > > > > > When fb_flashcursor() subsequently blinks the cursor, it retrieves the > > > > > foreground console from ops->currcon. Because *vc->vc_display_fg wasn't > > > > > updated, con_is_visible() incorrectly returns false and as a result, > > > > > fb_flashcursor() bails out without blinking the cursor. > > > > > > > > > > The invocation of ->con_switch() appears to have been erroneous. After > > > > > all, why should a console switch be performed when clearing the screen? > > > > > The commit message doesn't provide a rationale either. So delete it. > > > > > > > > The problem here is that only vgacon provides a con_flush_scrollback > > > > method. When not provided, the only way to flush the scrollback buffer > > > > is to invoke the switch method. If you remove it the scrollback buffer > > > > of the foreground console won't be flushed in the fb case and possibly > > > > others. > [...] > > Still, I'd prefer to get back to the same functional state from before > > commit a6dbe44275 with the switch method first. Can you confirm that the > > patch I propose does fix it for you? > > Yes, your patch is > > Reported-and-tested-by: Lukas Wunner <lukas@wunner.de> OK, thanks. > I'm withdrawing my own patch because further testing has shown that while it > fixes the non-blinking cursor issue, it doesn't flush scrollback if "\e[3J" > is written to the foreground console. > > Would you prefer me to submit your patch with your Signed-off-by or rather > submit it yourself with my Tested-by? If the latter, please include a code > comment explaining that ->con_switch() has the side effect of flushing > scrollback. That seems quite non-obvious to me. I'll submit it with a comment stating the non-obvious. Nicolas ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2020-01-28 16:23 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-01-22 14:17 [PATCH] vt: Fix non-blinking cursor regression Lukas Wunner 2020-01-22 16:40 ` Nicolas Pitre 2020-01-26 12:48 ` Lukas Wunner 2020-01-26 17:32 ` Nicolas Pitre 2020-01-28 11:11 ` Lukas Wunner 2020-01-28 16:23 ` Nicolas Pitre
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).