Skip to contentSkip to conversation

serial: implement flow control for ASPEED VUART driver

PATCH New Jeremy Kerr 4 participants 27 messages Mar 21, 2018 – Mar 27, 2018

Subsystems (3)
TTY layer and serial drivers Arm/Aspeed machine support 8250/16?50 (and clone uarts) serial driver
lore
Share & tools
lore

Jeremy Kerr

author
#0 lore raw

cover serial: implement flow control for ASPEED VUART driver

This series implements flow control for the ASPEED VUART driver. This hardware is slightly unusual in that the RX data rate can quickly overwhelm the flip buffer code, so the ldisc-driven throttle/unthrottle mechanisms don't entirely solve the problem.

To do this, we have a couple of minor changes to the tty core, as well as an update to the tty proc interface to display buffer overrun metrics (entirely optional, but does allow us to see the problem).

Then, we implement the standard throttle mechanism, and augment it with a fast-path to throttle if we overrun the flip buffers before the ldisc has had a chance to run.

Questions and comments most welcome; I'm fairly new to the tty layer.

Cheers,

Jeremy

---

Jeremy Kerr (5):
serial: Introduce UPSTAT_SYNC_FIFO for synchronised FIFOs
serial: expose buf_overrun count through proc interface serial/8250: export serial8250_read_char serial/aspeed-vuart: Implement rx throttling serial/aspeed-vuart: Implement quick throttle mechanism

 drivers/tty/serial/8250/8250_aspeed_vuart.c | 122 ++++++++++++++++++++++++++++
 drivers/tty/serial/8250/8250_port.c         |   3 +-
 drivers/tty/serial/serial_core.c            |   6 +-
 include/linux/serial_8250.h                 |   1 +
 include/linux/serial_core.h                 |   1 +
 5 files changed, 130 insertions(+), 3 deletions(-)
Signature · 1 line
2.14.1
author
#1 lore raw

PATCH 1/5 serial: Introduce UPSTAT_SYNC_FIFO for synchronised FIFOs

This change adds a flag to indicate that a UART is has an external means of synchronising its FIFO, without needing CTSRTS or XON/XOFF.

This allows us to use the throttle/unthrottle callbacks, without having to claim other methods of flow control.

Signed-off-by Jeremy Kerr <[email protected]>
---
 drivers/tty/serial/serial_core.c | 4 ++--
 include/linux/serial_core.h      | 1 +
 2 files changed, 3 insertions(+), 2 deletions(-)
drivers/tty/serial/serial_core.c +2 -2
index f534a40aebde..8f3dfc8b5307 100644
@@ -678,7 +678,7 @@ static void uart_throttle(struct tty_struct *tty)
 	if (C_CRTSCTS(tty))
 		mask |= UPSTAT_AUTORTS;
 
-	if (port->status & mask) {
+	if (port->status & (mask | UPSTAT_SYNC_FIFO)) {
 		port->ops->throttle(port);
 		mask &= ~port->status;
 	}

@@ -707,7 +707,7 @@ static void uart_unthrottle(struct tty_struct *tty)
 	if (C_CRTSCTS(tty))
 		mask |= UPSTAT_AUTORTS;
 
-	if (port->status & mask) {
+	if (port->status & (mask | UPSTAT_SYNC_FIFO)) {
 		port->ops->unthrottle(port);
 		mask &= ~port->status;
 	}
include/linux/serial_core.h +1 -0
index 1775500294bb..bf600ae0290d 100644
@@ -232,6 +232,7 @@ struct uart_port {
 #define UPSTAT_AUTORTS		((__force upstat_t) (1 << 2))
 #define UPSTAT_AUTOCTS		((__force upstat_t) (1 << 3))
 #define UPSTAT_AUTOXOFF		((__force upstat_t) (1 << 4))
+#define UPSTAT_SYNC_FIFO	((__force upstat_t) (1 << 5))
 
 	int			hw_stopped;		/* sw-assisted CTS flow state */
 	unsigned int		mctrl;			/* current modem ctrl settings */
Signature · 1 line
2.14.1
maintainer
#2 lore raw

PATCH 1/5 serial: Introduce UPSTAT_SYNC_FIFO for synchronised FIFOs

Quoted 17 lines · Jeremy Kerr

This change adds a flag to indicate that a UART is has an external means of synchronising its FIFO, without needing CTSRTS or XON/XOFF.

This allows us to use the throttle/unthrottle callbacks, without having to claim other methods of flow control.

Signed-off-by Jeremy Kerr <[email protected]>
---
 drivers/tty/serial/serial_core.c | 4 ++--
 include/linux/serial_core.h      | 1 +
 2 files changed, 3 insertions(+), 2 deletions(-)
drivers/tty/serial/serial_core.c +0 -0
index f534a40aebde..8f3dfc8b5307 100644

drivers/tty/serial/serial_core.c static void uart_throttle(struct tty_struct *tty) · line 684

Quoted 8 lines
drivers/tty/serial/serial_core.c +1 -1
@@ -678,7 +678,7 @@ static void uart_throttle(struct tty_struct *tty)
 	if (C_CRTSCTS(tty))
 		mask |= UPSTAT_AUTORTS;
 
-	if (port->status & mask) {
+	if (port->status & (mask | UPSTAT_SYNC_FIFO)) {
 		port->ops->throttle(port);
 		mask &= ~port->status;
 	}

Why not just set mask to UPSTAT_SYNC_FIFO at the top of this function?

drivers/tty/serial/serial_core.c static void uart_unthrottle(struct tty_struct *tty) · line 713

Quoted 9 lines
drivers/tty/serial/serial_core.c +1 -1
@@ -707,7 +707,7 @@ static void uart_unthrottle(struct tty_struct *tty)
 	if (C_CRTSCTS(tty))
 		mask |= UPSTAT_AUTORTS;
 
-	if (port->status & mask) {
+	if (port->status & (mask | UPSTAT_SYNC_FIFO)) {
 		port->ops->unthrottle(port);
 		mask &= ~port->status;
 	}

Same here. Would make it a bit more obvious (at least to me...)

thanks,

greg k-h

author
#3 lore raw

PATCH 1/5 serial: Introduce UPSTAT_SYNC_FIFO for synchronised FIFOs

Hi Greg,

Quoted 6 lines
Quoted 6 lines
drivers/tty/serial/serial_core.c +0 -0
@@ -678,7 +678,7 @@ static void uart_throttle(struct tty_struct *tty)
 	if (C_CRTSCTS(tty))
 		mask |= UPSTAT_AUTORTS;
Quoted 7 lines

drivers/tty/serial/serial_core.c static void uart_throttle(struct tty_struct *tty) · line 684

Quoted 5 lines
drivers/tty/serial/serial_core.c +1 -1
-	if (port->status & mask) {
+	if (port->status & (mask | UPSTAT_SYNC_FIFO)) {
 		port->ops->throttle(port);
 		mask &= ~port->status;
 	}

Why not just set mask to UPSTAT_SYNC_FIFO at the top of this function?

That was a bit of a line call - my thinking was that SYNC_FIFO is a little different from the calculated bit values in mask, hence keeping it separate and adding it explicitly at the check

However, adding it to the declaration of mask is a little simpler, so I'll redo that in v2.

Cheers,

Jeremy

#4 lore raw

PATCH 1/5 serial: Introduce UPSTAT_SYNC_FIFO for synchronised FIFOs

In reply to Jeremy Kerr
Quoted 5 lines · Jeremy Kerr

This change adds a flag to indicate that a UART is has an external means of synchronising its FIFO, without needing CTSRTS or XON/XOFF.

This allows us to use the throttle/unthrottle callbacks, without having to claim other methods of flow control.

This series fixed extremely heavy cpu usage we were getting with a lot of traffic coming in on the console. System is usable again with console getting hammered.

Tested-by Eddie James <[email protected]>
Quoted 41 lines
Signed-off-by Jeremy Kerr <[email protected]>
---
  drivers/tty/serial/serial_core.c | 4 ++--
  include/linux/serial_core.h      | 1 +
  2 files changed, 3 insertions(+), 2 deletions(-)
drivers/tty/serial/serial_core.c +2 -2
index f534a40aebde..8f3dfc8b5307 100644
@@ -678,7 +678,7 @@ static void uart_throttle(struct tty_struct *tty)
  	if (C_CRTSCTS(tty))
  		mask |= UPSTAT_AUTORTS;
 
-	if (port->status & mask) {
+	if (port->status & (mask | UPSTAT_SYNC_FIFO)) {
  		port->ops->throttle(port);
  		mask &= ~port->status;
  	}

@@ -707,7 +707,7 @@ static void uart_unthrottle(struct tty_struct *tty)
  	if (C_CRTSCTS(tty))
  		mask |= UPSTAT_AUTORTS;
 
-	if (port->status & mask) {
+	if (port->status & (mask | UPSTAT_SYNC_FIFO)) {
  		port->ops->unthrottle(port);
  		mask &= ~port->status;
  	}
include/linux/serial_core.h +1 -0
index 1775500294bb..bf600ae0290d 100644
@@ -232,6 +232,7 @@ struct uart_port {
  #define UPSTAT_AUTORTS		((__force upstat_t) (1 << 2))
  #define UPSTAT_AUTOCTS		((__force upstat_t) (1 << 3))
  #define UPSTAT_AUTOXOFF		((__force upstat_t) (1 << 4))
+#define UPSTAT_SYNC_FIFO	((__force upstat_t) (1 << 5))
 
  	int			hw_stopped;		/* sw-assisted CTS flow state */
  	unsigned int		mctrl;			/* current modem ctrl settings */
author
#5 lore raw

PATCH 2/5 serial: expose buf_overrun count through proc interface

In reply to Jeremy Kerr

The buf_overrun count is only every written, and not exposed to userspace anywhere. This means that dropped characters due to flip buffer overruns are never visible to userspace.

The /proc/tty/driver/serial file exports a bunch of metrics (including hardware overruns) already, so add the buf_overrun (as "bo:") to this file.

Signed-off-by Jeremy Kerr <[email protected]>
---
 drivers/tty/serial/serial_core.c | 2 ++
 1 file changed, 2 insertions(+)
drivers/tty/serial/serial_core.c +2 -0
index 8f3dfc8b5307..fc677534b510 100644
@@ -1780,6 +1780,8 @@ static void uart_line_info(struct seq_file *m, struct uart_driver *drv, int i)
 			seq_printf(m, " brk:%d", uport->icount.brk);
 		if (uport->icount.overrun)
 			seq_printf(m, " oe:%d", uport->icount.overrun);
+		if (uport->icount.buf_overrun)
+			seq_printf(m, " bo:%d", uport->icount.buf_overrun);
 
 #define INFOBIT(bit, str) \
 	if (uport->mctrl & (bit)) \
Signature · 1 line
2.14.1
maintainer
#6 lore raw

PATCH 2/5 serial: expose buf_overrun count through proc interface

Quoted 18 lines · Jeremy Kerr

The buf_overrun count is only every written, and not exposed to userspace anywhere. This means that dropped characters due to flip buffer overruns are never visible to userspace.

The /proc/tty/driver/serial file exports a bunch of metrics (including hardware overruns) already, so add the buf_overrun (as "bo:") to this file.

Signed-off-by Jeremy Kerr <[email protected]>
---
 drivers/tty/serial/serial_core.c | 2 ++
 1 file changed, 2 insertions(+)
drivers/tty/serial/serial_core.c +0 -0
index 8f3dfc8b5307..fc677534b510 100644

drivers/tty/serial/serial_core.c static void uart_line_info(struct seq_file *m, struct uart_driver *drv, int i) · line 1784

Quoted 5 lines
drivers/tty/serial/serial_core.c +2 -0
@@ -1780,6 +1780,8 @@ static void uart_line_info(struct seq_file *m, struct uart_driver *drv, int i)
 			seq_printf(m, " brk:%d", uport->icount.brk);
 		if (uport->icount.overrun)
 			seq_printf(m, " oe:%d", uport->icount.overrun);
+		if (uport->icount.buf_overrun)
+			seq_printf(m, " bo:%d", uport->icount.buf_overrun);

Hopefully this doesn't break any tools that might want to parse this file :)

author
#7 lore raw

PATCH 2/5 serial: expose buf_overrun count through proc interface

Hi Greg,

Quoted 5 lines
Quoted 5 lines
drivers/tty/serial/serial_core.c +0 -0
index 8f3dfc8b5307..fc677534b510 100644
@@ -1780,6 +1780,8 @@ static void uart_line_info(struct seq_file *m, struct uart_driver *drv, int i)
Quoted 8 lines

drivers/tty/serial/serial_core.c static void uart_line_info(struct seq_file *m, struct uart_driver *drv, int i) · line 1784

Quoted 5 lines
drivers/tty/serial/serial_core.c +2 -0
 			seq_printf(m, " brk:%d", uport->icount.brk);
 		if (uport->icount.overrun)
 			seq_printf(m, " oe:%d", uport->icount.overrun);
+		if (uport->icount.buf_overrun)
+			seq_printf(m, " bo:%d", uport->icount.buf_overrun);

Hopefully this doesn't break any tools that might want to parse this file :)

Given that those existing fe/pe/brk/oe fields only appear when they're non-zero, existing parsing code needs to be tolerant of different sets of fields being present/absent. Hopefully that means that a new field shouldn't be too much of a surprise?

Otherwise, I'm happy to report the buf_overrun count elsewhere, if there's a more suitable place.

Cheers,

Jeremy

#8 lore raw

PATCH 2/5 serial: expose buf_overrun count through proc interface

In reply to Jeremy Kerr
Quoted 7 lines · Jeremy Kerr

The buf_overrun count is only every written, and not exposed to userspace anywhere. This means that dropped characters due to flip buffer overruns are never visible to userspace.

The /proc/tty/driver/serial file exports a bunch of metrics (including hardware overruns) already, so add the buf_overrun (as "bo:") to this file.

Tested-by Eddie James <[email protected]>
Quoted 19 lines
Signed-off-by Jeremy Kerr <[email protected]>
---
  drivers/tty/serial/serial_core.c | 2 ++
  1 file changed, 2 insertions(+)
drivers/tty/serial/serial_core.c +2 -0
index 8f3dfc8b5307..fc677534b510 100644
@@ -1780,6 +1780,8 @@ static void uart_line_info(struct seq_file *m, struct uart_driver *drv, int i)
  			seq_printf(m, " brk:%d", uport->icount.brk);
  		if (uport->icount.overrun)
  			seq_printf(m, " oe:%d", uport->icount.overrun);
+		if (uport->icount.buf_overrun)
+			seq_printf(m, " bo:%d", uport->icount.buf_overrun);
 
  #define INFOBIT(bit, str) \
  	if (uport->mctrl & (bit)) \
author
#9 lore raw

PATCH 3/5 serial/8250: export serial8250_read_char

In reply to Jeremy Kerr

Currently, we export serial8250_rx_chars, which does a whole bunch of reads from the 8250 data register, without any form of flow control between reads.

An upcoming change to the aspeed vuart driver implements more fine-grained flow control in the interrupt handler, requiring character-at-a-time control over the rx path.

This change exports serial8250_read_char to allow this.

Signed-off-by Jeremy Kerr <[email protected]>
---
 drivers/tty/serial/8250/8250_port.c | 3 ++-
 include/linux/serial_8250.h         | 1 +
 2 files changed, 3 insertions(+), 1 deletion(-)
drivers/tty/serial/8250/8250_port.c +2 -1
index a5fe0e66c607..ea435406936c 100644
@@ -1650,7 +1650,7 @@ static void serial8250_enable_ms(struct uart_port *port)
 	serial8250_rpm_put(up);
 }
 
-static void serial8250_read_char(struct uart_8250_port *up, unsigned char lsr)
+void serial8250_read_char(struct uart_8250_port *up, unsigned char lsr)
 {
 	struct uart_port *port = &up->port;
 	unsigned char ch;

@@ -1710,6 +1710,7 @@ static void serial8250_read_char(struct uart_8250_port *up, unsigned char lsr)
 
 	uart_insert_char(port, lsr, UART_LSR_OE, ch, flag);
 }
+EXPORT_SYMBOL_GPL(serial8250_read_char);
 
 /*
  * serial8250_rx_chars: processes according to the passed in LSR
include/linux/serial_8250.h +1 -0
index 61fbb440449c..4639a3608614 100644
@@ -162,6 +162,7 @@ extern void serial8250_do_set_mctrl(struct uart_port *port, unsigned int mctrl);
 extern int fsl8250_handle_irq(struct uart_port *port);
 int serial8250_handle_irq(struct uart_port *port, unsigned int iir);
 unsigned char serial8250_rx_chars(struct uart_8250_port *up, unsigned char lsr);
+void serial8250_read_char(struct uart_8250_port *up, unsigned char lsr);
 void serial8250_tx_chars(struct uart_8250_port *up);
 unsigned int serial8250_modem_status(struct uart_8250_port *up);
 void serial8250_init_port(struct uart_8250_port *up);
Signature · 1 line
2.14.1
#10 lore raw

PATCH 3/5 serial/8250: export serial8250_read_char

Quoted 9 lines · Jeremy Kerr

Currently, we export serial8250_rx_chars, which does a whole bunch of reads from the 8250 data register, without any form of flow control between reads.

An upcoming change to the aspeed vuart driver implements more fine-grained flow control in the interrupt handler, requiring character-at-a-time control over the rx path.

This change exports serial8250_read_char to allow this.

Tested-by Eddie James <[email protected]>
Quoted 40 lines
Signed-off-by Jeremy Kerr <[email protected]>
---
  drivers/tty/serial/8250/8250_port.c | 3 ++-
  include/linux/serial_8250.h         | 1 +
  2 files changed, 3 insertions(+), 1 deletion(-)
drivers/tty/serial/8250/8250_port.c +2 -1
index a5fe0e66c607..ea435406936c 100644
@@ -1650,7 +1650,7 @@ static void serial8250_enable_ms(struct uart_port *port)
  	serial8250_rpm_put(up);
  }
 
-static void serial8250_read_char(struct uart_8250_port *up, unsigned char lsr)
+void serial8250_read_char(struct uart_8250_port *up, unsigned char lsr)
  {
  	struct uart_port *port = &up->port;
  	unsigned char ch;

@@ -1710,6 +1710,7 @@ static void serial8250_read_char(struct uart_8250_port *up, unsigned char lsr)
 
  	uart_insert_char(port, lsr, UART_LSR_OE, ch, flag);
  }
+EXPORT_SYMBOL_GPL(serial8250_read_char);
 
  /*
   * serial8250_rx_chars: processes according to the passed in LSR
include/linux/serial_8250.h +1 -0
index 61fbb440449c..4639a3608614 100644
@@ -162,6 +162,7 @@ extern void serial8250_do_set_mctrl(struct uart_port *port, unsigned int mctrl);
  extern int fsl8250_handle_irq(struct uart_port *port);
  int serial8250_handle_irq(struct uart_port *port, unsigned int iir);
  unsigned char serial8250_rx_chars(struct uart_8250_port *up, unsigned char lsr);
+void serial8250_read_char(struct uart_8250_port *up, unsigned char lsr);
  void serial8250_tx_chars(struct uart_8250_port *up);
  unsigned int serial8250_modem_status(struct uart_8250_port *up);
  void serial8250_init_port(struct uart_8250_port *up);
author
#11 lore raw

PATCH 4/5 serial/aspeed-vuart: Implement rx throttling

In reply to Jeremy Kerr

The aspeed VUART runs at LPC bus frequency, rather than being restricted to a typical UART baud rate. This means that the VUART can receive a lot of data, which can overrun tty flip buffers, and/or cause a large amount of interrupt traffic.

This change implements the uart_port->throttle & unthrottle callbacks, implemented by disabling the receiver line status & received data available IRQs.

Signed-off-by Jeremy Kerr <[email protected]>
---
 drivers/tty/serial/8250/8250_aspeed_vuart.c | 27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)
drivers/tty/serial/8250/8250_aspeed_vuart.c +27 -0
index 33a801353114..50c082b4a1ac 100644
@@ -183,6 +183,30 @@ static void aspeed_vuart_shutdown(struct uart_port *uart_port)
 	serial8250_do_shutdown(uart_port);
 }
 
+static void aspeed_vuart_set_throttle(struct uart_port *port, bool throttle)
+{
+	unsigned char irqs = UART_IER_RLSI | UART_IER_RDI;
+	struct uart_8250_port *up = up_to_u8250p(port);
+	unsigned long flags;
+
+	spin_lock_irqsave(&port->lock, flags);
+	up->ier &= ~irqs;
+	if (!throttle)
+		up->ier |= irqs;
+	serial_out(up, UART_IER, up->ier);
+	spin_unlock_irqrestore(&port->lock, flags);
+}
+
+static void aspeed_vuart_throttle(struct uart_port *port)
+{
+	aspeed_vuart_set_throttle(port, true);
+}
+
+static void aspeed_vuart_unthrottle(struct uart_port *port)
+{
+	aspeed_vuart_set_throttle(port, false);
+}
+
 static int aspeed_vuart_probe(struct platform_device *pdev)
 {
 	struct uart_8250_port port;

@@ -212,6 +236,9 @@ static int aspeed_vuart_probe(struct platform_device *pdev)
 	port.port.mapsize = resource_size(res);
 	port.port.startup = aspeed_vuart_startup;
 	port.port.shutdown = aspeed_vuart_shutdown;
+	port.port.throttle = aspeed_vuart_throttle;
+	port.port.unthrottle = aspeed_vuart_unthrottle;
+	port.port.status = UPSTAT_SYNC_FIFO;
 	port.port.dev = &pdev->dev;
 
 	rc = sysfs_create_group(&vuart->dev->kobj, &aspeed_vuart_attr_group);
Signature · 1 line
2.14.1
#12 lore raw

PATCH 4/5 serial/aspeed-vuart: Implement rx throttling

Quoted 8 lines · Jeremy Kerr

The aspeed VUART runs at LPC bus frequency, rather than being restricted to a typical UART baud rate. This means that the VUART can receive a lot of data, which can overrun tty flip buffers, and/or cause a large amount of interrupt traffic.

This change implements the uart_port->throttle & unthrottle callbacks, implemented by disabling the receiver line status & received data available IRQs.

Tested-by Eddie James <[email protected]>
Quoted 51 lines
Signed-off-by Jeremy Kerr <[email protected]>
---
  drivers/tty/serial/8250/8250_aspeed_vuart.c | 27 +++++++++++++++++++++++++++
  1 file changed, 27 insertions(+)
drivers/tty/serial/8250/8250_aspeed_vuart.c +27 -0
index 33a801353114..50c082b4a1ac 100644
@@ -183,6 +183,30 @@ static void aspeed_vuart_shutdown(struct uart_port *uart_port)
  	serial8250_do_shutdown(uart_port);
  }
 
+static void aspeed_vuart_set_throttle(struct uart_port *port, bool throttle)
+{
+	unsigned char irqs = UART_IER_RLSI | UART_IER_RDI;
+	struct uart_8250_port *up = up_to_u8250p(port);
+	unsigned long flags;
+
+	spin_lock_irqsave(&port->lock, flags);
+	up->ier &= ~irqs;
+	if (!throttle)
+		up->ier |= irqs;
+	serial_out(up, UART_IER, up->ier);
+	spin_unlock_irqrestore(&port->lock, flags);
+}
+
+static void aspeed_vuart_throttle(struct uart_port *port)
+{
+	aspeed_vuart_set_throttle(port, true);
+}
+
+static void aspeed_vuart_unthrottle(struct uart_port *port)
+{
+	aspeed_vuart_set_throttle(port, false);
+}
+
  static int aspeed_vuart_probe(struct platform_device *pdev)
  {
  	struct uart_8250_port port;

@@ -212,6 +236,9 @@ static int aspeed_vuart_probe(struct platform_device *pdev)
  	port.port.mapsize = resource_size(res);
  	port.port.startup = aspeed_vuart_startup;
  	port.port.shutdown = aspeed_vuart_shutdown;
+	port.port.throttle = aspeed_vuart_throttle;
+	port.port.unthrottle = aspeed_vuart_unthrottle;
+	port.port.status = UPSTAT_SYNC_FIFO;
  	port.port.dev = &pdev->dev;
 
  	rc = sysfs_create_group(&vuart->dev->kobj, &aspeed_vuart_attr_group);
author
#13 lore raw

PATCH 5/5 serial/aspeed-vuart: Implement quick throttle mechanism

In reply to Jeremy Kerr

Although we populate the ->throttle and ->unthrottle UART operations, these may not be called until the ldisc has had a chance to schedule and check buffer space. This means that we may overflow the flip buffers without ever hitting the ldisc's throttle threshold.

This change implements an interrupt-based throttle, where we check for space in the flip buffers before reading characters from the UART's FIFO. If there's no space available, we disable the RX interrupt and schedule a timer to check for space later.

This prevents a case where we drop characters under heavy RX load.

Signed-off-by Jeremy Kerr <[email protected]>
---
 drivers/tty/serial/8250/8250_aspeed_vuart.c | 103 ++++++++++++++++++++++++++--
 1 file changed, 99 insertions(+), 4 deletions(-)
drivers/tty/serial/8250/8250_aspeed_vuart.c +99 -4
index 50c082b4a1ac..bc2b63578e58 100644
@@ -14,6 +14,8 @@
 #include <linux/of_address.h>
 #include <linux/of_irq.h>
 #include <linux/of_platform.h>
+#include <linux/tty.h>
+#include <linux/tty_flip.h>
 #include <linux/clk.h>
 
 #include "8250.h"

@@ -32,8 +34,16 @@ struct aspeed_vuart {
 	void __iomem		*regs;
 	struct clk		*clk;
 	int			line;
+	struct timer_list	unthrottle_timer;
 };
 
+/*
+ * If we fill the tty flip buffers, we throttle the data ready interrupt
+ * to prevent dropped characters. This timeout defines how long we wait
+ * to (conditionally, depending on buffer state) unthrottle.
+ */
+static const int unthrottle_timeout = HZ/10;
+
 /*
  * The VUART is basically two UART 'front ends' connected by their FIFO
  * (no actual serial line in between). One is on the BMC side (management

@@ -183,17 +193,23 @@ static void aspeed_vuart_shutdown(struct uart_port *uart_port)
 	serial8250_do_shutdown(uart_port);
 }
 
-static void aspeed_vuart_set_throttle(struct uart_port *port, bool throttle)
+static void __aspeed_vuart_set_throttle(struct uart_8250_port *up,
+		bool throttle)
 {
 	unsigned char irqs = UART_IER_RLSI | UART_IER_RDI;
-	struct uart_8250_port *up = up_to_u8250p(port);
-	unsigned long flags;
 
-	spin_lock_irqsave(&port->lock, flags);
 	up->ier &= ~irqs;
 	if (!throttle)
 		up->ier |= irqs;
 	serial_out(up, UART_IER, up->ier);
+}
+static void aspeed_vuart_set_throttle(struct uart_port *port, bool throttle)
+{
+	struct uart_8250_port *up = up_to_u8250p(port);
+	unsigned long flags;
+
+	spin_lock_irqsave(&port->lock, flags);
+	__aspeed_vuart_set_throttle(up, throttle);
 	spin_unlock_irqrestore(&port->lock, flags);
 }
 

@@ -207,6 +223,81 @@ static void aspeed_vuart_unthrottle(struct uart_port *port)
 	aspeed_vuart_set_throttle(port, false);
 }
 
+static void aspeed_vuart_unthrottle_exp(unsigned long data)
+{
+	struct uart_8250_port *up = (void *)data;
+	struct aspeed_vuart *vuart = up->port.private_data;
+
+	if (!tty_buffer_space_avail(&up->port.state->port)) {
+		mod_timer(&vuart->unthrottle_timer, unthrottle_timeout);
+		return;
+	}
+
+	aspeed_vuart_unthrottle(&up->port);
+}
+
+/*
+ * Custom interrupt handler to manage finer-grained flow control. Although we
+ * have throttle/unthrottle callbacks, we've seen that the VUART device can
+ * deliver characters faster than the ldisc has a chance to check buffer space
+ * against the throttle threshold. This results in dropped characters before
+ * the throttle.
+ *
+ * We do this by checking for flip buffer space before RX. If we have no space,
+ * throttle now and schedule an unthrottle for later, once the ldisc has had
+ * a chance to drain the buffers.
+ */
+static int aspeed_vuart_handle_irq(struct uart_port *port) { struct
+	uart_8250_port *up = up_to_u8250p(port); unsigned int iir, lsr;
+	unsigned long flags; int space, count;
+
+	iir = serial_port_in(port, UART_IIR);
+
+	if (iir & UART_IIR_NO_INT)
+		return 0;
+
+	spin_lock_irqsave(&port->lock, flags);
+
+	lsr = serial_port_in(port, UART_LSR);
+
+	if (lsr & (UART_LSR_DR | UART_LSR_BI)) {
+		space = tty_buffer_space_avail(&port->state->port);
+
+		if (!space) {
+			/* throttle and schedule an unthrottle later */
+			struct aspeed_vuart *vuart = port->private_data;
+			__aspeed_vuart_set_throttle(up, true);
+
+			if (!timer_pending(&vuart->unthrottle_timer)) {
+				vuart->unthrottle_timer.data =
+					(unsigned long)up;
+				mod_timer(&vuart->unthrottle_timer,
+						unthrottle_timeout);
+			}
+
+		} else {
+			count = min(space, 256);
+
+			do {
+				serial8250_read_char(up, lsr);
+				lsr = serial_in(up, UART_LSR);
+				if (--count == 0)
+					break;
+			} while (lsr & (UART_LSR_DR | UART_LSR_BI));
+
+			tty_flip_buffer_push(&port->state->port);
+		}
+	}
+
+	serial8250_modem_status(up);
+	if (lsr & UART_LSR_THRE)
+		serial8250_tx_chars(up);
+
+	spin_unlock_irqrestore(&port->lock, flags);
+
+	return 1;
+}
+
 static int aspeed_vuart_probe(struct platform_device *pdev)
 {
 	struct uart_8250_port port;

@@ -223,6 +314,8 @@ static int aspeed_vuart_probe(struct platform_device *pdev)
 		return -ENOMEM;
 
 	vuart->dev = &pdev->dev;
+	setup_timer(&vuart->unthrottle_timer,
+			aspeed_vuart_unthrottle_exp, (unsigned long)vuart);
 
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	vuart->regs = devm_ioremap_resource(&pdev->dev, res);

@@ -284,6 +377,7 @@ static int aspeed_vuart_probe(struct platform_device *pdev)
 
 	port.port.irq = irq_of_parse_and_map(np, 0);
 	port.port.irqflags = IRQF_SHARED;
+	port.port.handle_irq = aspeed_vuart_handle_irq;
 	port.port.iotype = UPIO_MEM;
 	port.port.type = PORT_16550A;
 	port.port.uartclk = clk;

@@ -323,6 +417,7 @@ static int aspeed_vuart_remove(struct platform_device *pdev)
 {
 	struct aspeed_vuart *vuart = platform_get_drvdata(pdev);
 
+	del_timer_sync(&vuart->unthrottle_timer);
 	aspeed_vuart_set_enabled(vuart, false);
 	serial8250_unregister_port(vuart->line);
 	sysfs_remove_group(&vuart->dev->kobj, &aspeed_vuart_attr_group);
Signature · 1 line
2.14.1
maintainer
#14 lore raw

PATCH 5/5 serial/aspeed-vuart: Implement quick throttle mechanism

Hi Jeremy,

Quoted 50 lines · Jeremy Kerr

Although we populate the ->throttle and ->unthrottle UART operations, these may not be called until the ldisc has had a chance to schedule and check buffer space. This means that we may overflow the flip buffers without ever hitting the ldisc's throttle threshold.

This change implements an interrupt-based throttle, where we check for space in the flip buffers before reading characters from the UART's FIFO. If there's no space available, we disable the RX interrupt and schedule a timer to check for space later.

This prevents a case where we drop characters under heavy RX load.

Signed-off-by Jeremy Kerr <[email protected]>
---
 drivers/tty/serial/8250/8250_aspeed_vuart.c | 103 ++++++++++++++++++++++++++--
 1 file changed, 99 insertions(+), 4 deletions(-)
drivers/tty/serial/8250/8250_aspeed_vuart.c +10 -0
index 50c082b4a1ac..bc2b63578e58 100644
@@ -14,6 +14,8 @@
 #include <linux/of_address.h>
 #include <linux/of_irq.h>
 #include <linux/of_platform.h>
+#include <linux/tty.h>
+#include <linux/tty_flip.h>
 #include <linux/clk.h>
 
 #include "8250.h"

@@ -32,8 +34,16 @@ struct aspeed_vuart {
        void __iomem            *regs;
        struct clk              *clk;
        int                     line;
+       struct timer_list       unthrottle_timer;
 };
 
+/*
+ * If we fill the tty flip buffers, we throttle the data ready interrupt
+ * to prevent dropped characters. This timeout defines how long we wait
+ * to (conditionally, depending on buffer state) unthrottle.
+ */
+static const int unthrottle_timeout = HZ/10;
+
 /*
  * The VUART is basically two UART 'front ends' connected by their FIFO
  * (no actual serial line in between). One is on the BMC side (management

@@ -183,17 +193,23 @@ static void aspeed_vuart_shutdown(struct uart_port *uart_port)
        serial8250_do_shutdown(uart_port);
 }

drivers/tty/serial/8250/8250_aspeed_vuart.c static void aspeed_vuart_shutdown(struct uart_port *uart_port) · line 192

Quoted 10 lines
drivers/tty/serial/8250/8250_aspeed_vuart.c +2 -4
 
-static void aspeed_vuart_set_throttle(struct uart_port *port, bool throttle)
+static void __aspeed_vuart_set_throttle(struct uart_8250_port *up,
+               bool throttle)
 {
        unsigned char irqs = UART_IER_RLSI | UART_IER_RDI;
-       struct uart_8250_port *up = up_to_u8250p(port);
-       unsigned long flags;
 
-       spin_lock_irqsave(&port->lock, flags);

The diff is a bit confusing, considering you just added these functions. Can you explain why you did it that way?

Cheers,

Joel

author
#15 lore raw

PATCH 5/5 serial/aspeed-vuart: Implement quick throttle mechanism

Hi Joel,

Quoted 2 lines

The diff is a bit confusing, considering you just added these functions. Can you explain why you did it that way?

Sure. The initial versions were the standard callbacks for struct uart_ops, which all need to take the port lock.

Patch 5/5 needs a lockless version of these for calling from the irq handler, which already has the lock held. So, we introduce the __-versions there.

I could add the lockless version in the earlier 4.5 patch, but it looks a bit redundant there. Happy to go either way. Or, I can elaborate why in the commit log (I'll need a v2 anyway for the setup_timer change...).

Cheers,

Jeremy

author
#16 lore raw

PATCH 5/5 serial/aspeed-vuart: Implement quick throttle mechanism

In reply to Jeremy Kerr

Hi all,

drivers/tty/serial/8250/8250_aspeed_vuart.c static void aspeed_vuart_unthrottle(struct uart_port *port) · line 252

Quoted 3 lines
drivers/tty/serial/8250/8250_aspeed_vuart.c +3 -0
+static int aspeed_vuart_handle_irq(struct uart_port *port) { struct
+	uart_8250_port *up = up_to_u8250p(port); unsigned int iir, lsr;
+	unsigned long flags; int space, count;

Yikes, wrapping is messed up here. v2 coming soon, with the proper timer API too.

Cheers,

Jeremy

author
#17 lore raw

PATCH v2 5/5 serial/aspeed-vuart: Implement quick throttle mechanism

In reply to Jeremy Kerr

Although we populate the ->throttle and ->unthrottle UART operations, these may not be called until the ldisc has had a chance to schedule and check buffer space. This means that we may overflow the flip buffers without ever hitting the ldisc's throttle threshold.

This change implements an interrupt-based throttle, where we check for space in the flip buffers before reading characters from the UART's FIFO. If there's no space available, we disable the RX interrupt and schedule a timer to check for space later.

For this, we need an unlocked version of the set_throttle function to be able to change throttle state from the irq_handler, which already holds port->lock.

This prevents a case where we drop characters under heavy RX load.

Signed-off-by Jeremy Kerr <[email protected]>

---
v2: Use updated timer API, fix text wrap

---
 drivers/tty/serial/8250/8250_aspeed_vuart.c | 105 ++++++++++++++++++++++++++--
 1 file changed, 101 insertions(+), 4 deletions(-)
drivers/tty/serial/8250/8250_aspeed_vuart.c +101 -4
index cd1bb49dadfe..023db3266757 100644
@@ -10,6 +10,8 @@
 #include <linux/of_address.h>
 #include <linux/of_irq.h>
 #include <linux/of_platform.h>
+#include <linux/tty.h>
+#include <linux/tty_flip.h>
 #include <linux/clk.h>
 
 #include "8250.h"

@@ -28,8 +30,17 @@ struct aspeed_vuart {
 	void __iomem		*regs;
 	struct clk		*clk;
 	int			line;
+	struct timer_list	unthrottle_timer;
+	struct uart_8250_port	*port;
 };
 
+/*
+ * If we fill the tty flip buffers, we throttle the data ready interrupt
+ * to prevent dropped characters. This timeout defines how long we wait
+ * to (conditionally, depending on buffer state) unthrottle.
+ */
+static const int unthrottle_timeout = HZ/10;
+
 /*
  * The VUART is basically two UART 'front ends' connected by their FIFO
  * (no actual serial line in between). One is on the BMC side (management

@@ -179,17 +190,23 @@ static void aspeed_vuart_shutdown(struct uart_port *uart_port)
 	serial8250_do_shutdown(uart_port);
 }
 
-static void aspeed_vuart_set_throttle(struct uart_port *port, bool throttle)
+static void __aspeed_vuart_set_throttle(struct uart_8250_port *up,
+		bool throttle)
 {
 	unsigned char irqs = UART_IER_RLSI | UART_IER_RDI;
-	struct uart_8250_port *up = up_to_u8250p(port);
-	unsigned long flags;
 
-	spin_lock_irqsave(&port->lock, flags);
 	up->ier &= ~irqs;
 	if (!throttle)
 		up->ier |= irqs;
 	serial_out(up, UART_IER, up->ier);
+}
+static void aspeed_vuart_set_throttle(struct uart_port *port, bool throttle)
+{
+	struct uart_8250_port *up = up_to_u8250p(port);
+	unsigned long flags;
+
+	spin_lock_irqsave(&port->lock, flags);
+	__aspeed_vuart_set_throttle(up, throttle);
 	spin_unlock_irqrestore(&port->lock, flags);
 }
 

@@ -203,6 +220,83 @@ static void aspeed_vuart_unthrottle(struct uart_port *port)
 	aspeed_vuart_set_throttle(port, false);
 }
 
+static void aspeed_vuart_unthrottle_exp(struct timer_list *timer)
+{
+	struct aspeed_vuart *vuart = from_timer(vuart, timer, unthrottle_timer);
+	struct uart_8250_port *up = vuart->port;
+
+	if (!tty_buffer_space_avail(&up->port.state->port)) {
+		mod_timer(&vuart->unthrottle_timer, unthrottle_timeout);
+		return;
+	}
+
+	aspeed_vuart_unthrottle(&up->port);
+}
+
+/*
+ * Custom interrupt handler to manage finer-grained flow control. Although we
+ * have throttle/unthrottle callbacks, we've seen that the VUART device can
+ * deliver characters faster than the ldisc has a chance to check buffer space
+ * against the throttle threshold. This results in dropped characters before
+ * the throttle.
+ *
+ * We do this by checking for flip buffer space before RX. If we have no space,
+ * throttle now and schedule an unthrottle for later, once the ldisc has had
+ * a chance to drain the buffers.
+ */
+static int aspeed_vuart_handle_irq(struct uart_port *port)
+{
+	struct uart_8250_port *up = up_to_u8250p(port);
+	unsigned int iir, lsr;
+	unsigned long flags;
+	int space, count;
+
+	iir = serial_port_in(port, UART_IIR);
+
+	if (iir & UART_IIR_NO_INT)
+		return 0;
+
+	spin_lock_irqsave(&port->lock, flags);
+
+	lsr = serial_port_in(port, UART_LSR);
+
+	if (lsr & (UART_LSR_DR | UART_LSR_BI)) {
+		space = tty_buffer_space_avail(&port->state->port);
+
+		if (!space) {
+			/* throttle and schedule an unthrottle later */
+			struct aspeed_vuart *vuart = port->private_data;
+			__aspeed_vuart_set_throttle(up, true);
+
+			if (!timer_pending(&vuart->unthrottle_timer)) {
+				vuart->port = up;
+				mod_timer(&vuart->unthrottle_timer,
+						unthrottle_timeout);
+			}
+
+		} else {
+			count = min(space, 256);
+
+			do {
+				serial8250_read_char(up, lsr);
+				lsr = serial_in(up, UART_LSR);
+				if (--count == 0)
+					break;
+			} while (lsr & (UART_LSR_DR | UART_LSR_BI));
+
+			tty_flip_buffer_push(&port->state->port);
+		}
+	}
+
+	serial8250_modem_status(up);
+	if (lsr & UART_LSR_THRE)
+		serial8250_tx_chars(up);
+
+	spin_unlock_irqrestore(&port->lock, flags);
+
+	return 1;
+}
+
 static int aspeed_vuart_probe(struct platform_device *pdev)
 {
 	struct uart_8250_port port;

@@ -219,6 +313,7 @@ static int aspeed_vuart_probe(struct platform_device *pdev)
 		return -ENOMEM;
 
 	vuart->dev = &pdev->dev;
+	timer_setup(&vuart->unthrottle_timer, aspeed_vuart_unthrottle_exp, 0);
 
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	vuart->regs = devm_ioremap_resource(&pdev->dev, res);

@@ -280,6 +375,7 @@ static int aspeed_vuart_probe(struct platform_device *pdev)
 
 	port.port.irq = irq_of_parse_and_map(np, 0);
 	port.port.irqflags = IRQF_SHARED;
+	port.port.handle_irq = aspeed_vuart_handle_irq;
 	port.port.iotype = UPIO_MEM;
 	port.port.type = PORT_16550A;
 	port.port.uartclk = clk;

@@ -319,6 +415,7 @@ static int aspeed_vuart_remove(struct platform_device *pdev)
 {
 	struct aspeed_vuart *vuart = platform_get_drvdata(pdev);
 
+	del_timer_sync(&vuart->unthrottle_timer);
 	aspeed_vuart_set_enabled(vuart, false);
 	serial8250_unregister_port(vuart->line);
 	sysfs_remove_group(&vuart->dev->kobj, &aspeed_vuart_attr_group);
Signature · 1 line
2.14.1
#18 lore raw

PATCH 5/5 serial/aspeed-vuart: Implement quick throttle mechanism

In reply to Jeremy Kerr
Quoted 11 lines · Jeremy Kerr

Although we populate the ->throttle and ->unthrottle UART operations, these may not be called until the ldisc has had a chance to schedule and check buffer space. This means that we may overflow the flip buffers without ever hitting the ldisc's throttle threshold.

This change implements an interrupt-based throttle, where we check for space in the flip buffers before reading characters from the UART's FIFO. If there's no space available, we disable the RX interrupt and schedule a timer to check for space later.

This prevents a case where we drop characters under heavy RX load.

Tested-by Eddie James <[email protected]>
Quoted 171 lines
Signed-off-by Jeremy Kerr <[email protected]>
---
  drivers/tty/serial/8250/8250_aspeed_vuart.c | 103 ++++++++++++++++++++++++++--
  1 file changed, 99 insertions(+), 4 deletions(-)
drivers/tty/serial/8250/8250_aspeed_vuart.c +99 -4
index 50c082b4a1ac..bc2b63578e58 100644
@@ -14,6 +14,8 @@
  #include <linux/of_address.h>
  #include <linux/of_irq.h>
  #include <linux/of_platform.h>
+#include <linux/tty.h>
+#include <linux/tty_flip.h>
  #include <linux/clk.h>
 
  #include "8250.h"

@@ -32,8 +34,16 @@ struct aspeed_vuart {
  	void __iomem		*regs;
  	struct clk		*clk;
  	int			line;
+	struct timer_list	unthrottle_timer;
  };
 
+/*
+ * If we fill the tty flip buffers, we throttle the data ready interrupt
+ * to prevent dropped characters. This timeout defines how long we wait
+ * to (conditionally, depending on buffer state) unthrottle.
+ */
+static const int unthrottle_timeout = HZ/10;
+
  /*
   * The VUART is basically two UART 'front ends' connected by their FIFO
   * (no actual serial line in between). One is on the BMC side (management

@@ -183,17 +193,23 @@ static void aspeed_vuart_shutdown(struct uart_port *uart_port)
  	serial8250_do_shutdown(uart_port);
  }
 
-static void aspeed_vuart_set_throttle(struct uart_port *port, bool throttle)
+static void __aspeed_vuart_set_throttle(struct uart_8250_port *up,
+		bool throttle)
  {
  	unsigned char irqs = UART_IER_RLSI | UART_IER_RDI;
-	struct uart_8250_port *up = up_to_u8250p(port);
-	unsigned long flags;
 
-	spin_lock_irqsave(&port->lock, flags);
  	up->ier &= ~irqs;
  	if (!throttle)
  		up->ier |= irqs;
  	serial_out(up, UART_IER, up->ier);
+}
+static void aspeed_vuart_set_throttle(struct uart_port *port, bool throttle)
+{
+	struct uart_8250_port *up = up_to_u8250p(port);
+	unsigned long flags;
+
+	spin_lock_irqsave(&port->lock, flags);
+	__aspeed_vuart_set_throttle(up, throttle);
  	spin_unlock_irqrestore(&port->lock, flags);
  }
 

@@ -207,6 +223,81 @@ static void aspeed_vuart_unthrottle(struct uart_port *port)
  	aspeed_vuart_set_throttle(port, false);
  }
 
+static void aspeed_vuart_unthrottle_exp(unsigned long data)
+{
+	struct uart_8250_port *up = (void *)data;
+	struct aspeed_vuart *vuart = up->port.private_data;
+
+	if (!tty_buffer_space_avail(&up->port.state->port)) {
+		mod_timer(&vuart->unthrottle_timer, unthrottle_timeout);
+		return;
+	}
+
+	aspeed_vuart_unthrottle(&up->port);
+}
+
+/*
+ * Custom interrupt handler to manage finer-grained flow control. Although we
+ * have throttle/unthrottle callbacks, we've seen that the VUART device can
+ * deliver characters faster than the ldisc has a chance to check buffer space
+ * against the throttle threshold. This results in dropped characters before
+ * the throttle.
+ *
+ * We do this by checking for flip buffer space before RX. If we have no space,
+ * throttle now and schedule an unthrottle for later, once the ldisc has had
+ * a chance to drain the buffers.
+ */
+static int aspeed_vuart_handle_irq(struct uart_port *port) { struct
+	uart_8250_port *up = up_to_u8250p(port); unsigned int iir, lsr;
+	unsigned long flags; int space, count;
+
+	iir = serial_port_in(port, UART_IIR);
+
+	if (iir & UART_IIR_NO_INT)
+		return 0;
+
+	spin_lock_irqsave(&port->lock, flags);
+
+	lsr = serial_port_in(port, UART_LSR);
+
+	if (lsr & (UART_LSR_DR | UART_LSR_BI)) {
+		space = tty_buffer_space_avail(&port->state->port);
+
+		if (!space) {
+			/* throttle and schedule an unthrottle later */
+			struct aspeed_vuart *vuart = port->private_data;
+			__aspeed_vuart_set_throttle(up, true);
+
+			if (!timer_pending(&vuart->unthrottle_timer)) {
+				vuart->unthrottle_timer.data =
+					(unsigned long)up;
+				mod_timer(&vuart->unthrottle_timer,
+						unthrottle_timeout);
+			}
+
+		} else {
+			count = min(space, 256);
+
+			do {
+				serial8250_read_char(up, lsr);
+				lsr = serial_in(up, UART_LSR);
+				if (--count == 0)
+					break;
+			} while (lsr & (UART_LSR_DR | UART_LSR_BI));
+
+			tty_flip_buffer_push(&port->state->port);
+		}
+	}
+
+	serial8250_modem_status(up);
+	if (lsr & UART_LSR_THRE)
+		serial8250_tx_chars(up);
+
+	spin_unlock_irqrestore(&port->lock, flags);
+
+	return 1;
+}
+
  static int aspeed_vuart_probe(struct platform_device *pdev)
  {
  	struct uart_8250_port port;

@@ -223,6 +314,8 @@ static int aspeed_vuart_probe(struct platform_device *pdev)
  		return -ENOMEM;
 
  	vuart->dev = &pdev->dev;
+	setup_timer(&vuart->unthrottle_timer,
+			aspeed_vuart_unthrottle_exp, (unsigned long)vuart);
 
  	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
  	vuart->regs = devm_ioremap_resource(&pdev->dev, res);

@@ -284,6 +377,7 @@ static int aspeed_vuart_probe(struct platform_device *pdev)
 
  	port.port.irq = irq_of_parse_and_map(np, 0);
  	port.port.irqflags = IRQF_SHARED;
+	port.port.handle_irq = aspeed_vuart_handle_irq;
  	port.port.iotype = UPIO_MEM;
  	port.port.type = PORT_16550A;
  	port.port.uartclk = clk;

@@ -323,6 +417,7 @@ static int aspeed_vuart_remove(struct platform_device *pdev)
  {
  	struct aspeed_vuart *vuart = platform_get_drvdata(pdev);
 
+	del_timer_sync(&vuart->unthrottle_timer);
  	aspeed_vuart_set_enabled(vuart, false);
  	serial8250_unregister_port(vuart->line);
  	sysfs_remove_group(&vuart->dev->kobj, &aspeed_vuart_attr_group);
maintainer
#19 lore raw

cover serial: implement flow control for ASPEED VUART driver

In reply to Jeremy Kerr
Quoted 9 lines · Jeremy Kerr

This series implements flow control for the ASPEED VUART driver. This hardware is slightly unusual in that the RX data rate can quickly overwhelm the flip buffer code, so the ldisc-driven throttle/unthrottle mechanisms don't entirely solve the problem.

To do this, we have a couple of minor changes to the tty core, as well as an update to the tty proc interface to display buffer overrun metrics (entirely optional, but does allow us to see the problem).

Quoted 5 lines

Then, we implement the standard throttle mechanism, and augment it with a fast-path to throttle if we overrun the flip buffers before the ldisc has had a chance to run.

Questions and comments most welcome; I'm fairly new to the tty layer.

I've applied patch 2 here, can you redo the series again after fixing up the first one and include the v2 of patch 5 so it's easier for me to figure out what to really apply here?

thanks,

greg k-h

author
#20 lore raw

cover serial: implement flow control for ASPEED VUART driver

Hi Greg,

Quoted 3 lines

I've applied patch 2 here, can you redo the series again after fixing up the first one and include the v2 of patch 5 so it's easier for me to figure out what to really apply here?

Sure thing. I'll base v2 on top of tty-next, so you'll end up with a 4-patch series, without the duplicate.

Patches coming.

Cheers,

Jeremy

author
#21 lore raw

v2 cover serial: implement flow control for ASPEED VUART driver

In reply to Greg Kroah-Hartman

This series implements flow control for the ASPEED VUART driver. This hardware is slightly unusual in that the RX data rate can quickly overwhelm the flip buffer code, so the ldisc-driven throttle/unthrottle mechanisms don't entirely solve the problem.

To do this, we have a couple of minor changes to the tty core to allow tty drivers to do more explicit flow control.

Then, we implement the standard throttle mechanism, and augment it with a fast-path to throttle if we overrun the flip buffers before the ldisc has had a chance to run.

Questions and comments most welcome; I'm fairly new to the tty layer.

Cheers,

Jeremy

--
v2:
- rebase onto tty-next, dropping applied proc patch
- 1/4 simplify status mask checks
- 4/4 Use updated timer API, fix text wrap

Jeremy Kerr (4):
serial: Introduce UPSTAT_SYNC_FIFO for synchronised FIFOs serial/8250: export serial8250_read_char serial/aspeed-vuart: Implement rx throttling serial/aspeed-vuart: Implement quick throttle mechanism

 drivers/tty/serial/8250/8250_aspeed_vuart.c | 124 ++++++++++++++++++++++++++++
 drivers/tty/serial/8250/8250_port.c         |   3 +-
 drivers/tty/serial/serial_core.c            |   4 +-
 include/linux/serial_8250.h                 |   1 +
 include/linux/serial_core.h                 |   1 +
 5 files changed, 130 insertions(+), 3 deletions(-)
Signature · 1 line
2.14.1
author
#22 lore raw

PATCH v2 1/4 serial: Introduce UPSTAT_SYNC_FIFO for synchronised FIFOs

This change adds a flag to indicate that a UART is has an external means of synchronising its FIFO, without needing CTSRTS or XON/XOFF.

This allows us to use the throttle/unthrottle callbacks, without having to claim other methods of flow control.

Signed-off-by Jeremy Kerr <[email protected]>
Tested-by Eddie James <[email protected]>

--
v2:
- simplify status mask checks

---
 drivers/tty/serial/serial_core.c | 4 ++--
 include/linux/serial_core.h      | 1 +
 2 files changed, 3 insertions(+), 2 deletions(-)
drivers/tty/serial/serial_core.c +2 -2
index 0466f9f08a91..c47158c93202 100644
@@ -674,8 +674,8 @@ static void uart_send_xchar(struct tty_struct *tty, char ch)
 static void uart_throttle(struct tty_struct *tty)
 {
 	struct uart_state *state = tty->driver_data;
+	upstat_t mask = UPSTAT_SYNC_FIFO;
 	struct uart_port *port;
-	upstat_t mask = 0;
 
 	port = uart_port_ref(state);
 	if (!port)

@@ -703,8 +703,8 @@ static void uart_throttle(struct tty_struct *tty)
 static void uart_unthrottle(struct tty_struct *tty)
 {
 	struct uart_state *state = tty->driver_data;
+	upstat_t mask = UPSTAT_SYNC_FIFO;
 	struct uart_port *port;
-	upstat_t mask = 0;
 
 	port = uart_port_ref(state);
 	if (!port)
include/linux/serial_core.h +1 -0
index 1d356105f25a..d224961e1346 100644
@@ -233,6 +233,7 @@ struct uart_port {
 #define UPSTAT_AUTORTS		((__force upstat_t) (1 << 2))
 #define UPSTAT_AUTOCTS		((__force upstat_t) (1 << 3))
 #define UPSTAT_AUTOXOFF		((__force upstat_t) (1 << 4))
+#define UPSTAT_SYNC_FIFO	((__force upstat_t) (1 << 5))
 
 	int			hw_stopped;		/* sw-assisted CTS flow state */
 	unsigned int		mctrl;			/* current modem ctrl settings */
Signature · 1 line
2.14.1
author
#23 lore raw

PATCH v2 2/4 serial/8250: export serial8250_read_char

In reply to Jeremy Kerr

Currently, we export serial8250_rx_chars, which does a whole bunch of reads from the 8250 data register, without any form of flow control between reads.

An upcoming change to the aspeed vuart driver implements more fine-grained flow control in the interrupt handler, requiring character-at-a-time control over the rx path.

This change exports serial8250_read_char to allow this.

Signed-off-by Jeremy Kerr <[email protected]>
Tested-by Eddie James <[email protected]>
---
 drivers/tty/serial/8250/8250_port.c | 3 ++-
 include/linux/serial_8250.h         | 1 +
 2 files changed, 3 insertions(+), 1 deletion(-)
drivers/tty/serial/8250/8250_port.c +2 -1
index 95833cbc4338..8fbd5fbeb318 100644
@@ -1680,7 +1680,7 @@ static void serial8250_enable_ms(struct uart_port *port)
 	serial8250_rpm_put(up);
 }
 
-static void serial8250_read_char(struct uart_8250_port *up, unsigned char lsr)
+void serial8250_read_char(struct uart_8250_port *up, unsigned char lsr)
 {
 	struct uart_port *port = &up->port;
 	unsigned char ch;

@@ -1740,6 +1740,7 @@ static void serial8250_read_char(struct uart_8250_port *up, unsigned char lsr)
 
 	uart_insert_char(port, lsr, UART_LSR_OE, ch, flag);
 }
+EXPORT_SYMBOL_GPL(serial8250_read_char);
 
 /*
  * serial8250_rx_chars: processes according to the passed in LSR
include/linux/serial_8250.h +1 -0
index a27ef5f56431..76b9db71e489 100644
@@ -163,6 +163,7 @@ extern void serial8250_do_set_mctrl(struct uart_port *port, unsigned int mctrl);
 extern int fsl8250_handle_irq(struct uart_port *port);
 int serial8250_handle_irq(struct uart_port *port, unsigned int iir);
 unsigned char serial8250_rx_chars(struct uart_8250_port *up, unsigned char lsr);
+void serial8250_read_char(struct uart_8250_port *up, unsigned char lsr);
 void serial8250_tx_chars(struct uart_8250_port *up);
 unsigned int serial8250_modem_status(struct uart_8250_port *up);
 void serial8250_init_port(struct uart_8250_port *up);
Signature · 1 line
2.14.1
author
#24 lore raw

PATCH v2 3/4 serial/aspeed-vuart: Implement rx throttling

In reply to Jeremy Kerr

The aspeed VUART runs at LPC bus frequency, rather than being restricted to a typical UART baud rate. This means that the VUART can receive a lot of data, which can overrun tty flip buffers, and/or cause a large amount of interrupt traffic.

This change implements the uart_port->throttle & unthrottle callbacks, implemented by disabling the receiver line status & received data available IRQs.

Signed-off-by Jeremy Kerr <[email protected]>
Tested-by Eddie James <[email protected]>
---
 drivers/tty/serial/8250/8250_aspeed_vuart.c | 27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)
drivers/tty/serial/8250/8250_aspeed_vuart.c +27 -0
index 74a408d9db24..cd1bb49dadfe 100644
@@ -179,6 +179,30 @@ static void aspeed_vuart_shutdown(struct uart_port *uart_port)
 	serial8250_do_shutdown(uart_port);
 }
 
+static void aspeed_vuart_set_throttle(struct uart_port *port, bool throttle)
+{
+	unsigned char irqs = UART_IER_RLSI | UART_IER_RDI;
+	struct uart_8250_port *up = up_to_u8250p(port);
+	unsigned long flags;
+
+	spin_lock_irqsave(&port->lock, flags);
+	up->ier &= ~irqs;
+	if (!throttle)
+		up->ier |= irqs;
+	serial_out(up, UART_IER, up->ier);
+	spin_unlock_irqrestore(&port->lock, flags);
+}
+
+static void aspeed_vuart_throttle(struct uart_port *port)
+{
+	aspeed_vuart_set_throttle(port, true);
+}
+
+static void aspeed_vuart_unthrottle(struct uart_port *port)
+{
+	aspeed_vuart_set_throttle(port, false);
+}
+
 static int aspeed_vuart_probe(struct platform_device *pdev)
 {
 	struct uart_8250_port port;

@@ -208,6 +232,9 @@ static int aspeed_vuart_probe(struct platform_device *pdev)
 	port.port.mapsize = resource_size(res);
 	port.port.startup = aspeed_vuart_startup;
 	port.port.shutdown = aspeed_vuart_shutdown;
+	port.port.throttle = aspeed_vuart_throttle;
+	port.port.unthrottle = aspeed_vuart_unthrottle;
+	port.port.status = UPSTAT_SYNC_FIFO;
 	port.port.dev = &pdev->dev;
 
 	rc = sysfs_create_group(&vuart->dev->kobj, &aspeed_vuart_attr_group);
Signature · 1 line
2.14.1
author
#25 lore raw

PATCH v2 4/4 serial/aspeed-vuart: Implement quick throttle mechanism

In reply to Jeremy Kerr

Although we populate the ->throttle and ->unthrottle UART operations, these may not be called until the ldisc has had a chance to schedule and check buffer space. This means that we may overflow the flip buffers without ever hitting the ldisc's throttle threshold.

This change implements an interrupt-based throttle, where we check for space in the flip buffers before reading characters from the UART's FIFO. If there's no space available, we disable the RX interrupt and schedule a timer to check for space later.

For this, we need an unlocked version of the set_throttle function to be able to change throttle state from the irq_handler, which already holds port->lock.

This prevents a case where we drop characters under heavy RX load.

Signed-off-by Jeremy Kerr <[email protected]>
Tested-by Eddie James <[email protected]>

---
v2: Use updated timer API, fix text wrap

---
 drivers/tty/serial/8250/8250_aspeed_vuart.c | 105 ++++++++++++++++++++++++++--
 1 file changed, 101 insertions(+), 4 deletions(-)
drivers/tty/serial/8250/8250_aspeed_vuart.c +101 -4
index cd1bb49dadfe..023db3266757 100644
@@ -10,6 +10,8 @@
 #include <linux/of_address.h>
 #include <linux/of_irq.h>
 #include <linux/of_platform.h>
+#include <linux/tty.h>
+#include <linux/tty_flip.h>
 #include <linux/clk.h>
 
 #include "8250.h"

@@ -28,8 +30,17 @@ struct aspeed_vuart {
 	void __iomem		*regs;
 	struct clk		*clk;
 	int			line;
+	struct timer_list	unthrottle_timer;
+	struct uart_8250_port	*port;
 };
 
+/*
+ * If we fill the tty flip buffers, we throttle the data ready interrupt
+ * to prevent dropped characters. This timeout defines how long we wait
+ * to (conditionally, depending on buffer state) unthrottle.
+ */
+static const int unthrottle_timeout = HZ/10;
+
 /*
  * The VUART is basically two UART 'front ends' connected by their FIFO
  * (no actual serial line in between). One is on the BMC side (management

@@ -179,17 +190,23 @@ static void aspeed_vuart_shutdown(struct uart_port *uart_port)
 	serial8250_do_shutdown(uart_port);
 }
 
-static void aspeed_vuart_set_throttle(struct uart_port *port, bool throttle)
+static void __aspeed_vuart_set_throttle(struct uart_8250_port *up,
+		bool throttle)
 {
 	unsigned char irqs = UART_IER_RLSI | UART_IER_RDI;
-	struct uart_8250_port *up = up_to_u8250p(port);
-	unsigned long flags;
 
-	spin_lock_irqsave(&port->lock, flags);
 	up->ier &= ~irqs;
 	if (!throttle)
 		up->ier |= irqs;
 	serial_out(up, UART_IER, up->ier);
+}
+static void aspeed_vuart_set_throttle(struct uart_port *port, bool throttle)
+{
+	struct uart_8250_port *up = up_to_u8250p(port);
+	unsigned long flags;
+
+	spin_lock_irqsave(&port->lock, flags);
+	__aspeed_vuart_set_throttle(up, throttle);
 	spin_unlock_irqrestore(&port->lock, flags);
 }
 

@@ -203,6 +220,83 @@ static void aspeed_vuart_unthrottle(struct uart_port *port)
 	aspeed_vuart_set_throttle(port, false);
 }
 
+static void aspeed_vuart_unthrottle_exp(struct timer_list *timer)
+{
+	struct aspeed_vuart *vuart = from_timer(vuart, timer, unthrottle_timer);
+	struct uart_8250_port *up = vuart->port;
+
+	if (!tty_buffer_space_avail(&up->port.state->port)) {
+		mod_timer(&vuart->unthrottle_timer, unthrottle_timeout);
+		return;
+	}
+
+	aspeed_vuart_unthrottle(&up->port);
+}
+
+/*
+ * Custom interrupt handler to manage finer-grained flow control. Although we
+ * have throttle/unthrottle callbacks, we've seen that the VUART device can
+ * deliver characters faster than the ldisc has a chance to check buffer space
+ * against the throttle threshold. This results in dropped characters before
+ * the throttle.
+ *
+ * We do this by checking for flip buffer space before RX. If we have no space,
+ * throttle now and schedule an unthrottle for later, once the ldisc has had
+ * a chance to drain the buffers.
+ */
+static int aspeed_vuart_handle_irq(struct uart_port *port)
+{
+	struct uart_8250_port *up = up_to_u8250p(port);
+	unsigned int iir, lsr;
+	unsigned long flags;
+	int space, count;
+
+	iir = serial_port_in(port, UART_IIR);
+
+	if (iir & UART_IIR_NO_INT)
+		return 0;
+
+	spin_lock_irqsave(&port->lock, flags);
+
+	lsr = serial_port_in(port, UART_LSR);
+
+	if (lsr & (UART_LSR_DR | UART_LSR_BI)) {
+		space = tty_buffer_space_avail(&port->state->port);
+
+		if (!space) {
+			/* throttle and schedule an unthrottle later */
+			struct aspeed_vuart *vuart = port->private_data;
+			__aspeed_vuart_set_throttle(up, true);
+
+			if (!timer_pending(&vuart->unthrottle_timer)) {
+				vuart->port = up;
+				mod_timer(&vuart->unthrottle_timer,
+						unthrottle_timeout);
+			}
+
+		} else {
+			count = min(space, 256);
+
+			do {
+				serial8250_read_char(up, lsr);
+				lsr = serial_in(up, UART_LSR);
+				if (--count == 0)
+					break;
+			} while (lsr & (UART_LSR_DR | UART_LSR_BI));
+
+			tty_flip_buffer_push(&port->state->port);
+		}
+	}
+
+	serial8250_modem_status(up);
+	if (lsr & UART_LSR_THRE)
+		serial8250_tx_chars(up);
+
+	spin_unlock_irqrestore(&port->lock, flags);
+
+	return 1;
+}
+
 static int aspeed_vuart_probe(struct platform_device *pdev)
 {
 	struct uart_8250_port port;

@@ -219,6 +313,7 @@ static int aspeed_vuart_probe(struct platform_device *pdev)
 		return -ENOMEM;
 
 	vuart->dev = &pdev->dev;
+	timer_setup(&vuart->unthrottle_timer, aspeed_vuart_unthrottle_exp, 0);
 
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	vuart->regs = devm_ioremap_resource(&pdev->dev, res);

@@ -280,6 +375,7 @@ static int aspeed_vuart_probe(struct platform_device *pdev)
 
 	port.port.irq = irq_of_parse_and_map(np, 0);
 	port.port.irqflags = IRQF_SHARED;
+	port.port.handle_irq = aspeed_vuart_handle_irq;
 	port.port.iotype = UPIO_MEM;
 	port.port.type = PORT_16550A;
 	port.port.uartclk = clk;

@@ -319,6 +415,7 @@ static int aspeed_vuart_remove(struct platform_device *pdev)
 {
 	struct aspeed_vuart *vuart = platform_get_drvdata(pdev);
 
+	del_timer_sync(&vuart->unthrottle_timer);
 	aspeed_vuart_set_enabled(vuart, false);
 	serial8250_unregister_port(vuart->line);
 	sysfs_remove_group(&vuart->dev->kobj, &aspeed_vuart_attr_group);
Signature · 1 line
2.14.1
maintainer
#26 lore raw

v2 cover serial: implement flow control for ASPEED VUART driver

In reply to Jeremy Kerr
Quoted 7 lines · Jeremy Kerr

--
v2:
- rebase onto tty-next, dropping applied proc patch
- 1/4 simplify status mask checks
- 4/4 Use updated timer API, fix text wrap

Quoted 5 lines

Jeremy Kerr (4):
serial: Introduce UPSTAT_SYNC_FIFO for synchronised FIFOs serial/8250: export serial8250_read_char serial/aspeed-vuart: Implement rx throttling serial/aspeed-vuart: Implement quick throttle mechanism

I gave v2 a spin on top of 4.16:

Tested-by Joel Stanley <[email protected]>

Cheers,

Joel

Quoted 11 lines
 drivers/tty/serial/8250/8250_aspeed_vuart.c | 124 ++++++++++++++++++++++++++++
 drivers/tty/serial/8250/8250_port.c         |   3 +-
 drivers/tty/serial/serial_core.c            |   4 +-
 include/linux/serial_8250.h                 |   1 +
 include/linux/serial_core.h                 |   1 +
 5 files changed, 130 insertions(+), 3 deletions(-)

--
2.14.1