Hi,
On 08. 11. 24, 13:19, Claudiu Beznea wrote:
On 08.11.2024 12:57, Jiri Slaby wrote:
On 08. 11. 24, 11:05, Claudiu wrote:
...
--- a/drivers/tty/serial/sh-sci.c
+++ b/drivers/tty/serial/sh-sci.c
@@ -157,6 +157,7 @@ struct sci_port {
bool has_rtscts;
bool autorts;
+ bool first_time_tx;
This is a misnomer. It suggests to be set only during the first TX.
I chose this naming as this was the scenario I discovered it didn't work.
Reproducible though these steps:
1/ open the serial device (w/o running any TX/RX)
2/ call tx_empty()
What
about ::did_tx, ::performed_tx, ::transmitted, or alike?
I have nothing against any of these. Can you please let me know if you have
a preferred one?
No, you choose, or invent even better one :). Or let AI do it for you.
@@ -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.
What about the below?
@@ -2076,6 +2081,10 @@ static unsigned int sci_tx_empty(struct uart_port
*port)
{
unsigned short status = sci_serial_in(port, SCxSR);
unsigned short in_tx_fifo = sci_txfill(port);
+ struct sci_port *s = to_sci_port(port);
+
+ if (!s->first_time_tx)
+ return TIOCSER_TEMT;
So perhaps check if there is a TX DMA running here too?
This ^^^? Like dmaengine_tx_status()?
return (status & SCxSR_TEND(port)) && !in_tx_fifo ? TIOCSER_TEMT
: 0;
}
thanks,
--
js
suse labs
