Re: [PATCH next v1 1/2] serial: 8250: Switch to nbcon console

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

On 2024-09-06, Petr Mladek <pmladek@suse.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