Re: [PATCH tty-next v3 4/6] serial: 8250: Specify console context for rs485_start/stop_tx

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

On 2024-11-06, Petr Mladek <pmladek@suse.com> 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/stop_tx() callbacks. However, these
>> callbacks will disable/enable interrupts, which is a problem
>> for console write, as it must be responsible for
>> disabling/enabling interrupts.
>
> It is not clear to me what exactly is the problem.

serial8250_em485_stop_tx() blindly sets the RX interrupt bits in IER,
because it assumes they were cleared in serial8250_stop_rx(). This is
fine for the driver in general, but it is wrong for the console
->write(), which restores those bits on its own later.

> Is the main problem calling pm_runtime*() API because it uses extra
> locks and can cause deadlocks? Or is it more complicated?

pm_runtime*() is a second issue. In the v1 feeback we talked about
it. tglx summarized it well here:

https://lore.kernel.org/lkml/8734mbdwrf.ffs@tglx/

as well as explaining the need to split off the console-write code from
the generic driver code.

> IMHO, it would deserve some explanation.

This commit message only talks about the first issue, which is enough to
justify the patch. I will add that the callbacks are also not
appropriate because they call into the PM code, which is not needed by
console ->write() and is even unsafe in some contexts.

> IMHO, one thing which makes things comlicated is that
> serial8250_em485_start_tx() and serial8250_em485_stop_tx()
> are not completely reversible operations. Especially,
> the change done by __serial8250_stop_rx_mask_dr() is
> not reverted in serial8250_em485_stop_tx(). It makes
> things look tricky. But I think that it is beyond the scope
> of this patchset to do anything about it.

I agree that it is strange that the driver does not unmask DR later. I
have now run tests and it seems the use of @read_status_mask is
partially broken. I did some historical digging on it...

For Linux 1.1.60 [0] the @read_status_mask usage was extended to support
"stop listening to incoming characters" (text from the changelog
[1]). Looking at that version, it is clear why and how it was used.

For Linux 2.1.8 [2], the async handling was reworked, basically
reverting the change from 1.1.60. However, that revert forgot the piece
that clears the UART_LSR_DR bit in serial8250_stop_rx() (back then
called rs_close()).

And indeed, if you track the @read_status_mask value today, that bit
remains cleared until serial8250_do_set_termios() happens to be
called. But it didn't matter that the bit was not set again because that
bit was not being evaluated at any call sites.

For 4.6, RS485 support was added, but with a bug about re-enabling
interrupts. When that bug was fixed [3], the fix did not set the
UART_LSR_DR bit in @read_status_mask. Still that was not a problem
because at that time, that bit still had no users.

For 5.7, support was added to avoid reading characters when
throttling. This re-introduced a user of the UART_LSR_DR bit in
@read_status_mask. And thus now there _is_ a bug that the bit is not set
when starting RX in __do_stop_tx_rs485(). Interestingly enough, the OMAP
variant of the 8250 _did_ implement setting the bit when unthrottling
[5] (also from the same series).

So in summary, I will add a patch to my series that fixes [3] (or is it
fixing [4]?) by setting the bit in __do_stop_tx_rs485() when re-enabling
the RX interrupts.

John

[0] https://git.kernel.org/pub/scm/linux/kernel/git/history/history.git/diff/drivers/char/serial.c?id=ba97e35a1a8b45ff87ed37a58fca3ecf39c1c893
[1] https://git.kernel.org/pub/scm/linux/kernel/git/history/history.git/diff/drivers/char/ChangeLog?id=ba97e35a1a8b45ff87ed37a58fca3ecf39c1c893
[2] https://git.kernel.org/pub/scm/linux/kernel/git/history/history.git/diff/drivers/char/serial.c?id=0f9cac5b27076f801b29a0867868e1bce7310e00
[3] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit?id=0c66940d584d1aac92f6a78460dc0ba2efd3b7ba
[4] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit?id=f19c3f6c8109b8bab000afd35580929958e087a9
[5] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit?id=f4b042a050062b2dec456adfced13d61341939e2