On Mon, Oct 10, 2022 at 03:17:14PM +0900, Dominique MARTINET wrote:
+Cc stable
Lukas Wunner wrote on Thu, Sep 22, 2022 at 06:27:33PM +0200:
When a UART port is newly registered, uart_configure_port() seeks to deassert RS485 Transmit Enable by setting the RTS bit in port->mctrl. However a number of UART drivers interpret a set RTS bit as *assertion* instead of deassertion: Affected drivers include those using serial8250_em485_config() (except 8250_bcm2835aux.c) and some using mctrl_gpio (e.g. imx.c).
Since the interpretation of the RTS bit is driver-specific, it is not suitable as a means to centrally deassert Transmit Enable in the serial core. Instead, the serial core must call on drivers to deassert it in their driver-specific way. One way to achieve that is to call ->rs485_config(). It implicitly deasserts Transmit Enable.
So amend uart_configure_port() and uart_resume_port() to invoke uart_rs485_config(). That allows removing calls to uart_rs485_config() from drivers' ->probe() hooks and declaring the function static.
Skip any invocation of ->set_mctrl() if RS485 is enabled. RS485 has no hardware flow control, so the modem control lines are irrelevant and need not be touched. When leaving RS485 mode, reset the modem control lines to the state stored in port->mctrl. That way, UARTs which are muxed between RS485 and RS232 transceivers drive the lines correctly when switched to RS232. (serial8250_do_startup() historically raises the OUT1 modem signal because otherwise interrupts are not signaled on ancient PC UARTs, but I believe that no longer applies to modern, RS485-capable UARTs and is thus safe to be skipped.)
imx.c modifies port->mctrl whenever Transmit Enable is asserted and deasserted. Stop it from doing that so port->mctrl reflects the RS232 line state.
8250_omap.c deasserts Transmit Enable on ->runtime_resume() by calling ->set_mctrl(). Because that is now a no-op in RS485 mode, amend the function to call serial8250_em485_stop_tx().
fsl_lpuart.c retrieves and applies the RS485 device tree properties after registering the UART port. Because applying now happens on registration in uart_configure_port(), move retrieval of the properties ahead of uart_add_one_port().
Fixes: d3b3404df318 ("serial: Fix incorrect rs485 polarity on uart open")
Thanks for this fix! We also noticed rs485 DE was initially wrong last week and I noticed this when I was about to send a patch that just inverted the SER_RS485_RTS_AFTER_SEND check in uart_configure_port, but after reading the commit message here it's a lot more complicated than that depending on the serial driver... (fixing commit 2dd8a74fddd2 ("serial: core: Initialize rs485 RTS polarity already on probe"), but it's actually the same problem in the opposite direction)
Unfortunately you've marked this for v4.14+ stable, but it doesn't even apply to 5.19.14 due to (at least) commit d8fcd9cfbde5 ("serial: core: move sanitizing of RS485 delays into own function"), so it hasn't been picked up yet; since quite a bit of code was cleaned up/moved one will need to pay a bit attention when doing that.
What would you like to do for stable branches? Would you be able to send a patch that applies on older 5.10 and 5.15 where commit d3b3404df318 ("serial: Fix incorrect rs485 polarity on uart open") has been backported?
If that sounds too complicated we could probably just revert a handful of serial_core/rs485 commits, but going forward sounds more future-proof to me.
Thanks! (nothing below, leaving quote for stable@)
<formletter>
This is not the correct way to submit patches for inclusion in the stable kernel tree. Please read: https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html for how to do this properly.
</formletter>