| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <154336440880.88331.11610393939844825622@swboyd.mtv.corp.google.com>
Date: Tue, 27 Nov 2018 16:20:08 -0800
From: Stephen Boyd <swboyd@...omium.org>
To: Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
Jiri Slaby <jslaby@...e.com>,
Ryan Case <ryandcase@...omium.org>
Cc: Evan Green <evgreen@...omium.org>,
Doug Anderson <dianders@...omium.org>,
linux-kernel@...r.kernel.org, linux-serial@...r.kernel.org,
Ryan Case <ryandcase@...omium.org>
Subject: Re: [PATCH] tty: serial: qcom_geni_serial: Fix softlock
Quoting Ryan Case (2018-11-26 18:25:36)
> 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@...omium.org>
> Version: 1
I've never seen a Version tag before. Did you manually add this?
> diff --git a/drivers/tty/serial/qcom_geni_serial.c b/drivers/tty/serial/qcom_geni_serial.c
> index 7ded51081add..835a184e0b7d 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;
> +
> + u32 cur_tx_remaining;
Nitpick: Can it just be tx_remaining? And why u32? Why not unsigned int?
> };
>
> 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;
> + unsigned int geni_status;
Nitpick: Use u32 for register reads.
>
> WARN_ON(co->index < 0 || co->index >= GENI_UART_CONS_PORTS);
>
> @@ -465,9 +470,17 @@ 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->cur_tx_remaining)
> + /* It seems we can interrupt existing transfers unless all data
Nitpick: Have /* on a line by itself
Is this comment supposed to say "we can't interrupt existing transfers"?
> + * has been sent, in which case we need to look for done first.
> + */
> + qcom_geni_serial_poll_tx_done(uport);
Another nitpick: Please put braces around multi-line if branches for
greater code clarity.
>
> __qcom_geni_serial_console_write(uport, s, count);
> +
> + if (port->cur_tx_remaining)
> + qcom_geni_serial_setup_tx(uport, port->cur_tx_remaining);
Does this happen? Is the console being used as a tty at the same time?
> +
> if (locked)
> spin_unlock_irqrestore(&uport->lock, flags);
> }
> @@ -701,40 +714,47 @@ 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->cur_tx_remaining;
> + else
> + pending = uart_circ_chars_pending(xmit);
> +
> + /* All data has been transmitted and acknowledged as received */
> + if (!pending && !status && done) {
Nitpick: status is a poor variable name to test here. I don't understand
what this line is doing. Maybe it would help to have another local
variable like 'needs_attention'?
> 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;
> + if (avail < 0)
> + avail = 0;
How can 'avail' be less than 0? It's size_t which is unsigned? If
underflow is happening from that subtraction or overflow from the
multiply that could be bad but I hope that is impossible.
>
> - 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((size_t)pending, (size_t)(UART_XMIT_SIZE - tail), avail);
Nitpick: If we made 'avail' unsigned int would we be able to drop the
casts on this min3() call? This line is quite hard to read.
> if (!chunk)
> goto out_write_wakeup;
>
> - qcom_geni_serial_setup_tx(uport, chunk);
> + if (!port->cur_tx_remaining) {
> + qcom_geni_serial_setup_tx(uport, pending);
> + port->cur_tx_remaining = pending;
> + }
>
> remaining = chunk;
> for (i = 0; i < chunk; ) {
> @@ -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;
Nitpick: I guess this driver isn't using u32 for registers already.
Would be nice to mop this up in another patch.
> struct uart_port *uport = dev;
> unsigned long flags;
> unsigned int m_irq_en;
>
Powered by blists - more mailing lists