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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 24.07.2023 16:27, Oleksii Kurochko wrote:
> --- a/xen/drivers/char/console.c
> +++ b/xen/drivers/char/console.c
> @@ -993,6 +993,8 @@ void __init console_init_preirq(void)
>  #endif
>          else if ( !strncmp(p, "none", 4) )
>              continue;
> +        else if ( !strncmp(p, "polling", 7 )
> +            continue;
>          else if ( (sh = serial_parse_handle(p)) >= 0 )
>          {
>              sercon_handle = sh;

Looks like you mean the new option to go under "console=". Besides
this being guesswork because of you not updating the command line
doc, this also is wrong, as the property then applies to all
consoles. What you mean is a control for a specific instance of a
16550 console, which can only possibly go under "com<N>=". I would
suggest to simply extend [<irq>|msi] there to [<irq>|msi|poll].

> @@ -595,7 +601,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 : -1;
>  }

Please don't corrupt previously correct style. You can keep things
almost like they were (albeit there's no strict need for any of the
parentheses):

    return ((uart->intr_works != polling) ? uart->irq : -1);

> @@ -1330,9 +1341,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->intr_works = polling;
> +                }
>  #endif
> -                if ( !uart->irq )
> +                if ( !uart->irq || (uart->intr_works == polling) )
>                      printk(XENLOG_INFO
>                             "ns16550: %pp: no legacy IRQ, using poll mode\n",
>                             &PCI_SBDF(0, b, d, f));

Message and code (after your addition) continue to be out of sync.
As said before, any condition that leads to polling mode wants to
find itself expressed by uart->intr_works set to "polling".

> @@ -1552,6 +1566,7 @@ static bool __init parse_positional(struct ns16550 
> *uart, char **str)
>              conf += 3;
>              uart->msi = true;
>              uart->irq = 0;
> +            uart->intr_works = polling;

How that? "msi" is specifically asking for interrupt driven mode,
just that the IRQ number isn't known yet. (Seeing this I notice that
parse_namevalue_pairs() offers no way of selecting MSI mode. But
that's not something you need to sort out.)

Jan