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