Re: [PATCH] tty: serial: qcom_geni_serial: Fix softlock
Quoting Ryan Case (2018-11-27 18:37:15) > On Tue, Nov 27, 2018 at 6:04 PM Stephen Boyd 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.
Re: [PATCH] tty: serial: qcom_geni_serial: Fix softlock
Quoting Ryan Case (2018-11-27 18:37:15) > On Tue, Nov 27, 2018 at 6:04 PM Stephen Boyd 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.
Re: [PATCH] tty: serial: qcom_geni_serial: Fix softlock
On Tue, Nov 27, 2018 at 6:04 PM Stephen Boyd wrote:
>
> Quoting Ryan Case (2018-11-27 17:24:44)
> > On Tue, Nov 27, 2018 at 4:20 PM Stephen Boyd 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
> > > > 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(>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 = >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);
> > >
Re: [PATCH] tty: serial: qcom_geni_serial: Fix softlock
On Tue, Nov 27, 2018 at 6:04 PM Stephen Boyd wrote:
>
> Quoting Ryan Case (2018-11-27 17:24:44)
> > On Tue, Nov 27, 2018 at 4:20 PM Stephen Boyd 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
> > > > 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(>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 = >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);
> > >
Re: [PATCH] tty: serial: qcom_geni_serial: Fix softlock
Quoting Ryan Case (2018-11-27 17:24:44)
> On Tue, Nov 27, 2018 at 4:20 PM Stephen Boyd 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
> > > 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(>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 = >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
Re: [PATCH] tty: serial: qcom_geni_serial: Fix softlock
Quoting Ryan Case (2018-11-27 17:24:44)
> On Tue, Nov 27, 2018 at 4:20 PM Stephen Boyd 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
> > > 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(>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 = >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
Re: [PATCH] tty: serial: qcom_geni_serial: Fix softlock
On Tue, Nov 27, 2018 at 4:20 PM Stephen Boyd 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
> > 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(>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 = >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 +
Re: [PATCH] tty: serial: qcom_geni_serial: Fix softlock
On Tue, Nov 27, 2018 at 4:20 PM Stephen Boyd 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
> > 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(>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 = >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 +
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
> 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(>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 = >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 =
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
> 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(>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 = >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 =
Re: [PATCH] tty: serial: qcom_geni_serial: Fix softlock
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
To: kbu...@01.org
Cc: Julia Lawall
Subject: Re: [PATCH] tty: serial: qcom_geni_serial: Fix softlock
CC: kbuild-...@01.org
In-Reply-To: <20181127022536.104663-1-ryandc...@chromium.org>
References: <20181127022536.104663-1-ryandc...@chromium.org>
TO: Ryan Case
CC: Greg Kroah-Hartman , Jiri Slaby
, Evan Green , Doug Anderson
, linux-kernel@vger.kernel.org,
linux-ser...@vger.kernel.org, Ryan Case
CC: Evan Green , Doug Anderson ,
linux-kernel@vger.kernel.org, linux-ser...@vger.kernel.org, Ryan Case
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 =
>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 Mahadevan2018-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
Re: [PATCH] tty: serial: qcom_geni_serial: Fix softlock
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
To: kbu...@01.org
Cc: Julia Lawall
Subject: Re: [PATCH] tty: serial: qcom_geni_serial: Fix softlock
CC: kbuild-...@01.org
In-Reply-To: <20181127022536.104663-1-ryandc...@chromium.org>
References: <20181127022536.104663-1-ryandc...@chromium.org>
TO: Ryan Case
CC: Greg Kroah-Hartman , Jiri Slaby
, Evan Green , Doug Anderson
, linux-kernel@vger.kernel.org,
linux-ser...@vger.kernel.org, Ryan Case
CC: Evan Green , Doug Anderson ,
linux-kernel@vger.kernel.org, linux-ser...@vger.kernel.org, Ryan Case
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 =
>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 Mahadevan2018-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
