Re: [RFC PATCH v2 4/4] tty/serial8250: Make use of IRQ_HANDLED_MANY interface

[Leonardo Bras]

On Fri, Feb 16, 2024 at 12:12:44PM +0200, Ilpo Järvinen wrote:
> 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!?!
> 

Context:
I created a C program for writting data to the serial by opening /dev/ttyS0 
and fprintf() strings of about 100bytes to it. This fprintf() runs alone in 
a for loop. This is compiled with GCC.

I noticed that it will create an IRQ for every char written to ttyS0.
Maybe I missed something, and this is not a rule, but that's what I 
perceived as an average.

My scenario:
- Linux compiled with force_irqthreads = true
- serial8250 used as a serial terminal (emulated by qemu)

For non-irqthreads it works just fine.

But in this (force_irqthreads = true) scenario the IRQ will get triggered a 
lot of times, and the threaded handler runs only sometimes, which makes it 
easy for the IRQs to be handled in batch, which is fine.

The issue: this causes irqs_unhandled to be incremented once every time 
note_interrupt() did not perceive a change in threads_handled. Since the 
threads_handled increments in batches, it means many of those IRQs will be 
considered "unhandled", leading to the serial8250 IRQ to be disabled.

I created a way (patches 2 & 3) to account for how many IRQ requests have 
actually been handled by a handler, if that handler can deal with them in 
batches. 

This patch is about me trying to make serial8250 report how many IRQs it 
handled, by using the (possibly incorrect) information that it will cause 
1 IRQ per tx-byte.

This 1 IRQ/tx-byte info was perceived by tracing the IRQ count and the 
number of bytes sent. It was also reinforced by serial8250_tx_chars() which 
seems to transmit 1 byte at a time, even though it repeats that up to FIFO 
size (up->tx_loadsz) in that function.

If that proves to be not correct, I will need to find a way of tracking 
the number of IRQs handled in that scenario, so the IRQ disabling thing can 
be avoided.

Does it make sense?

Thanks!
Leo

> -- 
>  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@xxxxxxxxxx>
> > ---
> >  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);
> >  
> > 
>