linux-kernel - Re: [PATCH next v1 1/2] 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: <87mskkdhwd.fsf@jogness.linutronix.de>
Date: Fri, 06 Sep 2024 15:41:30 +0206
From: John Ogness <john.ogness@...utronix.de>
To: Petr Mladek <pmladek@...e.com>
Cc: 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>, linux-serial@...r.kernel.org,
 linux-kernel@...r.kernel.org, Andy Shevchenko
 <andriy.shevchenko@...ux.intel.com>, Tony Lindgren <tony@...mide.com>,
 "Paul E. McKenney" <paulmck@...nel.org>, Uwe Kleine-König
 <u.kleine-koenig@...gutronix.de>, Ilpo Järvinen
 <ilpo.jarvinen@...ux.intel.com>, Serge Semin <fancer.lancer@...il.com>,
 Rengarajan S <rengarajan.s@...rochip.com>, Wolfram Sang
 <wsa+renesas@...g-engineering.com>
Subject: Re: [PATCH next v1 1/2] serial: 8250: Switch to nbcon console

On 2024-09-06, Petr Mladek <pmladek@...e.com> wrote:
>> +#ifdef USE_SERIAL_8250_LEGACY_CONSOLE
>
> Just for record. I agree that it is better to simply remove the
> obsolete legacy code.

Agreed. I will be removing it for v2.

>> +#ifndef USE_SERIAL_8250_LEGACY_CONSOLE
>> +	if (uart_console(&p->port)) {
>> +		dev_warn(p->port.dev, "no atomic printing for rs485 consoles\n");
>> +		p->port.cons->write_atomic = NULL;
>> +	}
>
> Wait! This makes the rs485 consoles much less usable for debugging.
> They might have troubles to see the emergency and panic messages.
> Or do I miss anything, please?
>
> Is this acceptable? Why?

It is not acceptable. I am looking into making the atomic part work for
RS485 as well. My main problem is testing since I will need to get my
hands or real RS485 hardware.

>>  	wait_for_xmitr(up, UART_LSR_THRE);
>>  	serial_port_out(port, UART_TX, ch);
>> +
>> +	if (ch == '\n')
>> +		up->console_newline_needed = false;
>> +	else
>> +		up->console_newline_needed = true;
>
> I might be just dumb but this code confused me. I missed that the
> variable was actually set after printing the character. I inverted
> the logic in my head and it did not make sense.
>
> I vote for adding a comment. Or better make the code more
> straightforward by renaming the variable and inverting the logic:
>
> 	if (ch == '\n')
> 		up->console_line_ended = true;
> 	else
> 		up->console_line_ended = false;

OK. I will add a comment, rename the variable, and invert the logic.

>> +void serial8250_console_write_thread(struct uart_8250_port *up,
>> +				     struct nbcon_write_context *wctxt)
>> +{
>> +	struct uart_8250_em485 *em485 = up->em485;
>> +	struct uart_port *port = &up->port;
>> +	unsigned int ier;
>> +
>> +	touch_nmi_watchdog();
>
> This should not be needed in the write_thread() variant because
> it allows to schedule after emitting one record.

Agreed.

Thanks.

John