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

Commit Message

Marek Marczykowski-Górecki May 11, 2022, 2:30 p.m. UTC

Intel LPSS has INTERRUPT_LINE set to 0xff by default, that is declared
by the PCI Local Bus Specification Revision 3.0 (from 2004) as
"unknown"/"no connection". Fallback to poll mode in this case.
The 0xff handling is x86-specific, the surrounding code is guarded with
CONFIG_X86 anyway.

Signed-off-by: Marek Marczykowski-Górecki <[e-mail]>
Reviewed-by: Roger Pau Monné <[e-mail]>
---
Changes in v5:
 - drop IRQ 0 from the log message
Changes in v4:
 - adjust log message, change it from WARNING to INFO
 - re-add x86 reference in the commit message
Changes in v3:
 - change back to checking 0xff explicitly
 - adjust commit message, include spec reference
 - change warning to match the above
Changes in v2:
 - add log message
 - extend commit message
 - code style fix
---
 xen/drivers/char/ns16550.c | 7 +++++++
 1 file changed, 7 insertions(+)

Comments

Jan Beulich May 17, 2022, 2:41 p.m. UTC | #1

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.

Jan

Roger Pau Monné May 17, 2022, 2:48 p.m. UTC | #2

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.

Thanks, Roger.

Jan Beulich May 17, 2022, 3:13 p.m. UTC | #3

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.

Jan

Roger Pau Monné May 17, 2022, 3:43 p.m. UTC | #4

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.

Jan Beulich May 17, 2022, 3:46 p.m. UTC | #5

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

Marek Marczykowski-Górecki May 18, 2022, 5:48 p.m. UTC | #6

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.

Patch

diff --git a/xen/drivers/char/ns16550.c b/xen/drivers/char/ns16550.c
index fb75cee4a13a..417c07cde3dc 100644
--- 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;
             }
         }