[PATCH 0/5] serial: implement flow control for ASPEED VUART driver

[PATCH 0/5] serial: implement flow control for ASPEED VUART driver

[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).

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(-)

-- 
2.14.1

[PATCH 1/5] serial: Introduce UPSTAT_SYNC_FIFO for synchronised FIFOs

[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 <jk@ozlabs.org>
---
 drivers/tty/serial/serial_core.c | 4 ++--
 include/linux/serial_core.h      | 1 +
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
index f534a40aebde..8f3dfc8b5307 100644
--- a/drivers/tty/serial/serial_core.c
+++ b/drivers/tty/serial/serial_core.c
@@ -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;
 	}
diff --git a/include/linux/serial_core.h b/include/linux/serial_core.h
index 1775500294bb..bf600ae0290d 100644
--- a/include/linux/serial_core.h
+++ b/include/linux/serial_core.h
@@ -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 */
-- 
2.14.1

[PATCH 1/5] serial: Introduce UPSTAT_SYNC_FIFO for synchronised FIFOs

[Greg Kroah-Hartman]

On Wed, Mar 21, 2018 at 10:52:37AM +0800, Jeremy Kerr wrote:
> 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 <jk@ozlabs.org>
> ---
>  drivers/tty/serial/serial_core.c | 4 ++--
>  include/linux/serial_core.h      | 1 +
>  2 files changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
> index f534a40aebde..8f3dfc8b5307 100644
> --- a/drivers/tty/serial/serial_core.c
> +++ b/drivers/tty/serial/serial_core.c
> @@ -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?

> @@ -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

[PATCH 1/5] serial: Introduce UPSTAT_SYNC_FIFO for synchronised FIFOs

[Jeremy Kerr]

Hi Greg,

>> --- a/drivers/tty/serial/serial_core.c
>> +++ b/drivers/tty/serial/serial_core.c
>> @@ -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?

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

[PATCH 1/5] serial: Introduce UPSTAT_SYNC_FIFO for synchronised FIFOs

[Eddie James]

On 03/20/2018 09:52 PM, Jeremy Kerr wrote:
> 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 <eajames@linux.vnet.ibm.com>

>
> Signed-off-by: Jeremy Kerr <jk@ozlabs.org>
> ---
>   drivers/tty/serial/serial_core.c | 4 ++--
>   include/linux/serial_core.h      | 1 +
>   2 files changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
> index f534a40aebde..8f3dfc8b5307 100644
> --- a/drivers/tty/serial/serial_core.c
> +++ b/drivers/tty/serial/serial_core.c
> @@ -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;
>   	}
> diff --git a/include/linux/serial_core.h b/include/linux/serial_core.h
> index 1775500294bb..bf600ae0290d 100644
> --- a/include/linux/serial_core.h
> +++ b/include/linux/serial_core.h
> @@ -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 */

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

[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 <jk@ozlabs.org>
---
 drivers/tty/serial/serial_core.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
index 8f3dfc8b5307..fc677534b510 100644
--- a/drivers/tty/serial/serial_core.c
+++ b/drivers/tty/serial/serial_core.c
@@ -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)) \
-- 
2.14.1

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

[Greg Kroah-Hartman]

On Wed, Mar 21, 2018 at 10:52:38AM +0800, Jeremy Kerr wrote:
> 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 <jk@ozlabs.org>
> ---
>  drivers/tty/serial/serial_core.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
> index 8f3dfc8b5307..fc677534b510 100644
> --- a/drivers/tty/serial/serial_core.c
> +++ b/drivers/tty/serial/serial_core.c
> @@ -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 :)

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

[Jeremy Kerr]

Hi Greg,

>> diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
>> index 8f3dfc8b5307..fc677534b510 100644
>> --- a/drivers/tty/serial/serial_core.c
>> +++ b/drivers/tty/serial/serial_core.c
>> @@ -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 :)

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

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

[Eddie James]

On 03/20/2018 09:52 PM, Jeremy Kerr wrote:
> 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 <eajames@linux.vnet.ibm.com>

>
> Signed-off-by: Jeremy Kerr <jk@ozlabs.org>
> ---
>   drivers/tty/serial/serial_core.c | 2 ++
>   1 file changed, 2 insertions(+)
>
> diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
> index 8f3dfc8b5307..fc677534b510 100644
> --- a/drivers/tty/serial/serial_core.c
> +++ b/drivers/tty/serial/serial_core.c
> @@ -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)) \

[PATCH 3/5] serial/8250: export serial8250_read_char

[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 <jk@ozlabs.org>
---
 drivers/tty/serial/8250/8250_port.c | 3 ++-
 include/linux/serial_8250.h         | 1 +
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
index a5fe0e66c607..ea435406936c 100644
--- a/drivers/tty/serial/8250/8250_port.c
+++ b/drivers/tty/serial/8250/8250_port.c
@@ -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
diff --git a/include/linux/serial_8250.h b/include/linux/serial_8250.h
index 61fbb440449c..4639a3608614 100644
--- a/include/linux/serial_8250.h
+++ b/include/linux/serial_8250.h
@@ -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);
-- 
2.14.1

[PATCH 3/5] serial/8250: export serial8250_read_char

[Eddie James]

On 03/20/2018 09:52 PM, Jeremy Kerr wrote:
> 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 <eajames@linux.vnet.ibm.com>

>
> Signed-off-by: Jeremy Kerr <jk@ozlabs.org>
> ---
>   drivers/tty/serial/8250/8250_port.c | 3 ++-
>   include/linux/serial_8250.h         | 1 +
>   2 files changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
> index a5fe0e66c607..ea435406936c 100644
> --- a/drivers/tty/serial/8250/8250_port.c
> +++ b/drivers/tty/serial/8250/8250_port.c
> @@ -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
> diff --git a/include/linux/serial_8250.h b/include/linux/serial_8250.h
> index 61fbb440449c..4639a3608614 100644
> --- a/include/linux/serial_8250.h
> +++ b/include/linux/serial_8250.h
> @@ -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);

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

[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 <jk@ozlabs.org>
---
 drivers/tty/serial/8250/8250_aspeed_vuart.c | 27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)

diff --git a/drivers/tty/serial/8250/8250_aspeed_vuart.c b/drivers/tty/serial/8250/8250_aspeed_vuart.c
index 33a801353114..50c082b4a1ac 100644
--- a/drivers/tty/serial/8250/8250_aspeed_vuart.c
+++ b/drivers/tty/serial/8250/8250_aspeed_vuart.c
@@ -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);
-- 
2.14.1

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

[Eddie James]

On 03/20/2018 09:52 PM, Jeremy Kerr wrote:
> 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 <eajames@linux.vnet.ibm.com>

>
> Signed-off-by: Jeremy Kerr <jk@ozlabs.org>
> ---
>   drivers/tty/serial/8250/8250_aspeed_vuart.c | 27 +++++++++++++++++++++++++++
>   1 file changed, 27 insertions(+)
>
> diff --git a/drivers/tty/serial/8250/8250_aspeed_vuart.c b/drivers/tty/serial/8250/8250_aspeed_vuart.c
> index 33a801353114..50c082b4a1ac 100644
> --- a/drivers/tty/serial/8250/8250_aspeed_vuart.c
> +++ b/drivers/tty/serial/8250/8250_aspeed_vuart.c
> @@ -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);

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

[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 <jk@ozlabs.org>
---
 drivers/tty/serial/8250/8250_aspeed_vuart.c | 103 ++++++++++++++++++++++++++--
 1 file changed, 99 insertions(+), 4 deletions(-)

diff --git a/drivers/tty/serial/8250/8250_aspeed_vuart.c b/drivers/tty/serial/8250/8250_aspeed_vuart.c
index 50c082b4a1ac..bc2b63578e58 100644
--- a/drivers/tty/serial/8250/8250_aspeed_vuart.c
+++ b/drivers/tty/serial/8250/8250_aspeed_vuart.c
@@ -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);
-- 
2.14.1

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

[Joel Stanley]

Hi Jeremy,

On Wed, Mar 21, 2018 at 1:22 PM, Jeremy Kerr <jk@ozlabs.org> wrote:
> 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 <jk@ozlabs.org>
> ---
>  drivers/tty/serial/8250/8250_aspeed_vuart.c | 103 ++++++++++++++++++++++++++--
>  1 file changed, 99 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/tty/serial/8250/8250_aspeed_vuart.c b/drivers/tty/serial/8250/8250_aspeed_vuart.c
> index 50c082b4a1ac..bc2b63578e58 100644
> --- a/drivers/tty/serial/8250/8250_aspeed_vuart.c
> +++ b/drivers/tty/serial/8250/8250_aspeed_vuart.c
> @@ -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);

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

Cheers,

Joel

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

[Jeremy Kerr]

Hi Joel,

> 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

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

[Jeremy Kerr]

Hi all,

> +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

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

[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 <jk@ozlabs.org>

---
v2: Use updated timer API, fix text wrap
---
 drivers/tty/serial/8250/8250_aspeed_vuart.c | 105 ++++++++++++++++++++++++++--
 1 file changed, 101 insertions(+), 4 deletions(-)

diff --git a/drivers/tty/serial/8250/8250_aspeed_vuart.c b/drivers/tty/serial/8250/8250_aspeed_vuart.c
index cd1bb49dadfe..023db3266757 100644
--- a/drivers/tty/serial/8250/8250_aspeed_vuart.c
+++ b/drivers/tty/serial/8250/8250_aspeed_vuart.c
@@ -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);
-- 
2.14.1

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

[Eddie James]

On 03/20/2018 09:52 PM, Jeremy Kerr wrote:
> 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 <eajames@linux.vnet.ibm.com>

>
> Signed-off-by: Jeremy Kerr <jk@ozlabs.org>
> ---
>   drivers/tty/serial/8250/8250_aspeed_vuart.c | 103 ++++++++++++++++++++++++++--
>   1 file changed, 99 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/tty/serial/8250/8250_aspeed_vuart.c b/drivers/tty/serial/8250/8250_aspeed_vuart.c
> index 50c082b4a1ac..bc2b63578e58 100644
> --- a/drivers/tty/serial/8250/8250_aspeed_vuart.c
> +++ b/drivers/tty/serial/8250/8250_aspeed_vuart.c
> @@ -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);

[PATCH 0/5] serial: implement flow control for ASPEED VUART driver

[Greg Kroah-Hartman]

On Wed, Mar 21, 2018 at 10:52:36AM +0800, Jeremy Kerr wrote:
> 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.

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

[PATCH 0/5] serial: implement flow control for ASPEED VUART driver

[Jeremy Kerr]

Hi Greg,

> 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

[PATCH v2 0/4] serial: implement flow control for ASPEED VUART driver

[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 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(-)

-- 
2.14.1

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

[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 <jk@ozlabs.org>
Tested-by: Eddie James <eajames@linux.vnet.ibm.com>

--
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(-)

diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
index 0466f9f08a91..c47158c93202 100644
--- a/drivers/tty/serial/serial_core.c
+++ b/drivers/tty/serial/serial_core.c
@@ -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)
diff --git a/include/linux/serial_core.h b/include/linux/serial_core.h
index 1d356105f25a..d224961e1346 100644
--- a/include/linux/serial_core.h
+++ b/include/linux/serial_core.h
@@ -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 */
-- 
2.14.1

[PATCH v2 2/4] serial/8250: export serial8250_read_char

[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 <jk@ozlabs.org>
Tested-by: Eddie James <eajames@linux.vnet.ibm.com>
---
 drivers/tty/serial/8250/8250_port.c | 3 ++-
 include/linux/serial_8250.h         | 1 +
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
index 95833cbc4338..8fbd5fbeb318 100644
--- a/drivers/tty/serial/8250/8250_port.c
+++ b/drivers/tty/serial/8250/8250_port.c
@@ -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
diff --git a/include/linux/serial_8250.h b/include/linux/serial_8250.h
index a27ef5f56431..76b9db71e489 100644
--- a/include/linux/serial_8250.h
+++ b/include/linux/serial_8250.h
@@ -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);
-- 
2.14.1

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

[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 <jk@ozlabs.org>
Tested-by: Eddie James <eajames@linux.vnet.ibm.com>
---
 drivers/tty/serial/8250/8250_aspeed_vuart.c | 27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)

diff --git a/drivers/tty/serial/8250/8250_aspeed_vuart.c b/drivers/tty/serial/8250/8250_aspeed_vuart.c
index 74a408d9db24..cd1bb49dadfe 100644
--- a/drivers/tty/serial/8250/8250_aspeed_vuart.c
+++ b/drivers/tty/serial/8250/8250_aspeed_vuart.c
@@ -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);
-- 
2.14.1

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

[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 <jk@ozlabs.org>
Tested-by: Eddie James <eajames@linux.vnet.ibm.com>

---
v2: Use updated timer API, fix text wrap
---
 drivers/tty/serial/8250/8250_aspeed_vuart.c | 105 ++++++++++++++++++++++++++--
 1 file changed, 101 insertions(+), 4 deletions(-)

diff --git a/drivers/tty/serial/8250/8250_aspeed_vuart.c b/drivers/tty/serial/8250/8250_aspeed_vuart.c
index cd1bb49dadfe..023db3266757 100644
--- a/drivers/tty/serial/8250/8250_aspeed_vuart.c
+++ b/drivers/tty/serial/8250/8250_aspeed_vuart.c
@@ -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);
-- 
2.14.1

[PATCH v2 0/4] serial: implement flow control for ASPEED VUART driver

[Joel Stanley]

On 27 March 2018 at 14:18, Jeremy Kerr <jk@ozlabs.org> wrote:
> --
> 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

I gave v2 a spin on top of 4.16:

Tested-by: Joel Stanley <joel@jms.id.au>

Cheers,

Joel


>
>  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
>