linux-kernel - Re: [PATCH tty-next v4 5/6] 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] [day] [month] [year] [list]

Message-ID: <Z3v_7f3Oc_A3rAnr@pathway.suse.cz>
Date: Mon, 6 Jan 2025 17:08:13 +0100
From: Petr Mladek <pmladek@...e.com>
To: John Ogness <john.ogness@...utronix.de>
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>,
	Esben Haabendal <esben@...nix.com>, linux-serial@...r.kernel.org,
	linux-kernel@...r.kernel.org,
	Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
	Matt Turner <mattst88@...il.com>, Tony Lindgren <tony@...mide.com>,
	Arnd Bergmann <arnd@...db.de>,
	Niklas Schnelle <schnelle@...ux.ibm.com>,
	Serge Semin <fancer.lancer@...il.com>
Subject: Re: [PATCH tty-next v4 5/6] serial: 8250: Switch to nbcon console

On Sun 2025-01-05 02:03:41, John Ogness wrote:
> On 2025-01-03, Petr Mladek <pmladek@...e.com> wrote:
> >> --- a/drivers/tty/serial/8250/8250_port.c
> >> +++ b/drivers/tty/serial/8250/8250_port.c
> >> @@ -1406,9 +1416,6 @@ void serial8250_em485_stop_tx(struct uart_8250_port *p, bool toggle_ier)
> >>  {
> >>  	unsigned char mcr = serial8250_in_MCR(p);
> >>  
> >> -	/* Port locked to synchronize UART_IER access against the console. */
> >> -	lockdep_assert_held_once(&p->port.lock);
> >
> > We should explain why it is OK to move the assert.
> >
> > IMHO, most poeple would not understand the port lock is needed only
> > for UART_IER manipulation and not for UART_MCR manipulation.
> >
> > We should probably explain that even the UART_MCR manipulation
> > is synchronized either by the port lock or by nbcon context
> > ownership. Where the nbcon context owner ship actually provides
> > synchronization against the port lock in all situations
> > except for the final unsafe flush in panic().
> 
> Correct, although the "except for the final unsafe flush in panic()" is
> the reason that even an nbcon context ownership assert could not be used
> here.

Good point. Well, lockdep should be disabled by debug_locks_off()
before nbcon_atomic_flush_unsafe() is called. I hope that we could
keep the assert.

> > A comment might be enough.
> 
> OK. I will extend the comment at the new lockdep_assert site explaining
> why the port lock is only guaranteed to be held for the toggle_ier==true
> situation.

Thanks.

> >> +	bool			console_line_ended;	/* line fully output */
> >
> > I wonder if the following is better:
> >
> > 	bool			console_line_ended;	/* finished writing full line */
> 
> I would prefer to make it more technically exact. It would require a
> multi-line comment and this header uses 2 different styles for
> multi-line field comments.
> 
> How about:
> 
> 	/*
> 	 * Track when a console line has been fully written to the
> 	 * hardware, i.e. true when the most recent byte written by
> 	 * the console to UART_TX was '\n'.
> 	 */
> 	bool			console_line_ended;

Looks good to me.

Best Regards,
Petr