Re: [PATCH v3 1/2] ns16550: reject IRQ above nr_irqs_gsi

From: "Roger Pau Monné" <roger.pau@citrix.com>
To: "Marek Marczykowski-Górecki" <marmarek@invisiblethingslab.com>
Cc: xen-devel@lists.xenproject.org,
	Andrew Cooper <andrew.cooper3@citrix.com>,
	George Dunlap <george.dunlap@citrix.com>,
	Jan Beulich <jbeulich@suse.com>, Julien Grall <julien@xen.org>,
	Stefano Stabellini <sstabellini@kernel.org>, Wei Liu <wl@xen.org>
Subject: Re: [PATCH v3 1/2] ns16550: reject IRQ above nr_irqs_gsi
Date: Wed, 11 May 2022 09:20:46 +0200	[thread overview]
Message-ID: <YntjzqQl0CIeGzR4@Air-de-Roger> (raw)
In-Reply-To: <20220510155824.1779789-1-marmarek@invisiblethingslab.com>

Subject line needs to be updated :).

On Tue, May 10, 2022 at 05:58:23PM +0200, Marek Marczykowski-Górecki wrote:
> 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.
> 
> Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
> ---
> 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 | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/xen/drivers/char/ns16550.c b/xen/drivers/char/ns16550.c
> index fb75cee4a13a..b4434ad815e1 100644
> --- a/xen/drivers/char/ns16550.c
> +++ b/xen/drivers/char/ns16550.c
> @@ -1238,6 +1238,15 @@ 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 )
> +                {
> +                    printk(XENLOG_WARNING

XENLOG_INFO would be better, IMO this configuration is no reason to
warn the user.

> +                           "ns16550: %02x:%02x.%u has no legacy IRQ %d, "
> +                           "falling back to a poll mode\n",

Could you use %pp and then pass the parameter using &PCI_SBDF(0, b, d,
f)?

Also we try to avoid splitting printk format strings, what about
using:

"ns16550: %02x:%02x.%u no legacy IRQ, using poll mode\n"

TBH, we don't print a similar message if INTERRUPT_{PIN,LINE} is 0
(which also results in the console running in poll mode), so I wonder
if we should extend the printing of the message also to ->irq == 0 for
consistency.

Thanks, Roger.