Re: [PATCH 15/33] serial: 8250: extract serial8250_THRE_test()

["Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>]

On Wed, 11 Jun 2025, Jiri Slaby (SUSE) wrote:

> serial8250_do_startup() contains a stand-alone code for probing THRE.
> Furthermore, the code block is conditional (port->irq and test for
> UPF_NO_THRE_TEST).
> 
> Move this code to a separate function. The conditional can be evaluated
> easier there -- by a simple return in the beginning. So the indentation
> level lowers and the code is overall more readable now.
> 
> Signed-off-by: Jiri Slaby (SUSE) <jirislaby@kernel.org>
> ---
>  drivers/tty/serial/8250/8250_port.c | 92 +++++++++++++++--------------
>  1 file changed, 49 insertions(+), 43 deletions(-)
> 
> diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
> index c09a90b38d8f..5466286bb44f 100644
> --- a/drivers/tty/serial/8250/8250_port.c
> +++ b/drivers/tty/serial/8250/8250_port.c
> @@ -2199,6 +2199,54 @@ static void serial8250_set_TRG_levels(struct uart_port *port)
>  	}
>  }
>  
> +static void serial8250_THRE_test(struct uart_port *port)
> +{
> +	struct uart_8250_port *up = up_to_u8250p(port);
> +	unsigned long flags;
> +	bool iir_noint1, iir_noint2;
> +
> +	if (!port->irq)
> +		return;
> +
> +	if (up->port.flags & UPF_NO_THRE_TEST)
> +		return;
> +
> +	if (port->irqflags & IRQF_SHARED)
> +		disable_irq_nosync(port->irq);
> +
> +	/*
> +	 * Test for UARTs that do not reassert THRE when the transmitter is idle and the interrupt
> +	 * has already been cleared.  Real 16550s should always reassert this interrupt whenever the
> +	 * transmitter is idle and the interrupt is enabled.  Delays are necessary to allow register
> +	 * changes to become visible.

Very long comment lines are hard to read. (This is mostly not related to
line length limits, but with eye movement required.)

It may make sense to place some of the descriptive comment text into a 
function comment instead of placing them mid-function.

> +	 *
> +	 * Synchronize UART_IER access against the console.
> +	 */
> +	uart_port_lock_irqsave(port, &flags);
> +
> +	wait_for_xmitr(up, UART_LSR_THRE);
> +	serial_port_out_sync(port, UART_IER, UART_IER_THRI);
> +	udelay(1); /* allow THRE to set */

These comments mix visually into the code making this look a big wall of 
text overall. Maybe consider adding empty lines to the logic as well as
there are what looks clear steps in this logic.

> +	iir_noint1 = serial_port_in(port, UART_IIR) & UART_IIR_NO_INT;
> +	serial_port_out(port, UART_IER, 0);
> +	serial_port_out_sync(port, UART_IER, UART_IER_THRI);
> +	udelay(1); /* allow a working UART time to re-assert THRE */
> +	iir_noint2 = serial_port_in(port, UART_IIR) & UART_IIR_NO_INT;
> +	serial_port_out(port, UART_IER, 0);
> +
> +	uart_port_unlock_irqrestore(port, flags);
> +
> +	if (port->irqflags & IRQF_SHARED)
> +		enable_irq(port->irq);
> +
> +	/*
> +	 * If the interrupt is not reasserted, or we otherwise don't trust the iir, setup a timer to
> +	 * kick the UART on a regular basis.
> +	 */
> +	if ((!iir_noint1 && iir_noint2) || up->port.flags & UPF_BUG_THRE)
> +		up->bugs |= UART_BUG_THRE;
> +}
> +
>  int serial8250_do_startup(struct uart_port *port)
>  {
>  	struct uart_8250_port *up = up_to_u8250p(port);
> @@ -2258,49 +2306,7 @@ int serial8250_do_startup(struct uart_port *port)
>  	if (retval)
>  		goto out;
>  
> -	if (port->irq && !(up->port.flags & UPF_NO_THRE_TEST)) {
> -		unsigned char iir1;
> -
> -		if (port->irqflags & IRQF_SHARED)
> -			disable_irq_nosync(port->irq);
> -
> -		/*
> -		 * Test for UARTs that do not reassert THRE when the
> -		 * transmitter is idle and the interrupt has already
> -		 * been cleared.  Real 16550s should always reassert
> -		 * this interrupt whenever the transmitter is idle and
> -		 * the interrupt is enabled.  Delays are necessary to
> -		 * allow register changes to become visible.
> -		 *
> -		 * Synchronize UART_IER access against the console.
> -		 */
> -		uart_port_lock_irqsave(port, &flags);
> -
> -		wait_for_xmitr(up, UART_LSR_THRE);
> -		serial_port_out_sync(port, UART_IER, UART_IER_THRI);
> -		udelay(1); /* allow THRE to set */
> -		iir1 = serial_port_in(port, UART_IIR);
> -		serial_port_out(port, UART_IER, 0);
> -		serial_port_out_sync(port, UART_IER, UART_IER_THRI);
> -		udelay(1); /* allow a working UART time to re-assert THRE */
> -		iir = serial_port_in(port, UART_IIR);
> -		serial_port_out(port, UART_IER, 0);
> -
> -		uart_port_unlock_irqrestore(port, flags);
> -
> -		if (port->irqflags & IRQF_SHARED)
> -			enable_irq(port->irq);
> -
> -		/*
> -		 * If the interrupt is not reasserted, or we otherwise
> -		 * don't trust the iir, setup a timer to kick the UART
> -		 * on a regular basis.
> -		 */
> -		if ((!(iir1 & UART_IIR_NO_INT) && (iir & UART_IIR_NO_INT)) ||
> -		    up->port.flags & UPF_BUG_THRE) {
> -			up->bugs |= UART_BUG_THRE;
> -		}
> -	}
> +	serial8250_THRE_test(port);
>  
>  	up->ops->setup_timer(up);
>  
> 

-- 
 i.