[PATCH] arch:x86:pci:irq.c: Improve log message when IRQ cannot be identified

[PATCH] arch:x86:pci:irq.c: Improve log message when IRQ cannot be identified

[Brent Spillner]

The existing code always suggests trying the pci=biosirq kernel
parameter, but this option is only recognized when CONFIG_PCI_BIOS is
set, which in turn depends on CONFIG_X86_32, so it is never appropriate
on x86_64.

The new version tries to form a more useful message when pci=biosirq is
not available, including by suggesting different acpi= options if
appropriate (probably the most common cause of failed IRQ discovery).

See arch/x86/pci/common.c:535 for the interpretation of pci=biosirq, and
arch/x86/Kconfig:2633 for the dependencies of CONFIG_PCI_BIOS.

Signed-off-by: Brent Spillner <spillner@acm.org>
---
 arch/x86/pci/irq.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/arch/x86/pci/irq.c b/arch/x86/pci/irq.c
index 97b63e35e152..bc4aaaa74832 100644
--- a/arch/x86/pci/irq.c
+++ b/arch/x86/pci/irq.c
@@ -1522,7 +1522,21 @@ static int pirq_enable_irq(struct pci_dev *dev)
 		} else if (pci_probe & PCI_BIOS_IRQ_SCAN)
 			msg = "";
 		else
+#ifdef CONFIG_PCI_BIOS
 			msg = "; please try using pci=biosirq";
+#else
+			/* pci=biosirq is not a valid option */
+#ifdef CONFIG_ACPI
+			if (acpi_noirq)
+				msg = "; consider removing acpi=noirq";
+			else
+#endif
+				msg = "; recommend verifying UEFI/BIOS IRQ options"
+#ifndef CONFIG_ACPI
+					" or enabling ACPI"
+#endif
+					;
+#endif
 
 		/*
 		 * With IDE legacy devices the IRQ lookup failure is not
-- 
2.34.1

Re: [PATCH] arch:x86:pci:irq.c: Improve log message when IRQ cannot be identified

[Dave Hansen]

Please fix up that subject.  We don't tend to use ":" to separate
things.  Second, the prefix isn't a filename.  It's really a subsystem.
 Take a look at:

	git log arch/x86/pci/irq.c

for some other examples.

On 1/28/22 09:37, Brent Spillner wrote:
> The existing code always suggests trying the pci=biosirq kernel
> parameter, but this option is only recognized when CONFIG_PCI_BIOS is
> set, which in turn depends on CONFIG_X86_32, so it is never appropriate
> on x86_64.
> 
> The new version tries to form a more useful message when pci=biosirq is
> not available, including by suggesting different acpi= options if
> appropriate (probably the most common cause of failed IRQ discovery).
> 
> See arch/x86/pci/common.c:535 for the interpretation of pci=biosirq, and
> arch/x86/Kconfig:2633 for the dependencies of CONFIG_PCI_BIOS.

Shockingly enough, that parameter is in the documentation:

	Documentation/admin-guide/kernel-parameters.txt

and double-shockingly, it's even called out as X86-32-only:

	biosirq		[X86-32] Use PCI BIOS calls to get the interrupt

Given that, do we really need to refer to the line numbers of the
implementation which will probably be stale by the time this is merged
anyway?

>  arch/x86/pci/irq.c | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
> 
> diff --git a/arch/x86/pci/irq.c b/arch/x86/pci/irq.c
> index 97b63e35e152..bc4aaaa74832 100644
> --- a/arch/x86/pci/irq.c
> +++ b/arch/x86/pci/irq.c
> @@ -1522,7 +1522,21 @@ static int pirq_enable_irq(struct pci_dev *dev)
>  		} else if (pci_probe & PCI_BIOS_IRQ_SCAN)
>  			msg = "";
>  		else
> +#ifdef CONFIG_PCI_BIOS
>  			msg = "; please try using pci=biosirq";
> +#else
> +			/* pci=biosirq is not a valid option */
> +#ifdef CONFIG_ACPI
> +			if (acpi_noirq)
> +				msg = "; consider removing acpi=noirq";
> +			else
> +#endif
> +				msg = "; recommend verifying UEFI/BIOS IRQ options"
> +#ifndef CONFIG_ACPI
> +					" or enabling ACPI"
> +#endif
> +					;
> +#endif

Any chance you could make that, um, a bit more readable?  It's OK to add
brackets to the else{} for readability even if they're not *strictly*
necessary.

It might also be nice to use

	if (IS_ENABLED(CONFIG_FOO))
		...

rather than the #ifdefs.

I'd also be perfectly OK having two different strings rather than
relying on string concatenation and the #ifdefs.

Is the "or enabling ACPI" message really necessary?

Re: [PATCH] arch:x86:pci:irq.c: Improve log message when IRQ cannot be identified

[Brent Spillner]

On Fri, Jan 28, 2022 at 6:00 PM Dave Hansen <dave.hansen@intel.com> wrote:
> Shockingly enough, that parameter is in the documentation:
> and double-shockingly, it's even called out as X86-32-only:

Right, seeing that is what convinced me that not customizing the log
message for x86_64 could be considered a (admittedly very minor) bug,
and perhaps worth fixing.

> Given that, do we really need to refer to the line numbers of the
> implementation which will probably be stale by the time this is merged
> anyway?

Understood, will change the commit message to just refer to the
command line documentation.

> Any chance you could make that, um, a bit more readable?  It's OK to add
> brackets to the else{} for readability even if they're not *strictly*
> necessary.
>
> It might also be nice to use
>
>         if (IS_ENABLED(CONFIG_FOO))
>                 ...
>
> rather than the #ifdefs.

I don't mind doing either of those if that's the maintainer consensus,
but would note that neither would be consistent with the surrounding
code. Prior to the patch, the .c files in arch/x86/pci contain a total
of 33 #ifdefs and just one IS_ENABLED(), and systematically avoid
superfluous braces around single-statement if/else/for bodies.
Granted, the code has other style problems and triggers a number of
checkpatch.pl warnings (although not in the region affected by this
patch), but I was trying to be as light a touch as possible.

> I'd also be perfectly OK having two different strings rather than
> relying on string concatenation and the #ifdefs.
>
> Is the "or enabling ACPI" message really necessary?

Not strictly necessary--- it seems fair to assume that anyone
disabling ACPI does so intentionally and with good reason--- but I
thought it might stimulate the right thought process for someone who
doesn't understand why their hardware isn't being properly detected,
as ACPI triggers some very different code paths through this driver.

It seems like the multiline string literal is your main pain point--- would

+#ifdef CONFIG_ACPI
+                       if (acpi_noirq)
+                               msg = "; consider removing acpi=noirq";
+                       else
+                               msg = "; recommend verifying UEFI/BIOS
IRQ options";
+#else
+                       msg = "; recommend verifying UEFI/BIOS IRQ
options or enabling ACPI";
+#endif

be OK without going to IS_ENABLED()?  (Personally, I think the #ifdef
style is more readable.)

Re: [PATCH] arch:x86:pci:irq.c: Improve log message when IRQ cannot be identified

[Dave Hansen]

On 1/28/22 12:48, Brent Spillner wrote:
> It seems like the multiline string literal is your main pain point--- would
> 
> +#ifdef CONFIG_ACPI
> +                       if (acpi_noirq)
> +                               msg = "; consider removing acpi=noirq";
> +                       else
> +                               msg = "; recommend verifying UEFI/BIOS
> IRQ options";
> +#else
> +                       msg = "; recommend verifying UEFI/BIOS IRQ
> options or enabling ACPI";
> +#endif
> 
> be OK without going to IS_ENABLED()?  (Personally, I think the #ifdef
> style is more readable.)

I think that's _better_ than what was in the patch.  But, even with it,
I still think the #ifdef mess borders on unreadable.

But, if Bjorn likes it, then go for it. :)

Re: [PATCH] arch:x86:pci:irq.c: Improve log message when IRQ cannot be identified

[Bjorn Helgaas]

On Fri, Jan 28, 2022 at 01:36:53PM -0800, Dave Hansen wrote:
> On 1/28/22 12:48, Brent Spillner wrote:
> > It seems like the multiline string literal is your main pain point--- would
> > 
> > +#ifdef CONFIG_ACPI
> > +                       if (acpi_noirq)
> > +                               msg = "; consider removing acpi=noirq";
> > +                       else
> > +                               msg = "; recommend verifying UEFI/BIOS
> > IRQ options";
> > +#else
> > +                       msg = "; recommend verifying UEFI/BIOS IRQ
> > options or enabling ACPI";
> > +#endif
> > 
> > be OK without going to IS_ENABLED()?  (Personally, I think the #ifdef
> > style is more readable.)
> 
> I think that's _better_ than what was in the patch.  But, even with it,
> I still think the #ifdef mess borders on unreadable.
> 
> But, if Bjorn likes it, then go for it. :)

I was hoping to avoid commenting at all ;)

I'm a little bit averse to suggesting *any* command-line options
because users shouldn't have to use options like these.  It would be
better if we got a bug report and could fix the bug or add a quirk to
work around a firmware issue automatically.

If a user finds a command-line option that "works," the problem is
solved as far as they are concerned, but it doesn't help the next
person who trips over it.

I think pci_acpi_init() should warn when "acpi=noirq" was used because
the only reason to use it should be to work around a firmware defect,
and ideally, we could make a quirk to do that.  Maybe the doc should
suggest a bug report.

What does "verifying UEFI/BIOS IRQ options" mean?  I could go look at
the BIOS setup menu, but I would have no idea what to look for, so the
only thing I could do would be to try randomly changing IRQ-related
things.  If that happens to hit on a working configuration, I might
be happy, but it wouldn't help the next person at all.  I'd rather see
a bug report.  Then we could at least try to make a quirk so no
command line option would be needed.

Bjorn