Hi, Jiri,
On 08.11.2024 14:19, Claudiu Beznea wrote:
@@ -885,6 +887,7 @@ static void sci_transmit_chars(struct uart_port *port) } sci_serial_out(port, SCxTDR, c); + s->first_time_tx = true; port->icount.tx++; } while (--count > 0); @@ -1241,6 +1244,8 @@ static void sci_dma_tx_complete(void *arg) if (kfifo_len(&tport->xmit_fifo) < WAKEUP_CHARS) uart_write_wakeup(port); + s->first_time_tx = true;
This is too late IMO. The first in-flight dma won't be accounted in sci_tx_empty(). From DMA submit up to now.
If it's in-flight we can't determine it's status anyway with one variable. We can set this variable later but it wouldn't tell the truth as the TX might be in progress anyway or may have been finished?
The hardware might help with this though the TEND bit. According to the HW manual, the TEND bit has the following meaning:
0: Transmission is in the waiting state or in progress. 1: Transmission is completed.
But the problem, from my point of view, is that the 0 has double meaning.
I noticed the tx_empty() is called in kernel multiple times before declaring TX is empty or not. E.g., uart_suspend_port() call it 3 times, uart_wait_until_sent() call it in a while () look with a timeout. There is the uart_ioctl() which calls it though uart_get_lsr_info() only one time but I presumed the user space might implement the same multiple trials approach before declaring it empty.
Because of this I considered it wouldn't be harmful for the scenario you described "The first in-flight dma won't be accounted in sci_tx_empty()" as the user may try again later to check the status. For this reason I also chose to have no extra locking around this variable.
Please let me know if you consider otherwise.
With the above explanation, can you please let me know if you still consider I should change the approach for this patch?
Thank you, Claudiu Beznea