On Tue, Feb 01, 2022 at 12:40:29AM +0000, 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.
> kernel-parameters.txt confirms that pci=biosirq is intended to be supported
> only on X86-32.
>
> The new version tries to be more helpful by recommending changes to ACPI
> settings if appropriate, and only mentioning pci=biosirq (and the manual
> pirq= option) for kernels that support it. Additionally, it encourages
> the user to file bug reports so faulty firmware can be identified and
> potentially handled via known quirks in a future kernel release.
>
> ACPI is relevant to these warnings because it will significantly change
> the path taken through the PCI discovery (and later interrupt routing)
> code. Booting with acpi=noirq should be highly unusual and likely
> indicates an attempt to work around faulty motherboard firmware, so I
> added a new log message in pci_acpi_init() for this case, with yet
> another recommendation to file a bug report.
>
> Signed-off-by: Brent Spillner <spillner@xxxxxxx>
IIUC pirq_enable_irq() is only used for non-ACPI, non-DT, non-Xen,
non-Intel MID systems, so this is a real legacy path.
I don't think it's really worth cluttering an error case in a path
that should be rarely used in the first place.
Are you seeing a problem where you're getting the wrong error message
today? Can we just fix that problem instead so no kernel parameter is
needed in the first place?
> ---
> Changes in v2:
> - Tried to make the code more legible by reducing use of #ifdef (only
> used where required to guard reference to acpi_noirq)
> - The tradeoff is there's now an idiosyncratic use of do {...} while (0),
> but that lets me early-out from the acpi_noirq case without more #ifdefs
> or duplicated if statements.
> - Included a warning for acpi_noirq in pci_acpi_init() per maintainer suggestion
> - Encourage user to file bug reports in all warning messages
>
> arch/x86/pci/acpi.c | 4 +++-
> arch/x86/pci/irq.c | 22 +++++++++++++++++++---
> 2 files changed, 22 insertions(+), 4 deletions(-)
>
> diff --git a/arch/x86/pci/acpi.c b/arch/x86/pci/acpi.c
> index 052f1d78a562..12f894d345a9 100644
> --- a/arch/x86/pci/acpi.c
> +++ b/arch/x86/pci/acpi.c
> @@ -401,8 +401,10 @@ int __init pci_acpi_init(void)
> {
> struct pci_dev *dev = NULL;
>
> - if (acpi_noirq)
> + if (acpi_noirq) {
> + printk(KERN_WARNING "PCI: ACPI IRQ routing disabled; please submit a bug report if this was required to work around firmware defects\n");
> return -ENODEV;
> + }
>
> printk(KERN_INFO "PCI: Using ACPI for IRQ routing\n");
> acpi_irq_penalty_init();
> diff --git a/arch/x86/pci/irq.c b/arch/x86/pci/irq.c
> index 97b63e35e152..393b036e773b 100644
> --- a/arch/x86/pci/irq.c
> +++ b/arch/x86/pci/irq.c
> @@ -1519,10 +1519,26 @@ static int pirq_enable_irq(struct pci_dev *dev)
> } else
> msg = "; probably buggy MP table";
> #endif
> - } else if (pci_probe & PCI_BIOS_IRQ_SCAN)
> + } else if (pci_probe & PCI_BIOS_IRQ_SCAN) {
> msg = "";
> - else
> - msg = "; please try using pci=biosirq";
> + } else {
> + do { /* just one iteration; allows break to minimize code duplication */
> +#ifdef CONFIG_ACPI
> + if (acpi_noirq) {
> + msg = "; consider removing acpi=noirq, and file a bug report if that does not help";
> + break; /* out of remainder of one-iteration do {} loop */
> + }
> +#endif
> + if (IS_ENABLED(CONFIG_PCI_BIOS))
> + /* pci=biosirq and pirq= are valid only on x86_32, and should never be necessary */
> + msg = "; try using pci=biosirq or manual pirq=, and file a bug report for this device";
> + else if (!IS_ENABLED(CONFIG_ACPI))
> + /* ACPI will change code path through PCI subsystem, and is worth trying */
> + msg = "; try enabling ACPI if feasible, and file a bug report for this device";
> + else
> + msg = "; please file a bug report for failure to discover device IRQ via ACPI";
> + } while (0);
> + }
>
> /*
> * With IDE legacy devices the IRQ lookup failure is not
> --
> 2.35.1
>
