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

[Marek Marczykowski-Górecki <marmarek@xxxxxxxxxxxxxxxxxxxxxx>]

On Tue, May 17, 2022 at 05:46:07PM +0200, Jan Beulich wrote:
> 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

The whole section was wrapped in CONFIG_X86, so I haven't added it once
again. But if that's desirable, I can wrap the 0xff IRQ handling in yet
another CONFIG_X86 guard (since the spec says this value is x86
specific). I don't think having #error in non-x86 case makes much sense
here.

-- 
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab

Attachment: signature.asc
Description: PGP signature