Received-SPF: pass (google.com: domain of linux-kernel+bounces-68425-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
From: =?UTF-8?q?Ilpo=20J=C3=A4rvinen?= <ilpo.jarvinen@linux.intel.com>
Date: Fri, 16 Feb 2024 12:12:44 +0200 (EET)
To: Leonardo Bras <leobras@redhat.com>
cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>, 
    Jiri Slaby <jirislaby@kernel.org>, Thomas Gleixner <tglx@linutronix.de>, 
    Tony Lindgren <tony@atomide.com>, 
    Andy Shevchenko <andriy.shevchenko@linux.intel.com>, 
    John Ogness <john.ogness@linutronix.de>, 
    =?ISO-8859-15?Q?Uwe_Kleine-K=F6nig?= <u.kleine-koenig@pengutronix.de>, 
    Florian Fainelli <florian.fainelli@broadcom.com>, 
    Shanker Donthineni <sdonthineni@nvidia.com>, 
    LKML <linux-kernel@vger.kernel.org>, 
    linux-serial <linux-serial@vger.kernel.org>
Subject: Re: [RFC PATCH v2 4/4] tty/serial8250: Make use of IRQ_HANDLED_MANY
 interface
In-Reply-To: <20240216075948.131372-6-leobras@redhat.com>
Message-ID: <8c080e51-6ae5-0e8a-69f6-ca3666164248@linux.intel.com>
References: <20240216075948.131372-2-leobras@redhat.com> <20240216075948.131372-6-leobras@redhat.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII

On Fri, 16 Feb 2024, Leonardo Bras wrote:

> For every TX byte an IRQ is requested.
> On threaded IRQs, the handler calls serial8250_tx_chars can send multiple
> bytes, limited to it's queue size (tx_loadsz).

Perhaps I'm missing something here but I don't understand what this tries 
to say.

- 8250 driver gets TX empty IRQ
- We write x bytes to FIFO
- UART blasts those bits to wire, eventually emptying FIFO
- We get the next TX empty IRQ

What in this makes "for every TX byte an IRQ is requested" true? There's 
one IRQ only for every x bytes TX'ed as far as I can tell!?!

-- 
 i.

> When this happens, the handler return IRQ_HANDLED with reduces the
> unhandled IRQ counter only by 1, even though many requests have been
> handled at once.
> 
> This causes the unhandled IRQ counter to go up until it reaches the maximum
> and causes the registered IRQ to be disabled, thus breaking the serial
> console.
> 
> Make use of the newly introduced IRQ_HANDLED_MANY interface to return the
> number of requests handled, so the unhandled IRQ counter can get decreased
> accordingly.
> 
> Signed-off-by: Leonardo Bras <leobras@redhat.com>
> ---
>  include/linux/serial_8250.h         |  2 +-
>  drivers/tty/serial/8250/8250_core.c | 13 ++++++++-----
>  drivers/tty/serial/8250/8250_port.c | 16 ++++++++++------
>  3 files changed, 19 insertions(+), 12 deletions(-)
> 
> diff --git a/include/linux/serial_8250.h b/include/linux/serial_8250.h
> index ec46e3b49ee99..c9d4271b71d70 100644
> --- a/include/linux/serial_8250.h
> +++ b/include/linux/serial_8250.h
> @@ -200,7 +200,7 @@ int fsl8250_handle_irq(struct uart_port *port);
>  int serial8250_handle_irq(struct uart_port *port, unsigned int iir);
>  u16 serial8250_rx_chars(struct uart_8250_port *up, u16 lsr);
>  void serial8250_read_char(struct uart_8250_port *up, u16 lsr);
> -void serial8250_tx_chars(struct uart_8250_port *up);
> +int 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);
>  void serial8250_set_defaults(struct uart_8250_port *up);
> diff --git a/drivers/tty/serial/8250/8250_core.c b/drivers/tty/serial/8250/8250_core.c
> index ae637155fe7cd..2fab9102eec45 100644
> --- a/drivers/tty/serial/8250/8250_core.c
> +++ b/drivers/tty/serial/8250/8250_core.c
> @@ -110,7 +110,7 @@ static irqreturn_t serial8250_interrupt(int irq, void *dev_id)
>  {
>  	struct irq_info *i = dev_id;
>  	struct list_head *l, *end = NULL;
> -	int pass_counter = 0, handled = 0;
> +	int pass_counter = 0, handled_total = 0;
>  
>  	pr_debug("%s(%d): start\n", __func__, irq);
>  
> @@ -120,15 +120,18 @@ static irqreturn_t serial8250_interrupt(int irq, void *dev_id)
>  	do {
>  		struct uart_8250_port *up;
>  		struct uart_port *port;
> +		int handled;
>  
>  		up = list_entry(l, struct uart_8250_port, list);
>  		port = &up->port;
>  
> -		if (port->handle_irq(port)) {
> -			handled = 1;
> +		handled = port->handle_irq(port);
> +		if (handled) {
> +			handled_total += handled;
>  			end = NULL;
> -		} else if (end == NULL)
> +		} else if (end == NULL) {
>  			end = l;
> +		}
>  
>  		l = l->next;
>  
> @@ -140,7 +143,7 @@ static irqreturn_t serial8250_interrupt(int irq, void *dev_id)
>  
>  	pr_debug("%s(%d): end\n", __func__, irq);
>  
> -	return IRQ_RETVAL(handled);
> +	return IRQ_RETVAL_MANY(handled_total);
>  }
>  
>  /*
> diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
> index f799c34f1603c..74d53507a73d4 100644
> --- a/drivers/tty/serial/8250/8250_port.c
> +++ b/drivers/tty/serial/8250/8250_port.c
> @@ -1802,7 +1802,7 @@ u16 serial8250_rx_chars(struct uart_8250_port *up, u16 lsr)
>  }
>  EXPORT_SYMBOL_GPL(serial8250_rx_chars);
>  
> -void serial8250_tx_chars(struct uart_8250_port *up)
> +int serial8250_tx_chars(struct uart_8250_port *up)
>  {
>  	struct uart_port *port = &up->port;
>  	struct circ_buf *xmit = &port->state->xmit;
> @@ -1810,15 +1810,15 @@ void serial8250_tx_chars(struct uart_8250_port *up)
>  
>  	if (port->x_char) {
>  		uart_xchar_out(port, UART_TX);
> -		return;
> +		return 0;
>  	}
>  	if (uart_tx_stopped(port)) {
>  		serial8250_stop_tx(port);
> -		return;
> +		return 0;
>  	}
>  	if (uart_circ_empty(xmit)) {
>  		__stop_tx(up);
> -		return;
> +		return 0;
>  	}
>  
>  	count = up->tx_loadsz;
> @@ -1858,6 +1858,9 @@ void serial8250_tx_chars(struct uart_8250_port *up)
>  	 */
>  	if (uart_circ_empty(xmit) && !(up->capabilities & UART_CAP_RPM))
>  		__stop_tx(up);
> +
> +	/* Return number of chars sent */
> +	return up->tx_loadsz - count;
>  }
>  EXPORT_SYMBOL_GPL(serial8250_tx_chars);
>  
> @@ -1923,6 +1926,7 @@ int serial8250_handle_irq(struct uart_port *port, unsigned int iir)
>  	bool skip_rx = false;
>  	unsigned long flags;
>  	u16 status;
> +	int handled = 0;
>  
>  	if (iir & UART_IIR_NO_INT)
>  		return 0;
> @@ -1956,14 +1960,14 @@ int serial8250_handle_irq(struct uart_port *port, unsigned int iir)
>  	serial8250_modem_status(up);
>  	if ((status & UART_LSR_THRE) && (up->ier & UART_IER_THRI)) {
>  		if (!up->dma || up->dma->tx_err)
> -			serial8250_tx_chars(up);
> +			handled = serial8250_tx_chars(up);
>  		else if (!up->dma->tx_running)
>  			__stop_tx(up);
>  	}
>  
>  	uart_unlock_and_check_sysrq_irqrestore(port, flags);
>  
> -	return 1;
> +	return handled ? : 1;
>  }
>  EXPORT_SYMBOL_GPL(serial8250_handle_irq);
>  
>