[PATCH v2] tty: serial: qcom_geni_serial: Fix softlock

[PATCH v2] tty: serial: qcom_geni_serial: Fix softlock

[Ryan Case]

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 <ryandcase@chromium.org>
---

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

Re: [PATCH v2] tty: serial: qcom_geni_serial: Fix softlock

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 4327 bytes --]

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

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 58323 bytes --]

Re: [PATCH v2] tty: serial: qcom_geni_serial: Fix softlock

[Doug Anderson]

Hi,

On Wed, Nov 28, 2018 at 3:55 PM Ryan Case <ryandcase@chromium.org> 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

Re: [PATCH v2] tty: serial: qcom_geni_serial: Fix softlock

[Ryan Case]

On Thu, Nov 29, 2018 at 2:12 PM Doug Anderson <dianders@chromium.org> wrote:
>
> Hi,
>
> On Wed, Nov 28, 2018 at 3:55 PM Ryan Case <ryandcase@chromium.org> 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