Re: [PATCH printk v1 10/10] serial: 8250: implement write_atomic

Jiri Slaby
2021-08-05T07:47:54Z

On 03. 08. 21, 16:07, Andy Shevchenko wrote:
> On Tue, Aug 03, 2021 at 03:19:01PM +0206, John Ogness wrote:
>> Implement an NMI-safe write_atomic() console function in order to
>> support synchronous console printing.
>>
>> Since interrupts need to be disabled during transmit, all usage of
>> the IER register is wrapped with access functions that use the
>> printk cpulock to synchronize register access while tracking the
>> state of the interrupts. This is necessary because write_atomic()
>> can be called from an NMI context that has preempted write_atomic().
>
> ...
>
>> +static inline void serial8250_set_IER(struct uart_8250_port *up,
>> + unsigned char ier)
>> +{
>> + struct uart_port *port = &up->port;
>> + unsigned long flags;
>> + bool is_console;
>
>> + is_console = uart_console(port);
>> +
>> + if (is_console)
>> + console_atomic_cpu_lock(flags);
>> +
>> + serial_out(up, UART_IER, ier);
>> +
>> + if (is_console)
>> + console_atomic_cpu_unlock(flags);
>
> I would rewrite it as
>
> if (uart_console()) {
> console_atomic_cpu_lock(flags);
> serial_out(up, UART_IER, ier);
> console_atomic_cpu_unlock(flags);
> } else {
> serial_out(up, UART_IER, ier);
> }
>
> No additional variable, easier to get the algorithm on the first glance, less
> error prone.

Yes, the original is terrible.

Another option:

bool locked = console_atomic_cpu_lock(flags, uart_console());
serial_out(up, UART_IER, ier);
console_atomic_cpu_unlock(flags, locked);

Which makes console_atomic_cpu_lock to lock only if second parameter is
true and return its value too.

BTW I actually don't know what console_atomic_cpu_lock does to think
about it more as I was not CCed, and neither lore sees the other patches:
https://lore.kernel.org/linux-mips/20210803131301.5588-1-john.ogness@linutronix.de/

thanks,
--
js
suse labs