Hi Jeremy,
On Wed, Mar 21, 2018 at 1:22 PM, Jeremy Kerr <jk@xxxxxxxxxx> 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@xxxxxxxxxx>
> ---
> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-serial" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
