| Message ID | 20241106120118.1719888-3-claudiu.beznea.uj@bp.renesas.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
| Series | Add support for the rest of Renesas RZ/G3S serial interfaces | expand |
On Wed, Nov 06, 2024 at 02:01:11PM +0200, Claudiu wrote: > From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> > > On the Renesas RZ/G3S, when doing suspend to RAM, the uart_suspend_port() > is called. The uart_suspend_port() calls 3 times the > struct uart_port::ops::tx_empty() before shutting down the port. > > According to the documentation, the struct uart_port::ops::tx_empty() > API tests whether the transmitter FIFO and shifter for the port is > empty. > > The Renesas RZ/G3S SCIFA IP reports the number of data units stored in the > transmit FIFO through the FDR (FIFO Data Count Register). The data units > in the FIFOs are written in the shift register and transmitted from there. > The TEND bit in the Serial Status Register reports if the data was > transmitted from the shift register. > > In the previous code, in the tx_empty() API implemented by the sh-sci > driver, it is considered that the TX is empty if the hardware reports the > TEND bit set and the number of data units in the FIFO is zero. > > 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. > > It has been noticed that when opening the serial device w/o using it and > then switch to a power saving mode, the tx_empty() call in the > uart_port_suspend() function fails, leading to the "Unable to drain > transmitter" message being printed on the console. This is because the > TEND=0 if nothing has been transmitted and the FIFOs are empty. As the > TEND=0 has double meaning (waiting state, in progress) we can't > determined the scenario described above. > > Add a software workaround for this. This sets a variable if any data has > been sent on the serial console (when using PIO) or if the DMA callback has > been called (meaning something has been transmitted). > > Fixes: 73a19e4c0301 ("serial: sh-sci: Add DMA support.") > Cc: stable@vger.kernel.org > Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> > --- > drivers/tty/serial/sh-sci.c | 11 +++++++++++ > 1 file changed, 11 insertions(+) > > diff --git a/drivers/tty/serial/sh-sci.c b/drivers/tty/serial/sh-sci.c > index df523c744423..8e2d534401fa 100644 > --- a/drivers/tty/serial/sh-sci.c > +++ b/drivers/tty/serial/sh-sci.c > @@ -153,6 +153,7 @@ struct sci_port { > int rx_trigger; > struct timer_list rx_fifo_timer; > int rx_fifo_timeout; > + atomic_t first_time_tx; Don't use an atomic variable for an informational thing like this, it is racy and doesn't work properly. Either use a real lock (because you care about the locking stuff here), or just use a boolean and live with any potential races. > u16 hscif_tot; > > bool has_rtscts; > @@ -850,6 +851,7 @@ static void sci_transmit_chars(struct uart_port *port) > { > struct tty_port *tport = &port->state->port; > unsigned int stopped = uart_tx_stopped(port); > + struct sci_port *s = to_sci_port(port); > unsigned short status; > unsigned short ctrl; > int count; > @@ -885,6 +887,7 @@ static void sci_transmit_chars(struct uart_port *port) > } > > sci_serial_out(port, SCxTDR, c); > + atomic_set(&s->first_time_tx, 1); > > 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); > > + atomic_set(&s->first_time_tx, 1); > + > if (!kfifo_is_empty(&tport->xmit_fifo)) { > s->cookie_tx = 0; > schedule_work(&s->work_tx); > @@ -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 (!atomic_read(&s->first_time_tx)) > + return TIOCSER_TEMT; See, what happens here if the value changes right after you check it? Being an atomic doesn't mean anything :( thanks, greg k-h
Hi, Greg, On 07.11.2024 10:47, Greg KH wrote: > On Wed, Nov 06, 2024 at 02:01:11PM +0200, Claudiu wrote: >> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> >> >> On the Renesas RZ/G3S, when doing suspend to RAM, the uart_suspend_port() >> is called. The uart_suspend_port() calls 3 times the >> struct uart_port::ops::tx_empty() before shutting down the port. >> >> According to the documentation, the struct uart_port::ops::tx_empty() >> API tests whether the transmitter FIFO and shifter for the port is >> empty. >> >> The Renesas RZ/G3S SCIFA IP reports the number of data units stored in the >> transmit FIFO through the FDR (FIFO Data Count Register). The data units >> in the FIFOs are written in the shift register and transmitted from there. >> The TEND bit in the Serial Status Register reports if the data was >> transmitted from the shift register. >> >> In the previous code, in the tx_empty() API implemented by the sh-sci >> driver, it is considered that the TX is empty if the hardware reports the >> TEND bit set and the number of data units in the FIFO is zero. >> >> 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. >> >> It has been noticed that when opening the serial device w/o using it and >> then switch to a power saving mode, the tx_empty() call in the >> uart_port_suspend() function fails, leading to the "Unable to drain >> transmitter" message being printed on the console. This is because the >> TEND=0 if nothing has been transmitted and the FIFOs are empty. As the >> TEND=0 has double meaning (waiting state, in progress) we can't >> determined the scenario described above. >> >> Add a software workaround for this. This sets a variable if any data has >> been sent on the serial console (when using PIO) or if the DMA callback has >> been called (meaning something has been transmitted). >> >> Fixes: 73a19e4c0301 ("serial: sh-sci: Add DMA support.") >> Cc: stable@vger.kernel.org >> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> >> --- >> drivers/tty/serial/sh-sci.c | 11 +++++++++++ >> 1 file changed, 11 insertions(+) >> >> diff --git a/drivers/tty/serial/sh-sci.c b/drivers/tty/serial/sh-sci.c >> index df523c744423..8e2d534401fa 100644 >> --- a/drivers/tty/serial/sh-sci.c >> +++ b/drivers/tty/serial/sh-sci.c >> @@ -153,6 +153,7 @@ struct sci_port { >> int rx_trigger; >> struct timer_list rx_fifo_timer; >> int rx_fifo_timeout; >> + atomic_t first_time_tx; > > Don't use an atomic variable for an informational thing like this, it is > racy and doesn't work properly. Either use a real lock (because you > care about the locking stuff here), or just use a boolean and live with > any potential races. OK, I'll drop it and use a boolean. > > > >> u16 hscif_tot; >> >> bool has_rtscts; >> @@ -850,6 +851,7 @@ static void sci_transmit_chars(struct uart_port *port) >> { >> struct tty_port *tport = &port->state->port; >> unsigned int stopped = uart_tx_stopped(port); >> + struct sci_port *s = to_sci_port(port); >> unsigned short status; >> unsigned short ctrl; >> int count; >> @@ -885,6 +887,7 @@ static void sci_transmit_chars(struct uart_port *port) >> } >> >> sci_serial_out(port, SCxTDR, c); >> + atomic_set(&s->first_time_tx, 1); >> >> 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); >> >> + atomic_set(&s->first_time_tx, 1); >> + >> if (!kfifo_is_empty(&tport->xmit_fifo)) { >> s->cookie_tx = 0; >> schedule_work(&s->work_tx); >> @@ -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 (!atomic_read(&s->first_time_tx)) >> + return TIOCSER_TEMT; > > See, what happens here if the value changes right after you check it? I agree. I am aware if it. I chose this approach (w/o locking) as I noticed (as of my code checking) that this function is called in kernel through uart_ioctl(), uart_wait_until_sent(), uart_suspend_port(). The uart_wait_until_sent(), uart_suspend_port() are implementing a multiple try approach when checking the ops::tx_timeout() return value. I haven't checked any user space application but considered that it might work in a similar way. I will switch to a boolean in the next version. Thank you, Claudiu Beznea > Being an atomic doesn't mean anything :( > > thanks, > > greg k-h
diff --git a/drivers/tty/serial/sh-sci.c b/drivers/tty/serial/sh-sci.c index df523c744423..8e2d534401fa 100644 --- a/drivers/tty/serial/sh-sci.c +++ b/drivers/tty/serial/sh-sci.c @@ -153,6 +153,7 @@ struct sci_port { int rx_trigger; struct timer_list rx_fifo_timer; int rx_fifo_timeout; + atomic_t first_time_tx; u16 hscif_tot; bool has_rtscts; @@ -850,6 +851,7 @@ static void sci_transmit_chars(struct uart_port *port) { struct tty_port *tport = &port->state->port; unsigned int stopped = uart_tx_stopped(port); + struct sci_port *s = to_sci_port(port); unsigned short status; unsigned short ctrl; int count; @@ -885,6 +887,7 @@ static void sci_transmit_chars(struct uart_port *port) } sci_serial_out(port, SCxTDR, c); + atomic_set(&s->first_time_tx, 1); 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); + atomic_set(&s->first_time_tx, 1); + if (!kfifo_is_empty(&tport->xmit_fifo)) { s->cookie_tx = 0; schedule_work(&s->work_tx); @@ -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 (!atomic_read(&s->first_time_tx)) + return TIOCSER_TEMT; return (status & SCxSR_TEND(port)) && !in_tx_fifo ? TIOCSER_TEMT : 0; } @@ -2247,6 +2256,7 @@ static int sci_startup(struct uart_port *port) dev_dbg(port->dev, "%s(%d)\n", __func__, port->line); + atomic_set(&s->first_time_tx, 0); sci_request_dma(port); ret = sci_request_irq(s); @@ -2267,6 +2277,7 @@ static void sci_shutdown(struct uart_port *port) dev_dbg(port->dev, "%s(%d)\n", __func__, port->line); s->autorts = false; + atomic_set(&s->first_time_tx, 0); mctrl_gpio_disable_ms(to_sci_port(port)->gpios); uart_port_lock_irqsave(port, &flags);