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

[Oleksii <oleksii.kurochko@xxxxxxxxx>]

On Tue, 2023-07-18 at 16:40 +0200, Jan Beulich wrote:
> 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).
You are right it should matter what is the value of uart->irq in case
when polling mode is set.
> 
> > @@ -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.
Missed that it should be if (( uart->intr_works == polling ) || !uart-
>irq).
> 
> 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.
It think it would be better to pass 'uart_force_polling' ( or kind of )
via command line.

~ Oleksii