Re: [PATCH v5 1/2] ns16550: use poll mode if INTERRUPT_LINE is 0xff

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Tue, May 17, 2022 at 05:13:46PM +0200, Jan Beulich wrote:
> On 17.05.2022 16:48, Roger Pau Monné wrote:
> > On Tue, May 17, 2022 at 04:41:31PM +0200, Jan Beulich wrote:
> >> On 11.05.2022 16:30, Marek Marczykowski-Górecki wrote:
> >>> --- a/xen/drivers/char/ns16550.c
> >>> +++ b/xen/drivers/char/ns16550.c
> >>> @@ -1238,6 +1238,13 @@ pci_uart_config(struct ns16550 *uart, bool_t 
> >>> skip_amt, unsigned int idx)
> >>>                              pci_conf_read8(PCI_SBDF(0, b, d, f),
> >>>                                             PCI_INTERRUPT_LINE) : 0;
> >>>  
> >>> +                if ( uart->irq == 0xff )
> >>> +                    uart->irq = 0;
> >>> +                if ( !uart->irq )
> >>> +                    printk(XENLOG_INFO
> >>> +                           "ns16550: %pp no legacy IRQ, using poll 
> >>> mode\n",
> >>> +                           &PCI_SBDF(0, b, d, f));
> >>> +
> >>>                  return 0;
> >>>              }
> >>>          }
> >>
> >> While this code is inside a CONFIG_HAS_PCI conditional, I still
> >> think - as was previously suggested - that the 1st if() should be
> >> inside a CONFIG_X86 conditional, to not leave a trap for other
> >> architectures to fall into.
> > 
> > The CONFIG_HAS_PCI region is itself inside of a (bigger) CONFIG_X86
> > region already.
> 
> But that's likely to change sooner or later, I expect. I'd rather see
> the surrounding region be shrunk in scope. Already when that
> CONFIG_X86 was introduced I had reservations, as I don't think all of
> the enclosed code really is x86-specific.

My though was that anyone removing the CONFIG_X86 guard will already
have to deal with setting ->irq properly, as I expect this will differ
between arches, at which point the check are likely to diverge anyway.

In any case, I don't see an issue with adding an extra guard, albeit a
comment would also be acceptable IMO.

Thanks, Roger.