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

[John Ogness <john.ogness@linutronix.de>]

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.

> 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.

>> +static int fifo_wait_for_lsr(struct uart_8250_port *up,
>> +			      struct nbcon_write_context *wctxt,
>> +			      unsigned int count)
>>  {
>>  	unsigned int i;
>>  
>>  	for (i = 0; i < count; i++) {
>
> This would deserve a comment. The following comes to my mind:
>
> 		/*
> 		 * Pass the ownership as quickly as possible to a higher
> 		 * priority context. Otherwise, its attempt to take
> 		 * over the ownership might timeout. The new owner
> 		 * will wait for lsr before reusing the fifo.
> 		 */

OK.

>>  static void serial8250_console_fifo_write(struct uart_8250_port *up,
>> -					  const char *s, unsigned int count)
>> +					  struct nbcon_write_context *wctxt)
>>  {
>> -	const char *end = s + count;
>>  	unsigned int fifosize = up->tx_loadsz;
>>  	struct uart_port *port = &up->port;
>> +	const char *s = wctxt->outbuf;
>> +	const char *end = s + wctxt->len;
>>  	unsigned int tx_count = 0;
>>  	bool cr_sent = false;
>>  	unsigned int i;
>>  
>>  	while (s != end) {
>>  		/* Allow timeout for each byte of a possibly full FIFO */
>> -		fifo_wait_for_lsr(up, fifosize);
>> +		if (fifo_wait_for_lsr(up, wctxt, fifosize) == -EPERM)
>> +			return;
>>  
>>  		for (i = 0; i < fifosize && s != end; ++i) {
>>  			if (*s == '\n' && !cr_sent) {
>
> We might want to check nbcon_can_proceed() after each character.
> I think that you have already suggested this in the reply to Andy.

Yes v5 will check per character.

> But maybe, it is not that important because filling the FIFO buffer is
> probably fast.

Surely it could fill the FIFO in less time than the timeout (currently
hard-coded at 2ms). But I do not think we should rely on such timings
since they could change later. Also, since the CPU is busy-waiting
during the full Tx anyway, the check is not wasting any CPU cycles or
slowing the Tx.

>> +	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;

John