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@xxxxxxxxxxxxx>
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@xxxxxxxx>
Best Regards,
Petr
