Re: [PATCH tty-next v4 5/6] serial: 8250: Switch to nbcon console

[Petr Mladek <pmladek@suse.com>]

On Sun 2025-01-05 02:03:41, John Ogness wrote:
> On 2025-01-03, Petr Mladek <pmladek@suse.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