Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Tue, 28 Jan 2020 11:23:32 -0500 (EST)
From:   Nicolas Pitre <nico@fluxnic.net>
To:     Lukas Wunner <lukas@wunner.de>
cc:     Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        Jiri Slaby <jslaby@suse.com>,
        Mikulas Patocka <mpatocka@redhat.com>,
        Matthew Whitehead <tedheadster@gmail.com>,
        Daniel Vetter <daniel.vetter@ffwll.ch>,
        Arvind Sankar <nivedita@alum.mit.edu>,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH] vt: Fix non-blinking cursor regression
In-Reply-To: <20200128111107.ppqeebsctffmk7n6@wunner.de>
Message-ID: <nycvar.YSQ.7.76.2001281121340.1655@knanqh.ubzr>
References: <575cb82102aa59a7a8e34248821b78e1dd844777.1579701673.git.lukas@wunner.de> <nycvar.YSQ.7.76.2001221032210.1655@knanqh.ubzr> <20200126124834.ea7uxyprz2uaek5x@wunner.de> <nycvar.YSQ.7.76.2001261208030.1655@knanqh.ubzr>
 <20200128111107.ppqeebsctffmk7n6@wunner.de>
User-Agent: Alpine 2.21 (LFD 202 2017-01-01)
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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