Re: [PATCH v2] ns16550: add support for polling mode when device tree is used

[Jan Beulich <jbeulich@xxxxxxxx>]

On 18.07.2023 16:13, Oleksii Kurochko wrote:
> --- a/xen/drivers/char/ns16550.c
> +++ b/xen/drivers/char/ns16550.c
> @@ -40,6 +40,8 @@
>  #include <asm/fixmap.h>
>  #endif
>  
> +#define NO_IRQ_POLL 0

Do you really need this? I ask because ...

> @@ -595,7 +603,9 @@ static void __init cf_check ns16550_endboot(struct 
> serial_port *port)
>  static int __init cf_check ns16550_irq(struct serial_port *port)
>  {
>      struct ns16550 *uart = port->uart;
> -    return ((uart->irq > 0) ? uart->irq : -1);
> +
> +    return (((uart->intr_works != polling) && (uart->irq >= 0)) ?

... you now use >= here, which includes that special value. As long
as intr_works is always set to "polling", the particular value in
uart->irq shouldn't matter (and hence you wouldn't need to store
anywhere that or any other special value).

> @@ -1330,9 +1340,12 @@ pci_uart_config(struct ns16550 *uart, bool_t skip_amt, 
> unsigned int idx)
>                   * as special only for X86.
>                   */
>                  if ( uart->irq == 0xff )
> -                    uart->irq = 0;
> +                {
> +                    uart->irq = NO_IRQ_POLL;
> +                    uart->intr_works = polling;
> +                }
>  #endif
> -                if ( !uart->irq )
> +                if ( uart->intr_works == polling )

Careful here - we may also have read 0 from PCI_INTERRUPT_LINE, or
forced 0 because we read 0 from PCI_INTERRUPT_PIN. All these cases,
unless provably broken, need to continue to function as they were.

Further you alter parse_positional(), but you leave alone
parse_namevalue_pairs(). I think you're changing the admin (command
line) interface that way, because so far "irq=0" was the way to
request polling. While it may be unavoidable to change that interface
(which will then need noting in ./CHANGELOG.md), you still need to
offer a way to forcibly set polling mode.

Jan