* Re: [PATCH] tty: serial: qcom_geni_serial: Fix softlock
@ 2018-11-27 8:48 Julia Lawall
0 siblings, 0 replies; 7+ messages in thread
From: Julia Lawall @ 2018-11-27 8:48 UTC (permalink / raw)
To: Ryan Case
Cc: Greg Kroah-Hartman, Jiri Slaby, Evan Green, Doug Anderson,
linux-kernel, linux-serial
Hello,
Since size_t is unsigned, avail will not be less than 0 on line 742.
Perhaps just reorganize the computation on line 740.
julia
---------- Forwarded message ----------
Date: Tue, 27 Nov 2018 16:13:53 +0800
From: kbuild test robot <lkp@intel.com>
To: kbuild@01.org
Cc: Julia Lawall <julia.lawall@lip6.fr>
Subject: Re: [PATCH] tty: serial: qcom_geni_serial: Fix softlock
CC: kbuild-all@01.org
In-Reply-To: <20181127022536.104663-1-ryandcase@chromium.org>
References: <20181127022536.104663-1-ryandcase@chromium.org>
TO: Ryan Case <ryandcase@chromium.org>
CC: Greg Kroah-Hartman <gregkh@linuxfoundation.org>, Jiri Slaby <jslaby@suse.com>, Evan Green <evgreen@chromium.org>, Doug Anderson <dianders@chromium.org>, linux-kernel@vger.kernel.org, linux-serial@vger.kernel.org, Ryan Case <ryandcase@chromium.org>
CC: Evan Green <evgreen@chromium.org>, Doug Anderson <dianders@chromium.org>, linux-kernel@vger.kernel.org, linux-serial@vger.kernel.org, Ryan Case <ryandcase@chromium.org>
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-20181126]
[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/20181127-102810
base: https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/tty.git tty-testing
:::::: branch date: 6 hours ago
:::::: commit date: 6 hours ago
>> drivers/tty/serial/qcom_geni_serial.c:742:5-10: WARNING: Unsigned expression compared with zero: avail < 0
# https://github.com/0day-ci/linux/commit/407559b41ed61fd8c95ebe39539677bc577c7c66
git remote add linux-review https://github.com/0day-ci/linux
git remote update linux-review
git checkout 407559b41ed61fd8c95ebe39539677bc577c7c66
vim +742 drivers/tty/serial/qcom_geni_serial.c
c4f528795 Karthikeyan Ramasubramanian 2018-03-14 712
407559b41 Ryan Case 2018-11-26 713 static void qcom_geni_serial_handle_tx(struct uart_port *uport, bool done,
407559b41 Ryan Case 2018-11-26 714 bool active)
c4f528795 Karthikeyan Ramasubramanian 2018-03-14 715 {
c4f528795 Karthikeyan Ramasubramanian 2018-03-14 716 struct qcom_geni_serial_port *port = to_dev_port(uport, uport);
c4f528795 Karthikeyan Ramasubramanian 2018-03-14 717 struct circ_buf *xmit = &uport->state->xmit;
c4f528795 Karthikeyan Ramasubramanian 2018-03-14 718 size_t avail;
c4f528795 Karthikeyan Ramasubramanian 2018-03-14 719 size_t remaining;
407559b41 Ryan Case 2018-11-26 720 size_t pending;
c4f528795 Karthikeyan Ramasubramanian 2018-03-14 721 int i;
c4f528795 Karthikeyan Ramasubramanian 2018-03-14 722 u32 status;
c4f528795 Karthikeyan Ramasubramanian 2018-03-14 723 unsigned int chunk;
c4f528795 Karthikeyan Ramasubramanian 2018-03-14 724 int tail;
c4f528795 Karthikeyan Ramasubramanian 2018-03-14 725
c4f528795 Karthikeyan Ramasubramanian 2018-03-14 726 status = readl_relaxed(uport->membase + SE_GENI_TX_FIFO_STATUS);
407559b41 Ryan Case 2018-11-26 727
407559b41 Ryan Case 2018-11-26 728 /* Complete the current tx command before taking newly added data */
407559b41 Ryan Case 2018-11-26 729 if (active)
407559b41 Ryan Case 2018-11-26 730 pending = port->cur_tx_remaining;
407559b41 Ryan Case 2018-11-26 731 else
407559b41 Ryan Case 2018-11-26 732 pending = uart_circ_chars_pending(xmit);
407559b41 Ryan Case 2018-11-26 733
407559b41 Ryan Case 2018-11-26 734 /* All data has been transmitted and acknowledged as received */
407559b41 Ryan Case 2018-11-26 735 if (!pending && !status && done) {
c4f528795 Karthikeyan Ramasubramanian 2018-03-14 736 qcom_geni_serial_stop_tx(uport);
c4f528795 Karthikeyan Ramasubramanian 2018-03-14 737 goto out_write_wakeup;
c4f528795 Karthikeyan Ramasubramanian 2018-03-14 738 }
c4f528795 Karthikeyan Ramasubramanian 2018-03-14 739
407559b41 Ryan Case 2018-11-26 740 avail = port->tx_fifo_depth - (status & TX_FIFO_WC);
407559b41 Ryan Case 2018-11-26 741 avail *= port->tx_bytes_pw;
407559b41 Ryan Case 2018-11-26 @742 if (avail < 0)
407559b41 Ryan Case 2018-11-26 743 avail = 0;
8a8a66a1a Girish Mahadevan 2018-07-13 744
638a6f4eb Evan Green 2018-05-09 745 tail = xmit->tail;
407559b41 Ryan Case 2018-11-26 746 chunk = min3((size_t)pending, (size_t)(UART_XMIT_SIZE - tail), avail);
c4f528795 Karthikeyan Ramasubramanian 2018-03-14 747 if (!chunk)
c4f528795 Karthikeyan Ramasubramanian 2018-03-14 748 goto out_write_wakeup;
c4f528795 Karthikeyan Ramasubramanian 2018-03-14 749
407559b41 Ryan Case 2018-11-26 750 if (!port->cur_tx_remaining) {
407559b41 Ryan Case 2018-11-26 751 qcom_geni_serial_setup_tx(uport, pending);
407559b41 Ryan Case 2018-11-26 752 port->cur_tx_remaining = pending;
407559b41 Ryan Case 2018-11-26 753 }
c4f528795 Karthikeyan Ramasubramanian 2018-03-14 754
c4f528795 Karthikeyan Ramasubramanian 2018-03-14 755 remaining = chunk;
c4f528795 Karthikeyan Ramasubramanian 2018-03-14 756 for (i = 0; i < chunk; ) {
c4f528795 Karthikeyan Ramasubramanian 2018-03-14 757 unsigned int tx_bytes;
69736b57d Karthikeyan Ramasubramanian 2018-05-03 758 u8 buf[sizeof(u32)];
c4f528795 Karthikeyan Ramasubramanian 2018-03-14 759 int c;
c4f528795 Karthikeyan Ramasubramanian 2018-03-14 760
69736b57d Karthikeyan Ramasubramanian 2018-05-03 761 memset(buf, 0, ARRAY_SIZE(buf));
6a10635e9 Karthikeyan Ramasubramanian 2018-05-03 762 tx_bytes = min_t(size_t, remaining, port->tx_bytes_pw);
c4f528795 Karthikeyan Ramasubramanian 2018-03-14 763 for (c = 0; c < tx_bytes ; c++)
69736b57d Karthikeyan Ramasubramanian 2018-05-03 764 buf[c] = xmit->buf[tail + c];
c4f528795 Karthikeyan Ramasubramanian 2018-03-14 765
69736b57d Karthikeyan Ramasubramanian 2018-05-03 766 iowrite32_rep(uport->membase + SE_GENI_TX_FIFOn, buf, 1);
c4f528795 Karthikeyan Ramasubramanian 2018-03-14 767
c4f528795 Karthikeyan Ramasubramanian 2018-03-14 768 i += tx_bytes;
638a6f4eb Evan Green 2018-05-09 769 tail += tx_bytes;
c4f528795 Karthikeyan Ramasubramanian 2018-03-14 770 uport->icount.tx += tx_bytes;
c4f528795 Karthikeyan Ramasubramanian 2018-03-14 771 remaining -= tx_bytes;
407559b41 Ryan Case 2018-11-26 772 port->cur_tx_remaining -= tx_bytes;
c4f528795 Karthikeyan Ramasubramanian 2018-03-14 773 }
638a6f4eb Evan Green 2018-05-09 774
638a6f4eb Evan Green 2018-05-09 775 xmit->tail = tail & (UART_XMIT_SIZE - 1);
c4f528795 Karthikeyan Ramasubramanian 2018-03-14 776 out_write_wakeup:
638a6f4eb Evan Green 2018-05-09 777 if (uart_circ_chars_pending(xmit) < WAKEUP_CHARS)
c4f528795 Karthikeyan Ramasubramanian 2018-03-14 778 uart_write_wakeup(uport);
c4f528795 Karthikeyan Ramasubramanian 2018-03-14 779 }
c4f528795 Karthikeyan Ramasubramanian 2018-03-14 780
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
^ permalink raw reply [flat|nested] 7+ messages in thread* [PATCH] tty: serial: qcom_geni_serial: Fix softlock
@ 2018-11-27 2:25 Ryan Case
2018-11-28 0:20 ` Stephen Boyd
0 siblings, 1 reply; 7+ messages in thread
From: Ryan Case @ 2018-11-27 2:25 UTC (permalink / raw)
To: Greg Kroah-Hartman, Jiri Slaby
Cc: Evan Green, Doug Anderson, linux-kernel, linux-serial, 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>
Version: 1
---
drivers/tty/serial/qcom_geni_serial.c | 58 ++++++++++++++++++---------
1 file changed, 40 insertions(+), 18 deletions(-)
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;
};
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;
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,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
+ * 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->cur_tx_remaining)
+ qcom_geni_serial_setup_tx(uport, port->cur_tx_remaining);
+
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) {
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;
- 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);
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; ) {
@@ -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->cur_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
^ permalink raw reply related [flat|nested] 7+ messages in thread* Re: [PATCH] tty: serial: qcom_geni_serial: Fix softlock
2018-11-27 2:25 Ryan Case
@ 2018-11-28 0:20 ` Stephen Boyd
2018-11-28 1:24 ` Ryan Case
0 siblings, 1 reply; 7+ messages in thread
From: Stephen Boyd @ 2018-11-28 0:20 UTC (permalink / raw)
To: Greg Kroah-Hartman, Jiri Slaby, Ryan Case
Cc: Evan Green, Doug Anderson, linux-kernel, linux-serial, Ryan Case
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@chromium.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;
>
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH] tty: serial: qcom_geni_serial: Fix softlock
2018-11-28 0:20 ` Stephen Boyd
@ 2018-11-28 1:24 ` Ryan Case
2018-11-28 2:04 ` Stephen Boyd
0 siblings, 1 reply; 7+ messages in thread
From: Ryan Case @ 2018-11-28 1:24 UTC (permalink / raw)
To: Stephen Boyd
Cc: Greg Kroah-Hartman, Jiri Slaby, Evan Green, Doug Anderson,
linux-kernel, linux-serial
On Tue, Nov 27, 2018 at 4:20 PM Stephen Boyd <swboyd@chromium.org> wrote:
>
> 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@chromium.org>
> > Version: 1
>
> I've never seen a Version tag before. Did you manually add this?
I submitted with patman, this should have been Series-version:
>
> > 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?
Sure, and unsigned int is fine.
>
> > };
> >
> > 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.
will do.
>
> >
> > 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"?
Nope, comment is correct as is.
>
> > + * 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.
will do.
>
> >
> > __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?
Yup, happens quite a bit.
>
> > +
> > 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'?
It could be renamed but since this isn't a general file cleanup patch
I was avoiding non-functional changes. It is the TX_FIFO_STATUS
register, if non-zero there is still data in the FIFO or related
activity ongoing.
>
> > 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.
I hope underflow is impossible as well. However, if the hardware did
wind up in a strange state I wanted to err on the side of not throwing
away data and being able to resume later if things recovered. I can
remove the defensive checks if that's the custom, otherwise I'll
update the comparison logic accordingly.
>
> >
> > - 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.
Seems they can go away without any changes.
>
> > 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;
> >
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH] tty: serial: qcom_geni_serial: Fix softlock
2018-11-28 1:24 ` Ryan Case
@ 2018-11-28 2:04 ` Stephen Boyd
2018-11-28 2:37 ` Ryan Case
0 siblings, 1 reply; 7+ messages in thread
From: Stephen Boyd @ 2018-11-28 2:04 UTC (permalink / raw)
To: Ryan Case
Cc: Greg Kroah-Hartman, Jiri Slaby, Evan Green, Doug Anderson,
linux-kernel, linux-serial
Quoting Ryan Case (2018-11-27 17:24:44)
> On Tue, Nov 27, 2018 at 4:20 PM Stephen Boyd <swboyd@chromium.org> wrote:
> >
> > 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@chromium.org>
> > > Version: 1
> >
> > I've never seen a Version tag before. Did you manually add this?
>
> I submitted with patman, this should have been Series-version:
Hmm ok. I'm not aware of this being a kernel idiom so I would remove
this tag before sending.
>
> >
> > >
> > > 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"?
>
> Nope, comment is correct as is.
Ok. I fail at parsing it then. Perhaps
"It seems we can interrupt existing transfers except for when all data
has been sent"
would make it easier for me to read.
>
> >
> > >
> > > __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?
>
> Yup, happens quite a bit.
So its being used in both modes 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'?
>
> It could be renamed but since this isn't a general file cleanup patch
> I was avoiding non-functional changes. It is the TX_FIFO_STATUS
> register, if non-zero there is still data in the FIFO or related
> activity ongoing.
Ok.
>
> >
> > > 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.
>
> I hope underflow is impossible as well. However, if the hardware did
> wind up in a strange state I wanted to err on the side of not throwing
> away data and being able to resume later if things recovered. I can
> remove the defensive checks if that's the custom, otherwise I'll
> update the comparison logic accordingly.
Well it looks like impossible code because an unsigned value can't be
less than zero. So it's not about customs, more about dead code removal.
>
> >
> > >
> > > - 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.
>
> Seems they can go away without any changes.
Ok!
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH] tty: serial: qcom_geni_serial: Fix softlock
2018-11-28 2:04 ` Stephen Boyd
@ 2018-11-28 2:37 ` Ryan Case
2018-11-28 16:32 ` Stephen Boyd
0 siblings, 1 reply; 7+ messages in thread
From: Ryan Case @ 2018-11-28 2:37 UTC (permalink / raw)
To: Stephen Boyd
Cc: Greg Kroah-Hartman, Jiri Slaby, Evan Green, Doug Anderson,
linux-kernel, linux-serial
On Tue, Nov 27, 2018 at 6:04 PM Stephen Boyd <swboyd@chromium.org> wrote:
>
> Quoting Ryan Case (2018-11-27 17:24:44)
> > On Tue, Nov 27, 2018 at 4:20 PM Stephen Boyd <swboyd@chromium.org> wrote:
> > >
> > > 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@chromium.org>
> > > > Version: 1
> > >
> > > I've never seen a Version tag before. Did you manually add this?
> >
> > I submitted with patman, this should have been Series-version:
>
> Hmm ok. I'm not aware of this being a kernel idiom so I would remove
> this tag before sending.
Yup. Series-version: would be properly parsed out and adds the
v1/v2/etc... tags so this won't show up in the v2.
>
> >
> > >
> > > >
> > > > 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"?
> >
> > Nope, comment is correct as is.
>
> Ok. I fail at parsing it then. Perhaps
>
> "It seems we can interrupt existing transfers except for when all data
> has been sent"
>
> would make it easier for me to read.
>
> >
> > >
> > > >
> > > > __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?
> >
> > Yup, happens quite a bit.
>
> So its being used in both modes at the same time?
Yes.
>
> >
> > >
> > > > +
> > > > 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'?
> >
> > It could be renamed but since this isn't a general file cleanup patch
> > I was avoiding non-functional changes. It is the TX_FIFO_STATUS
> > register, if non-zero there is still data in the FIFO or related
> > activity ongoing.
>
> Ok.
>
> >
> > >
> > > > 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.
> >
> > I hope underflow is impossible as well. However, if the hardware did
> > wind up in a strange state I wanted to err on the side of not throwing
> > away data and being able to resume later if things recovered. I can
> > remove the defensive checks if that's the custom, otherwise I'll
> > update the comparison logic accordingly.
>
> Well it looks like impossible code because an unsigned value can't be
> less than zero. So it's not about customs, more about dead code removal.
Agreed on the current dead code aspect which is why I offered the
option of updating the comparison logic, but I can just delete it if
you want.
>
> >
> > >
> > > >
> > > > - 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.
> >
> > Seems they can go away without any changes.
>
> Ok!
>
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH] tty: serial: qcom_geni_serial: Fix softlock
2018-11-28 2:37 ` Ryan Case
@ 2018-11-28 16:32 ` Stephen Boyd
0 siblings, 0 replies; 7+ messages in thread
From: Stephen Boyd @ 2018-11-28 16:32 UTC (permalink / raw)
To: Ryan Case
Cc: Greg Kroah-Hartman, Jiri Slaby, Evan Green, Doug Anderson,
linux-kernel, linux-serial
Quoting Ryan Case (2018-11-27 18:37:15)
> On Tue, Nov 27, 2018 at 6:04 PM Stephen Boyd <swboyd@chromium.org> wrote:
> >
> > Quoting Ryan Case (2018-11-27 17:24:44)
> >
> > > I hope underflow is impossible as well. However, if the hardware did
> > > wind up in a strange state I wanted to err on the side of not throwing
> > > away data and being able to resume later if things recovered. I can
> > > remove the defensive checks if that's the custom, otherwise I'll
> > > update the comparison logic accordingly.
> >
> > Well it looks like impossible code because an unsigned value can't be
> > less than zero. So it's not about customs, more about dead code removal.
>
> Agreed on the current dead code aspect which is why I offered the
> option of updating the comparison logic, but I can just delete it if
> you want.
Ok, got it.
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2018-11-28 16:32 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-11-27 8:48 [PATCH] tty: serial: qcom_geni_serial: Fix softlock Julia Lawall
-- strict thread matches above, loose matches on Subject: below --
2018-11-27 2:25 Ryan Case
2018-11-28 0:20 ` Stephen Boyd
2018-11-28 1:24 ` Ryan Case
2018-11-28 2:04 ` Stephen Boyd
2018-11-28 2:37 ` Ryan Case
2018-11-28 16:32 ` Stephen Boyd
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).