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