Re: [PATCH tty-next v4 4/6] serial: 8250: Provide flag for IER toggling for RS485

[Petr Mladek <pmladek@suse.com>]

On Fri 2024-12-27 23:51:20, John Ogness wrote:
> For RS485 mode, if SER_RS485_RX_DURING_TX is not available, the
> console ->write() callback needs to enable/disable Tx. It does
> this by calling the ->rs485_start_tx() and ->rs485_stop_tx()
> callbacks. However, some of these callbacks also disable/enable
> interrupts and makes power management calls. This causes 2
> problems for console writing:
> 
> 1. A console write can occur in contexts that are illegal for
>    pm_runtime_*(). It is not even necessary for console writing
>    to use pm_runtime_*() because a console already does this in
>    serial8250_console_setup() and serial8250_console_exit().
>
> 2. The console ->write() callback already handles
>    disabling/enabling the interrupts by properly restoring the
>    previous IER value.

I was a bit confused by the description of the 2nd problem.
It is not clear whether it actually is a problem.

My understanding is that the nested IER manipulation does not
harm. And it is not needed at the same time. As a result,
the related pr_runtime_*() calls are not needed either.
So this is 2nd reason why the problematic pr_runtime_*() calls
can be removed in the serial8250_console_write() code path.

> Add an argument @toggle_ier to the ->rs485_start_tx() and
> ->rs485_stop_tx() callbacks to specify if they may disable/enable
> receive interrupts while using pm_runtime_*(). Console writing
> will not allow the toggling.
> 
> For all call sites other than console writing there is no
> functional change.
> 
> Signed-off-by: John Ogness <john.ogness@linutronix.de>

It seems that the patch does what is says. I'll try to describe
it using my words to explain how I understand it.

IMHO, the main motivation is to prevent calling pm_runtime_*() in
serial8250_console_write(). It is not safe in some contexts,
especially in NMI. And it is not needed from two reasons:

  1. The console->write() callback is used only by registered consoles.
     And the pm_runtime usage counter is already bumped during
     the console registration by serial8250_console_setup().

  2. The pm_runtime_*() calls are used in the following code path

	+ serial8250_console_write()
	  + up->rs485_start_tx()
	    + serial8250_em485_start_tx()
	      + serial8250_stop_rx()

     This code is not needed because serial8250_console_write()
     always clears IER by __serial8250_clear_IER(up) anyway.

     In fact, the extra IER manipulation in serial8250_em485_start_tx()
     might be seen as a bug. Well, it is a NOP.

All in all, the patch looks good to me.

Reviewed-by: Petr Mladek <pmladek@suse.com>

Best Regards,
Petr