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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 27.07.2023 18:45, Oleksii Kurochko wrote:
> @@ -654,6 +674,9 @@ static void ns16550_init_common(struct ns16550 *uart)
>  
>      /* Default lsr_mask = UART_LSR_THRE */
>      uart->lsr_mask  = UART_LSR_THRE;
> +
> +    if ( strstr(opt_com1, "poll") || strstr(opt_com2, "poll") )
> +        uart->intr_works = polling;
>  }

A non-__init function may not reference __initdata objects. But strstr()
is too lax anyway, and you also shouldn't check the wrong port's options.
You want to recognize "poll" _only_ where all other command line options
are processed.

Also may I remind you that extending command line options requires their
doc to also be updated?

> @@ -1333,9 +1356,13 @@ pci_uart_config(struct ns16550 *uart, bool_t skip_amt, 
> unsigned int idx)
>                      uart->irq = 0;
>  #endif
>                  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));
> +                }

I'm okay to leave it at this for now, since this way at least nothing
regresses that was working before. I'm not convinced this is all
correct, but that's a largely separate (and pre-existing) issue then.

> @@ -1791,8 +1808,11 @@ static int __init ns16550_uart_dt_init(struct 
> dt_device_node *dev,
>      }
>  
>      res = platform_get_irq(dev, 0);
> -    if ( ! res )
> -        return -EINVAL;
> +    if ( res < 0 )
> +    {
> +        printk("there is no interrupt property, polling will be used\n");
> +        uart->intr_works = polling;
> +    }
>      uart->irq = res;

Shouldn't you avoid writing uart->irq when res is negative?

Jan