Transfers were being divided into device FIFO sized (64 byte max)
operations which would poll for completion within a spin_lock_irqsave /
spin_unlock_irqrestore block. This both made things slow by waiting for
the FIFO to completely drain before adding further data and would also
result in softlocks on large transmissions.
This patch allows larger transfers with continuous FIFO additions as
space becomes available and removes polling from the interrupt handler.
Signed-off-by: Ryan Case <[email protected]>
---
Changes in v2:
- Addressed nits raised by Stephen
drivers/tty/serial/qcom_geni_serial.c | 56 +++++++++++++++++++--------
1 file changed, 39 insertions(+), 17 deletions(-)
diff --git a/drivers/tty/serial/qcom_geni_serial.c b/drivers/tty/serial/qcom_geni_serial.c
index 7ded51081add..26f7c0df83ae 100644
--- a/drivers/tty/serial/qcom_geni_serial.c
+++ b/drivers/tty/serial/qcom_geni_serial.c
@@ -117,6 +117,8 @@ struct qcom_geni_serial_port {
u32 *rx_fifo;
u32 loopback;
bool brk;
+
+ unsigned int tx_remaining;
};
static const struct uart_ops qcom_geni_console_pops;
@@ -439,6 +441,7 @@ static void qcom_geni_serial_console_write(struct console *co, const char *s,
struct qcom_geni_serial_port *port;
bool locked = true;
unsigned long flags;
+ u32 geni_status;
WARN_ON(co->index < 0 || co->index >= GENI_UART_CONS_PORTS);
@@ -452,6 +455,8 @@ static void qcom_geni_serial_console_write(struct console *co, const char *s,
else
spin_lock_irqsave(&uport->lock, flags);
+ geni_status = readl_relaxed(uport->membase + SE_GENI_STATUS);
+
/* Cancel the current write to log the fault */
if (!locked) {
geni_se_cancel_m_cmd(&port->se);
@@ -465,9 +470,19 @@ static void qcom_geni_serial_console_write(struct console *co, const char *s,
}
writel_relaxed(M_CMD_CANCEL_EN, uport->membase +
SE_GENI_M_IRQ_CLEAR);
+ } else if ((geni_status & M_GENI_CMD_ACTIVE) && !port->tx_remaining) {
+ /*
+ * It seems we can interrupt existing transfers unless all data
+ * has been sent, in which case we need to look for done first.
+ */
+ qcom_geni_serial_poll_tx_done(uport);
}
__qcom_geni_serial_console_write(uport, s, count);
+
+ if (port->tx_remaining)
+ qcom_geni_serial_setup_tx(uport, port->tx_remaining);
+
if (locked)
spin_unlock_irqrestore(&uport->lock, flags);
}
@@ -701,40 +716,45 @@ static void qcom_geni_serial_handle_rx(struct uart_port *uport, bool drop)
port->handle_rx(uport, total_bytes, drop);
}
-static void qcom_geni_serial_handle_tx(struct uart_port *uport)
+static void qcom_geni_serial_handle_tx(struct uart_port *uport, bool done,
+ bool active)
{
struct qcom_geni_serial_port *port = to_dev_port(uport, uport);
struct circ_buf *xmit = &uport->state->xmit;
size_t avail;
size_t remaining;
+ size_t pending;
int i;
u32 status;
unsigned int chunk;
int tail;
- u32 irq_en;
- chunk = uart_circ_chars_pending(xmit);
status = readl_relaxed(uport->membase + SE_GENI_TX_FIFO_STATUS);
- /* Both FIFO and framework buffer are drained */
- if (!chunk && !status) {
+
+ /* Complete the current tx command before taking newly added data */
+ if (active)
+ pending = port->tx_remaining;
+ else
+ pending = uart_circ_chars_pending(xmit);
+
+ /* All data has been transmitted and acknowledged as received */
+ if (!pending && !status && done) {
qcom_geni_serial_stop_tx(uport);
goto out_write_wakeup;
}
- if (!uart_console(uport)) {
- irq_en = readl_relaxed(uport->membase + SE_GENI_M_IRQ_EN);
- irq_en &= ~(M_TX_FIFO_WATERMARK_EN);
- writel_relaxed(0, uport->membase + SE_GENI_TX_WATERMARK_REG);
- writel_relaxed(irq_en, uport->membase + SE_GENI_M_IRQ_EN);
- }
+ avail = port->tx_fifo_depth - (status & TX_FIFO_WC);
+ avail *= port->tx_bytes_pw;
- avail = (port->tx_fifo_depth - port->tx_wm) * port->tx_bytes_pw;
tail = xmit->tail;
- chunk = min3((size_t)chunk, (size_t)(UART_XMIT_SIZE - tail), avail);
+ chunk = min3(avail, pending, (UART_XMIT_SIZE - tail));
if (!chunk)
goto out_write_wakeup;
- qcom_geni_serial_setup_tx(uport, chunk);
+ if (!port->tx_remaining) {
+ qcom_geni_serial_setup_tx(uport, pending);
+ port->tx_remaining = pending;
+ }
remaining = chunk;
for (i = 0; i < chunk; ) {
@@ -753,11 +773,10 @@ static void qcom_geni_serial_handle_tx(struct uart_port *uport)
tail += tx_bytes;
uport->icount.tx += tx_bytes;
remaining -= tx_bytes;
+ port->tx_remaining -= tx_bytes;
}
xmit->tail = tail & (UART_XMIT_SIZE - 1);
- if (uart_console(uport))
- qcom_geni_serial_poll_tx_done(uport);
out_write_wakeup:
if (uart_circ_chars_pending(xmit) < WAKEUP_CHARS)
uart_write_wakeup(uport);
@@ -767,6 +786,7 @@ static irqreturn_t qcom_geni_serial_isr(int isr, void *dev)
{
unsigned int m_irq_status;
unsigned int s_irq_status;
+ unsigned int geni_status;
struct uart_port *uport = dev;
unsigned long flags;
unsigned int m_irq_en;
@@ -780,6 +800,7 @@ static irqreturn_t qcom_geni_serial_isr(int isr, void *dev)
spin_lock_irqsave(&uport->lock, flags);
m_irq_status = readl_relaxed(uport->membase + SE_GENI_M_IRQ_STATUS);
s_irq_status = readl_relaxed(uport->membase + SE_GENI_S_IRQ_STATUS);
+ geni_status = readl_relaxed(uport->membase + SE_GENI_STATUS);
m_irq_en = readl_relaxed(uport->membase + SE_GENI_M_IRQ_EN);
writel_relaxed(m_irq_status, uport->membase + SE_GENI_M_IRQ_CLEAR);
writel_relaxed(s_irq_status, uport->membase + SE_GENI_S_IRQ_CLEAR);
@@ -794,7 +815,8 @@ static irqreturn_t qcom_geni_serial_isr(int isr, void *dev)
if (m_irq_status & (M_TX_FIFO_WATERMARK_EN | M_CMD_DONE_EN) &&
m_irq_en & (M_TX_FIFO_WATERMARK_EN | M_CMD_DONE_EN))
- qcom_geni_serial_handle_tx(uport);
+ qcom_geni_serial_handle_tx(uport, m_irq_status & M_CMD_DONE_EN,
+ geni_status & M_GENI_CMD_ACTIVE);
if (s_irq_status & S_GP_IRQ_0_EN || s_irq_status & S_GP_IRQ_1_EN) {
if (s_irq_status & S_GP_IRQ_0_EN)
--
2.20.0.rc0.387.gc7a69e6b6c-goog
Hi Ryan,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on tty/tty-testing]
[also build test WARNING on v4.20-rc4 next-20181129]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Ryan-Case/tty-serial-qcom_geni_serial-Fix-softlock/20181129-174407
base: https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/tty.git tty-testing
config: mips-allyesconfig (attached as .config)
compiler: mips-linux-gnu-gcc (Debian 7.2.0-11) 7.2.0
reproduce:
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
GCC_VERSION=7.2.0 make.cross ARCH=mips
All warnings (new ones prefixed by >>):
In file included from include/linux/clk.h:16:0,
from drivers/tty/serial/qcom_geni_serial.c:8:
drivers/tty/serial/qcom_geni_serial.c: In function 'qcom_geni_serial_handle_tx':
include/linux/kernel.h:845:29: warning: comparison of distinct pointer types lacks a cast
(!!(sizeof((typeof(x) *)1 == (typeof(y) *)1)))
^
include/linux/kernel.h:859:4: note: in expansion of macro '__typecheck'
(__typecheck(x, y) && __no_side_effects(x, y))
^~~~~~~~~~~
include/linux/kernel.h:869:24: note: in expansion of macro '__safe_cmp'
__builtin_choose_expr(__safe_cmp(x, y), \
^~~~~~~~~~
include/linux/kernel.h:878:19: note: in expansion of macro '__careful_cmp'
#define min(x, y) __careful_cmp(x, y, <)
^~~~~~~~~~~~~
include/linux/kernel.h:893:23: note: in expansion of macro 'min'
#define min3(x, y, z) min((typeof(x))min(x, y), z)
^~~
>> drivers/tty/serial/qcom_geni_serial.c:746:10: note: in expansion of macro 'min3'
chunk = min3(avail, pending, (UART_XMIT_SIZE - tail));
^~~~
vim +/min3 +746 drivers/tty/serial/qcom_geni_serial.c
714
715 static void qcom_geni_serial_handle_tx(struct uart_port *uport, bool done,
716 bool active)
717 {
718 struct qcom_geni_serial_port *port = to_dev_port(uport, uport);
719 struct circ_buf *xmit = &uport->state->xmit;
720 size_t avail;
721 size_t remaining;
722 size_t pending;
723 int i;
724 u32 status;
725 unsigned int chunk;
726 int tail;
727
728 status = readl_relaxed(uport->membase + SE_GENI_TX_FIFO_STATUS);
729
730 /* Complete the current tx command before taking newly added data */
731 if (active)
732 pending = port->tx_remaining;
733 else
734 pending = uart_circ_chars_pending(xmit);
735
736 /* All data has been transmitted and acknowledged as received */
737 if (!pending && !status && done) {
738 qcom_geni_serial_stop_tx(uport);
739 goto out_write_wakeup;
740 }
741
742 avail = port->tx_fifo_depth - (status & TX_FIFO_WC);
743 avail *= port->tx_bytes_pw;
744
745 tail = xmit->tail;
> 746 chunk = min3(avail, pending, (UART_XMIT_SIZE - tail));
747 if (!chunk)
748 goto out_write_wakeup;
749
750 if (!port->tx_remaining) {
751 qcom_geni_serial_setup_tx(uport, pending);
752 port->tx_remaining = pending;
753 }
754
755 remaining = chunk;
756 for (i = 0; i < chunk; ) {
757 unsigned int tx_bytes;
758 u8 buf[sizeof(u32)];
759 int c;
760
761 memset(buf, 0, ARRAY_SIZE(buf));
762 tx_bytes = min_t(size_t, remaining, port->tx_bytes_pw);
763 for (c = 0; c < tx_bytes ; c++)
764 buf[c] = xmit->buf[tail + c];
765
766 iowrite32_rep(uport->membase + SE_GENI_TX_FIFOn, buf, 1);
767
768 i += tx_bytes;
769 tail += tx_bytes;
770 uport->icount.tx += tx_bytes;
771 remaining -= tx_bytes;
772 port->tx_remaining -= tx_bytes;
773 }
774
775 xmit->tail = tail & (UART_XMIT_SIZE - 1);
776 out_write_wakeup:
777 if (uart_circ_chars_pending(xmit) < WAKEUP_CHARS)
778 uart_write_wakeup(uport);
779 }
780
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
Hi,
On Wed, Nov 28, 2018 at 3:55 PM Ryan Case <[email protected]> wrote:
> @@ -465,9 +470,19 @@ static void qcom_geni_serial_console_write(struct console *co, const char *s,
> }
> writel_relaxed(M_CMD_CANCEL_EN, uport->membase +
> SE_GENI_M_IRQ_CLEAR);
> + } else if ((geni_status & M_GENI_CMD_ACTIVE) && !port->tx_remaining) {
> + /*
> + * It seems we can interrupt existing transfers unless all data
s/It seems we can interrupt/It seems we can't interrupt/
> +static void qcom_geni_serial_handle_tx(struct uart_port *uport, bool done,
> + bool active)
> {
> struct qcom_geni_serial_port *port = to_dev_port(uport, uport);
> struct circ_buf *xmit = &uport->state->xmit;
> size_t avail;
> size_t remaining;
> + size_t pending;
> int i;
> u32 status;
> unsigned int chunk;
> int tail;
> - u32 irq_en;
>
> - chunk = uart_circ_chars_pending(xmit);
> status = readl_relaxed(uport->membase + SE_GENI_TX_FIFO_STATUS);
> - /* Both FIFO and framework buffer are drained */
> - if (!chunk && !status) {
> +
> + /* Complete the current tx command before taking newly added data */
> + if (active)
> + pending = port->tx_remaining;
I almost feel like this should be:
if (port->tx_remaining)
pending = port->tx_remaining
I could imagine active being false but "port->tx_remaining" being
non-zero if we happened to take a long time to handle the interrupt
for some reason. Presumably you could simulator this and see what
breaks. I think what would happen would be "pending" will be larger
than you expect and you could write a few extra bytes into the FIFO
causing it to go beyond the length of the transfer you setup. ...so I
guess you'd drop some bytes?
If it's somehow important for "pending" to be 0 still when we're
active but port->tx_remaining is non-zero, then I guess you could also
write it as:
if (active || port->tx_remaining)
pending = port->tx_remaining
Maybe I'm misunderstanding though.
-Doug
On Thu, Nov 29, 2018 at 2:12 PM Doug Anderson <[email protected]> wrote:
>
> Hi,
>
> On Wed, Nov 28, 2018 at 3:55 PM Ryan Case <[email protected]> wrote:
> > @@ -465,9 +470,19 @@ static void qcom_geni_serial_console_write(struct console *co, const char *s,
> > }
> > writel_relaxed(M_CMD_CANCEL_EN, uport->membase +
> > SE_GENI_M_IRQ_CLEAR);
> > + } else if ((geni_status & M_GENI_CMD_ACTIVE) && !port->tx_remaining) {
> > + /*
> > + * It seems we can interrupt existing transfers unless all data
>
> s/It seems we can interrupt/It seems we can't interrupt/
Comment is correct as is, but will reword to make things clearer.
>
>
> > +static void qcom_geni_serial_handle_tx(struct uart_port *uport, bool done,
> > + bool active)
> > {
> > struct qcom_geni_serial_port *port = to_dev_port(uport, uport);
> > struct circ_buf *xmit = &uport->state->xmit;
> > size_t avail;
> > size_t remaining;
> > + size_t pending;
> > int i;
> > u32 status;
> > unsigned int chunk;
> > int tail;
> > - u32 irq_en;
> >
> > - chunk = uart_circ_chars_pending(xmit);
> > status = readl_relaxed(uport->membase + SE_GENI_TX_FIFO_STATUS);
> > - /* Both FIFO and framework buffer are drained */
> > - if (!chunk && !status) {
> > +
> > + /* Complete the current tx command before taking newly added data */
> > + if (active)
> > + pending = port->tx_remaining;
>
> I almost feel like this should be:
>
> if (port->tx_remaining)
> pending = port->tx_remaining
>
> I could imagine active being false but "port->tx_remaining" being
> non-zero if we happened to take a long time to handle the interrupt
> for some reason. Presumably you could simulator this and see what
> breaks. I think what would happen would be "pending" will be larger
> than you expect and you could write a few extra bytes into the FIFO
> causing it to go beyond the length of the transfer you setup. ...so I
> guess you'd drop some bytes?
>
>
> If it's somehow important for "pending" to be 0 still when we're
> active but port->tx_remaining is non-zero, then I guess you could also
> write it as:
>
> if (active || port->tx_remaining)
> pending = port->tx_remaining
>
>
> Maybe I'm misunderstanding though.
Yeah, this flag has different behavior on the hardware than you're
expecting. Active will be set for the duration of the command no
matter the size of the transfer or across how many interrupts the
transfer takes, including after we're done on our side
(port->tx_remaining is zero).
>
>
> -Doug