On 17.05.2022 17:43, Roger Pau Monné wrote:
> 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.
Hmm, true. What I would really like (and what I should have spelled out)
is that the build would fail if this code was enabled for no-x86, such
that it ends up very obvious that something needs doing there. Hence ...
> In any case, I don't see an issue with adding an extra guard, albeit a
> comment would also be acceptable IMO.
... maybe
#ifdef CONFIG_X86
...
#else
# error
#endif
?
Jan
