From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S965220AbcKJVrz (ORCPT ); Thu, 10 Nov 2016 16:47:55 -0500 Received: from out4-smtp.messagingengine.com ([66.111.4.28]:60753 "EHLO out4-smtp.messagingengine.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S964856AbcKJVrx (ORCPT ); Thu, 10 Nov 2016 16:47:53 -0500 X-ME-Sender: X-Sasl-enc: wvLR/f6Px5icvKA3T/KgtyWOIKMdBkGqMAKT3msHH60O 1478814471 Date: Thu, 10 Nov 2016 21:47:48 +0000 From: Andrey Utkin To: Manuel =?iso-8859-1?Q?Sch=F6lling?= Cc: plagnioj@jcrosoft.com, tomi.valkeinen@ti.com, jslaby@suse.cz, gregkh@linuxfoundation.org, linux-fbdev@vger.kernel.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH v3] console: Add persistent scrollback buffers for all VGA consoles Message-ID: <20161110214748.GB26324@dell-m4800.home> References: <1474236417-27150-1-git-send-email-manuel.schoelling@gmx.de> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <1474236417-27150-1-git-send-email-manuel.schoelling@gmx.de> User-Agent: Mutt/1.7.1 (2016-10-04) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Mon, Sep 19, 2016 at 12:06:57AM +0200, Manuel Schölling wrote: > Add a scrollback buffers for each VGA console. The benefit is that > the scrollback history is not flushed when switching between consoles > but is persistent. > The buffers are allocated on demand when a new console is opened. Hi, I have noticed your message on #kernelnewbies IRC channel. Thanks for the patch. This is definitely very useful, especially when you boot into rescue system and use raw console and not a fancy X11-based terminal app. > > It always annoys me when I switch back to a console and I can just > read half of an error backtrace. This should fix issues like these. Personal feelings don't belong to commit messages. Previous paragraph is enough to express the purpose. > > This breaks tools like clear_console that rely on flushing the > scrollback history by switching back and forth between consoles > which is why this feature is disabled by default. > Use the escape sequence \e[3J instead for flushing the buffer. Have never heard of such tool as clear_console. Why mention it at all, where is it used, which are practical configurations which are broken in this way? Does this break clear(1)? > > Signed-off-by: Manuel Schölling > --- > Changes in v3: > - Add config option for this feature > - Fallback to old scrollback buffer if kcalloc() fails > - Remove ioctl() call again and add documentation about existing > escape sequence to flush the scrollback buffer > Changes in v2: > - Add ioctl() call to flush scrollback buffer > - (Patch v2 was not labeled as such, sorry) > --- > drivers/video/console/Kconfig | 23 +++++- > drivers/video/console/vgacon.c | 160 +++++++++++++++++++++++++++-------------- > 2 files changed, 128 insertions(+), 55 deletions(-) > > diff --git a/drivers/video/console/Kconfig b/drivers/video/console/Kconfig > index 38da6e2..67e52f0 100644 > --- a/drivers/video/console/Kconfig > +++ b/drivers/video/console/Kconfig > @@ -43,9 +43,26 @@ config VGACON_SOFT_SCROLLBACK_SIZE > range 1 1024 > default "64" > help > - Enter the amount of System RAM to allocate for the scrollback > - buffer. Each 64KB will give you approximately 16 80x25 > - screenfuls of scrollback buffer > + Enter the amount of System RAM to allocate for scrollback > + buffers of VGA consoles. Each 64KB will give you approximately > + 16 80x25 screenfuls of scrollback buffer. Indentation amended, I don't know which way it should be, please double-check or mimic old indentation to stay on safe side. > + > +config VGACON_SOFT_SCROLLBACK_FOR_EACH_CONSOLE Feels like bad naming. What about VGACON_SOFT_SCROLLBACK_PERSISTENT? > + bool "Persistent Scrollback History for each console" > + depends on VGACON_SOFT_SCROLLBACK > + default n > + help > + Say Y here if for each VGA console a scrollback buffer should > + be allocated. The scrollback history will persist when switching Unnatural order of sentence IMHO, however I'm not native English. > + between consoles. If you say N here, scrollback is only supported > + for the active VGA console and scrollback history will be flushed > + when switching between consoles. I think you may put less text here. You can skip details like "buffer ... allocated", just explain the difference in behaviour. > + > + This breaks legacy versions of tools like clear_console which > + might cause security issues. > + Use the escape sequence \e[3J instead if this feature is activated. See above. I'd drop this paragraph if clear(1) is not broken. > + > + If you use a RAM-constrained system, say N here. Which exactly cases to consider? I can't think of any practical one. You may want to help user to figure out if he should consider this by explaining that VGACON_SOFT_SCROLLBACK_SIZE is taken for each created tty device (right?). Or drop this line :) > > config MDA_CONSOLE > depends on !M68K && !PARISC && ISA > diff --git a/drivers/video/console/vgacon.c b/drivers/video/console/vgacon.c > index 1157661..e70e8fe 100644 > --- a/drivers/video/console/vgacon.c > +++ b/drivers/video/console/vgacon.c > @@ -1,5 +1,5 @@ > /* > - * linux/drivers/video/vgacon.c -- Low level VGA based console driver > + * linux/drivers/video/console/vgacon.c -- Low level VGA based console driver Cosmetic changes in separate patch please. > * > * Created 28 Sep 1997 by Geert Uytterhoeven > * > @@ -181,70 +181,125 @@ static inline void vga_set_mem_top(struct vc_data *c) > > #ifdef CONFIG_VGACON_SOFT_SCROLLBACK > /* software scrollback */ > -static void *vgacon_scrollback; > -static int vgacon_scrollback_tail; > -static int vgacon_scrollback_size; > -static int vgacon_scrollback_rows; > -static int vgacon_scrollback_cnt; > -static int vgacon_scrollback_cur; > -static int vgacon_scrollback_save; > -static int vgacon_scrollback_restore; > - > -static void vgacon_scrollback_init(int pitch) > +struct vgacon_scrollback_info { > + void *data; > + int tail; > + int size; > + int rows; > + int cnt; > + int cur; > + int save; > + int restore; > +}; Refactor in separate patch please. It would be much nicer if you make first patch in series which would gather all these variables into struct and uses it everywhere, leaving external behaviour the same. Then, in following patches, change the behaviour, one trait at a time. > +static struct vgacon_scrollback_info *vgacon_scrollback_cur; Could you use name vgacon_scrollback instead of new vgacon_scrollback_cur? It doesn't change anything conceptually, but is a bit shorter and seems to mean current scrollback context anyway, right? I think I see nothing to comment on below. Looking forward to see your reply. Would definitely like to test this when I have some time.