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

[Oleksii <oleksii.kurochko@xxxxxxxxx>]

On Mon, 2023-07-24 at 16:43 +0200, Jan Beulich wrote:
> 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].
Thanks. It would be definitely better to go under "com<N>"
> 
> > @@ -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);
Thanks.
> 
> > @@ -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".
Well. It looks like it would be better to set 'uart->intr_works =
polling' inside if ( !uart->irq ):
  if ( !uart->irq || (uart->intr_works == polling) )
  {
      uart->intr_works = polling;
      printk(XENLOG_INFO
             "ns16550: %pp: using poll mode\n",
             &PCI_SBDF(0, b, d, f));
  }
Then "uart->intr_works = polling;" can be removed from "if ( uart->irq
== 0xff )" as in that case 'uart->irq = 0' means polling mode for X86.
  
> 
> > @@ -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.)
I was confused by the code from ns16550_init_postirq():
                if ( rc )
                {
                    uart->irq = 0;
                    if ( msi_desc )
                        msi_free_irq(msi_desc);
                    else
                        destroy_irq(msi.irq);
                }
where "uart->irq = 0;" means that polling mode should be used because
of the following code in the same function:
    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 )
            printk("ERROR: Failed to allocate ns16550 IRQ %d\n", uart-
>irq);
    }

~ Oleksii