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

[Oleksii <oleksii.kurochko@xxxxxxxxx>]

On Mon, 2023-07-31 at 15:24 +0200, Jan Beulich wrote:
> 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.
Just to confirm, do you mean that I should use parse_positional() ( or
something similar ) for the device-tree method of initializing ns16550?

I checked the polling behavior as described above because
parse_positional() is utilized solely for X86.

It appears that command line options are parsed only in the case of
x86.
> 
> Also may I remind you that extending command line options requires
> their
> doc to also be updated?
Thank you for the reminder. It seems I misunderstood which doc I need
to update. I believed you were referring to the comment above the
declaration of opt_com1[].

I think you meant xen-command-line.pandoc.
> 
> > @@ -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?
I think you are right, and I missed that. I'll update it in a new patch
version.

~ Oleksii