linux-kernel - Re: [PATCH next v2 3/4] serial: 8250: Switch to nbcon console

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <ZurlGqIdCbeSjYDj@pathway.suse.cz>
Date: Wed, 18 Sep 2024 16:35:06 +0200
From: Petr Mladek <pmladek@...e.com>
To: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
Cc: John Ogness <john.ogness@...utronix.de>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	Jiri Slaby <jirislaby@...nel.org>,
	Sergey Senozhatsky <senozhatsky@...omium.org>,
	Steven Rostedt <rostedt@...dmis.org>,
	Thomas Gleixner <tglx@...utronix.de>,
	Esben Haabendal <esben@...nix.com>, linux-serial@...r.kernel.org,
	linux-kernel@...r.kernel.org, Tony Lindgren <tony@...mide.com>,
	Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>,
	Uwe Kleine-König <u.kleine-koenig@...gutronix.de>,
	Arnd Bergmann <arnd@...db.de>,
	Florian Fainelli <florian.fainelli@...adcom.com>,
	Serge Semin <fancer.lancer@...il.com>,
	Wolfram Sang <wsa+renesas@...g-engineering.com>,
	Lino Sanfilippo <l.sanfilippo@...bus.com>
Subject: Re: [PATCH next v2 3/4] serial: 8250: Switch to nbcon console

On Wed 2024-09-18 17:01:27, Andy Shevchenko wrote:
> On Wed, Sep 18, 2024 at 02:26:25PM +0200, Petr Mladek wrote:
> > On Fri 2024-09-13 16:11:37, John Ogness wrote:
> > > Implement the necessary callbacks to switch the 8250 console driver
> > > to perform as an nbcon console.
> > > 
> > > Add implementations for the nbcon console callbacks (write_atomic,
> > > write_thread, device_lock, device_unlock) and add CON_NBCON to the
> > > initial flags.
> > > 
> > > All register access in the callbacks are within unsafe sections.
> > > The write_thread() callback allows safe handover/takeover per byte.
> > > The write_atomic() callback allows safe handover/takeover per
> > > printk record and adds a preceding newline if it took over mid-line.
> > > 
> > > For the write_atomic() case, a new irq_work is used to defer modem
> > > control since it may be a context that does not allow waking up
> > > tasks.
> > 
> > It would be fair to mention that it does not longer support fifo in
> > the 8250 driver. It basically reverted the commit 8f3631f0f6eb42e5
> > ("serial/8250: Use fifo in 8250 console driver").
> > 
> > It is not usable in write_thread() because it would not allow
> > a safe takeover between emitting particular characters.
> > 
> > It might still be used in write_atomic() but it is probably not
> > worth it. This callback is used "only" in emergency and panic
> > situations.
> 
> This is unfortunate. It will drop down the efficiency of printing.

The FIFO mode has been added by the commit 8f3631f0f6eb42e5
("serial/8250: Use fifo in 8250 console driver"). The interesting
parts are:

<paste>
  While investigating a bug in the RHEL kernel, I noticed that the serial
  console throughput is way below the configured speed of 115200 bps in
  a HP Proliant DL380 Gen9. I was expecting something above 10KB/s, but
  I got 2.5KB/s.

  In another machine, I measured a throughput of 11.5KB/s, with the serial
  controller taking between 80-90us to send each byte. That matches the
  expected throughput for a configuration of 115200 bps.

  This patch changes the serial8250_console_write to use the 16550 fifo
  if available. In my benchmarks I got around 25% improvement in the slow
  machine, and no performance penalty in the fast machine.
</paste>

I would translate it:

The FIFO mode helped with some buggy serial console. But it helped to gain
only small portion of the expected speed. The commit message does not
mention any gain with the normally working system.

It has been added in 2022. It was considered only because of a
"broken" system. Nobody cared enough before.

> I think it should be done differently, i.e. the takeover the code
> has to drop FIFO (IIRC it's easy to achieve by disabling it or so)
> and switch to printing the panic/emergency message. But still at
> some baud rate speeds draining the FIFO to the other end may be
> not a bad idea as it takes a few dozens of microseconds.

Sure. it is doable. But I am not convinced that it is really worth it.

Best Regards,
Petr