Hi Jonas,
On 6/4/2024 6:04 AM, Jonas Gorski wrote:
Hi,
On Sun, 2 Jun 2024 at 19:24, Doug Brown <doug@xxxxxxxxxxxxx> wrote:
Hi Jonas,
On Mon, 20 May 2024 at 08:41, Jiri Slaby <jirislaby@xxxxxxxxxx> wrote:
Perhaps, it should be reverted and the affected driver should just pass
UART_TX_NOSTOP instead?
uart_port_tx_limited(&up->port, ch, up->port.fifosize / 2,
true,
serial_out(up, UART_TX, ch),
Does the bcm63xx_uart driver work correctly if 7bfb915a597a is reverted
and UART_TX_NOSTOP is used instead?
I was able to further confirm on hardware (BeagleBoard) that omap-serial
is also broken after 7bfb915a597a. I'm trying to figure out how to
safely revert it while also allowing bcm63xx to continue to work properly.
So I went through all users of uart_port_tx(_limited()), and
expectedly most do just disable the IRQ (or do nothing) in their
stop_tx() call back.
The notable exceptions I could identify are:
- bcm63xx_uart: disables the emitter as well
- mxs-uart: disables the emitter (introduced and uses UART_TX_NOSTOP)
- sprd_serial: stops alls dma transfers as well
- atmel_serial: stops dma and disables emitter as well
I suspect sprd_serial and atmel_serial to not work properly with the
old behavior, but I have no devices where I could test this.
I see what you mean. Unfortunately I also don't have the ability to test
either of those drivers. It looks like atmel_serial didn't previously
have a stop_tx() anywhere in the code that was refactored to use
uart_port_tx(), so I'm assuming it should also have UART_TX_NOSTOP
added. I'm not confident enough to submit a patch for it though.
sprd_serial prior to the uart_port_tx_limited() refactor was calling
stop_tx() when uart_circ_empty() was true -- in fact it was almost
identical to the pxa driver's code -- so I feel like any bug in there
related to stopping too early would have predated any of these changes.
Regardless of the chosen quick fix, it feels like stop_tx() is the
wrong thing to use for uart_port_tx(), and just happened to do the
right thing for the majority or drivers. But the correct function
(something along "done submitting tx") also doesn't exist.
That makes complete sense to me.
But I'm no maintainer for tty/serial, nor do I have much (intimate)
knowledge there. This is just from looking at it from the outside, and
my thoughts may very well be wrong.
I'm in the same boat. I wouldn't feel comfortable submitting a large fix
for something like that. There are so many hardware platforms that are
using this code, it seems very intimidating to work on. Thanks for the
analysis, and thanks for linking your previous patch! I'll work on
submitting a quick fix for this at least. I think if we go back to the
approach your earlier patchset used, it will fix the issue I'm seeing on
omap-serial and pxa while also working properly on bcm63xx_uart.
Doug
