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.
