Re: [PATCH 17/33] serial: 8250: extract serial8250_clear_interrupts()

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

[-- Attachment #1: Type: text/plain, Size: 3241 bytes --]

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

> On three places in 8250_port.c, the interrupts are cleared by reading 4
> registers. Extract this to a separate function:
> serial8250_clear_interrupts(). And call it from all the places.
> 
> Note autoconfig_irq() now uses serial_port_in() instead of serial_in().
> But they are the same, in fact (modulo parameter).
> 
> Signed-off-by: Jiri Slaby (SUSE) <jirislaby@kernel.org>
> ---
>  drivers/tty/serial/8250/8250_port.c | 27 ++++++++++++---------------
>  1 file changed, 12 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
> index 6851c197b31d..a73f4db22feb 100644
> --- a/drivers/tty/serial/8250/8250_port.c
> +++ b/drivers/tty/serial/8250/8250_port.c
> @@ -705,6 +705,15 @@ static void serial8250_set_sleep(struct uart_8250_port *p, int sleep)
>  	serial8250_rpm_put(p);
>  }
>  
> +/* Clear the interrupt registers. */
> +static void serial8250_clear_interrupts(struct uart_port *port)
> +{
> +	serial_port_in(port, UART_LSR);
> +	serial_port_in(port, UART_RX);
> +	serial_port_in(port, UART_IIR);
> +	serial_port_in(port, UART_MSR);

Okay, although I'd call some of these "status" registers. Yes, one can use 
their status flags to trigger interrupts but it's not immediately obvious 
to me if the calling code really _only_ wants to clear interrupts or if it 
is also clearing stale status bits even if that's not explicitly 
mentioned. Especially, the one callsite which zeroes also the stored 
flags looks like it wants to get rid of stale status too (some status 
bits might have not yet migrated from the register to the cached flags).

Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>


> +}
> +
>  static void serial8250_clear_IER(struct uart_8250_port *up)
>  {
>  	if (up->capabilities & UART_CAP_UUE)
> @@ -1275,10 +1284,7 @@ static void autoconfig_irq(struct uart_8250_port *up)
>  	uart_port_lock_irq(port);
>  	serial_out(up, UART_IER, UART_IER_ALL_INTR);
>  	uart_port_unlock_irq(port);
> -	serial_in(up, UART_LSR);
> -	serial_in(up, UART_RX);
> -	serial_in(up, UART_IIR);
> -	serial_in(up, UART_MSR);
> +	serial8250_clear_interrupts(port);
>  	serial_out(up, UART_TX, 0xFF);
>  	udelay(20);
>  	irq = probe_irq_off(irqs);
> @@ -2322,13 +2328,7 @@ int serial8250_do_startup(struct uart_port *port)
>  	 */
>  	serial8250_clear_fifos(up);
>  
> -	/*
> -	 * Clear the interrupt registers.
> -	 */
> -	serial_port_in(port, UART_LSR);
> -	serial_port_in(port, UART_RX);
> -	serial_port_in(port, UART_IIR);
> -	serial_port_in(port, UART_MSR);
> +	serial8250_clear_interrupts(port);
>  
>  	/*
>  	 * At this point, there's no way the LSR could still be 0xff;
> @@ -2363,10 +2363,7 @@ int serial8250_do_startup(struct uart_port *port)
>  	 * saved flags to avoid getting false values from polling
>  	 * routines or the previous session.
>  	 */
> -	serial_port_in(port, UART_LSR);
> -	serial_port_in(port, UART_RX);
> -	serial_port_in(port, UART_IIR);
> -	serial_port_in(port, UART_MSR);
> +	serial8250_clear_interrupts(port);
>  	up->lsr_saved_flags = 0;
>  	up->msr_saved_flags = 0;
>  
> 

-- 
 i.