Re: [Xen-devel] [PATCH] xen: char: Remove unnecessary (uart->irq > 0) check

[Andre Przywara <andre.przywara@xxxxxxx>]

Hi,

On 28/04/18 10:08, Amit Singh Tomar wrote:
> While working on MVEBU uart driver, Julien pointed out that (uart->irq > 0)
> check is unnecessary during irq set up.if ever there is an invalid irq, driver
> initialization itself would be bailed out from platform_get_irq.
> 
> This patch would remove similar check for other uart drivers present in XEN.
> 
> Signed-off-by: Amit Singh Tomar <amittomer25@xxxxxxxxx>
> ---
>     * This patch is only compiled tested.
> ---
>  xen/drivers/char/cadence-uart.c | 15 ++++++++-------
>  xen/drivers/char/ns16550.c      | 35 ++++++++++++++---------------------
>  xen/drivers/char/omap-uart.c    |  2 +-
>  xen/drivers/char/pl011.c        | 13 +++++++------
>  4 files changed, 30 insertions(+), 35 deletions(-)
> 
> diff --git a/xen/drivers/char/cadence-uart.c b/xen/drivers/char/cadence-uart.c
> index 22905ba..1575787 100644
> --- a/xen/drivers/char/cadence-uart.c
> +++ b/xen/drivers/char/cadence-uart.c
> @@ -72,13 +72,14 @@ static void __init cuart_init_postirq(struct serial_port 
> *port)
>      struct cuart *uart = port->uart;
>      int rc;
>  
> -    if ( uart->irq > 0 )
> +    uart->irqaction.handler = cuart_interrupt;
> +    uart->irqaction.name    = "cadence-uart";
> +    uart->irqaction.dev_id  = port;
> +
> +    if ( (rc = setup_irq(uart->irq, 0, &uart->irqaction)) != 0 )
>      {
> -        uart->irqaction.handler = cuart_interrupt;
> -        uart->irqaction.name    = "cadence-uart";
> -        uart->irqaction.dev_id  = port;
> -        if ( (rc = setup_irq(uart->irq, 0, &uart->irqaction)) != 0 )
> -            printk("ERROR: Failed to allocate cadence-uart IRQ %d\n", 
> uart->irq);
> +        printk("ERROR: Failed to allocate cadence-uart IRQ %d\n", uart->irq);
> +        return;

Careful, this changes the behaviour here:
Formerly a failure in setup_irq() led to just the warning, but then
execution (and initialisation) continued, even without an IRQ properly
set up. Depending on the UART and its driver this may or may not work.
But at least it deserves some mentioning in the commit message.

I quickly tested with an PL011: if setup_irq() does not succeed (hacked
it to use the VGIC IRQ, which is already taken), the UART ignores any
input, because it never actively polls or checks for incoming
characters. But output works perfectly fine, and the system works as
excepted (I can login via ssh, but not on the console).
So we might want to upgrade the error message to state the fatality of
this failure, but proceed anyway (as we do right now).

I haven't (and mostly can't) test other UARTs, but I expect the
behaviour to be the same (even with 16550), at least on ARM, as nothing
polls the UART periodically.

Cheers,
Andre.

>      }
>  
>      /* Clear pending error interrupts */
> @@ -130,7 +131,7 @@ static int __init cuart_irq(struct serial_port *port)
>  {
>      struct cuart *uart = port->uart;
>  
> -    return ( (uart->irq > 0) ? uart->irq : -1 );
> +    return uart->irq;
>  }
>  
>  static const struct vuart_info *cuart_vuart(struct serial_port *port)
> diff --git a/xen/drivers/char/ns16550.c b/xen/drivers/char/ns16550.c
> index f32dbd3..ba50a1e 100644
> --- a/xen/drivers/char/ns16550.c
> +++ b/xen/drivers/char/ns16550.c
> @@ -714,18 +714,12 @@ static void __init ns16550_init_preirq(struct 
> serial_port *port)
>  
>  static void ns16550_setup_postirq(struct ns16550 *uart)
>  {
> -    if ( uart->irq > 0 )
> -    {
> -        /* Master interrupt enable; also keep DTR/RTS asserted. */
> -        ns_write_reg(uart,
> -                     UART_MCR, UART_MCR_OUT2 | UART_MCR_DTR | UART_MCR_RTS);
> -
> -        /* Enable receive interrupts. */
> -        ns_write_reg(uart, UART_IER, UART_IER_ERDAI);
> -    }
> +    /* Master interrupt enable; also keep DTR/RTS asserted. */
> +    ns_write_reg(uart, UART_MCR, UART_MCR_OUT2 | UART_MCR_DTR | 
> UART_MCR_RTS);
> +    /* Enable receive interrupts. */
> +    ns_write_reg(uart, UART_IER, UART_IER_ERDAI);
>  
> -    if ( uart->irq >= 0 )
> -        set_timer(&uart->timer, NOW() + MILLISECS(uart->timeout_ms));
> +    set_timer(&uart->timer, NOW() + MILLISECS(uart->timeout_ms));
>  }
>  
>  static void __init ns16550_init_postirq(struct serial_port *port)
> @@ -733,9 +727,6 @@ static void __init ns16550_init_postirq(struct 
> serial_port *port)
>      struct ns16550 *uart = port->uart;
>      int rc, bits;
>  
> -    if ( uart->irq < 0 )
> -        return;
> -
>      serial_async_transmit(port);
>  
>      init_timer(&uart->timer, ns16550_poll, port, 0);
> @@ -746,13 +737,14 @@ static void __init ns16550_init_postirq(struct 
> serial_port *port)
>      uart->timeout_ms = max_t(
>          unsigned int, 1, (bits * uart->fifo_size * 1000) / uart->baud);
>  
> -    if ( uart->irq > 0 )
> +    uart->irqaction.handler = ns16550_interrupt;
> +    uart->irqaction.name    = "ns16550";
> +    uart->irqaction.dev_id  = port;
> +
> +    if ( (rc = setup_irq(uart->irq, 0, &uart->irqaction)) != 0 )
>      {
> -        uart->irqaction.handler = ns16550_interrupt;
> -        uart->irqaction.name    = "ns16550";
> -        uart->irqaction.dev_id  = port;
> -        if ( (rc = setup_irq(uart->irq, 0, &uart->irqaction)) != 0 )
> -            printk("ERROR: Failed to allocate ns16550 IRQ %d\n", uart->irq);
> +        printk("ERROR: Failed to allocate ns16550 IRQ %d\n", uart->irq);
> +        return;
>      }
>  
>      ns16550_setup_postirq(uart);
> @@ -874,7 +866,8 @@ static void __init ns16550_endboot(struct serial_port 
> *port)
>  static int __init ns16550_irq(struct serial_port *port)
>  {
>      struct ns16550 *uart = port->uart;
> -    return ((uart->irq > 0) ? uart->irq : -1);
> +
> +    return uart->irq;
>  }
>  
>  static void ns16550_start_tx(struct serial_port *port)
> diff --git a/xen/drivers/char/omap-uart.c b/xen/drivers/char/omap-uart.c
> index d6a5d59..2ce4e71 100644
> --- a/xen/drivers/char/omap-uart.c
> +++ b/xen/drivers/char/omap-uart.c
> @@ -294,7 +294,7 @@ static int __init omap_uart_irq(struct serial_port *port)
>  {
>      struct omap_uart *uart = port->uart;
>  
> -    return ((uart->irq > 0) ? uart->irq : -1);
> +    return uart->irq;
>  }
>  
>  static const struct vuart_info *omap_vuart_info(struct serial_port *port)
> diff --git a/xen/drivers/char/pl011.c b/xen/drivers/char/pl011.c
> index be67242..e007918 100644
> --- a/xen/drivers/char/pl011.c
> +++ b/xen/drivers/char/pl011.c
> @@ -128,13 +128,14 @@ static void __init pl011_init_postirq(struct 
> serial_port *port)
>      struct pl011 *uart = port->uart;
>      int rc;
>  
> -    if ( uart->irq > 0 )
> +    uart->irqaction.handler = pl011_interrupt;
> +    uart->irqaction.name    = "pl011";
> +    uart->irqaction.dev_id  = port;
> +
> +    if ( (rc = setup_irq(uart->irq, 0, &uart->irqaction)) != 0 )
>      {
> -        uart->irqaction.handler = pl011_interrupt;
> -        uart->irqaction.name    = "pl011";
> -        uart->irqaction.dev_id  = port;
> -        if ( (rc = setup_irq(uart->irq, 0, &uart->irqaction)) != 0 )
> -            printk("ERROR: Failed to allocate pl011 IRQ %d\n", uart->irq);
> +        printk("ERROR: Failed to allocate pl011 IRQ %d\n", uart->irq);
> +        return;
>      }
>  
>      /* Clear pending error interrupts */
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel