Re: [PATCH] x86/quirks: Fix logic to apply quirk once

Lucas De Marchi <lucas.demarchi@xxxxxxxxx> · Wed, 29 Dec 2021 23:26:41 -0800

On Wed, Dec 29, 2021 at 05:27:52PM -0600, Bjorn Helgaas wrote:
On Fri, Dec 17, 2021 at 10:13:13PM -0800, Lucas De Marchi wrote:
When using QFLAG_APPLY_ONCE we make sure the quirk is applied only once.

Maybe "called" only once, since you're about to add a distinction
between "called" and "applied"?

ok... yeah, the current issue is that the logic here considers
"called" as "applied", and it's not true since the called function may
do additional checks


I'm not really sure the concept of QFLAG_APPLY_ONCE, QFLAG_APPLIED,
QFLAG_DONE is general purpose enough to be handled at the level of
check_dev_quirk().

We don't have anything like that for the regular PCI fixups (see
pci_do_fixups()).  If a regular fixup needed something like that, it
would use a static local variable.  Maybe that would be simpler
overall here, too, since the quirk would be *always* called for
matching devices, and the "one-time" logic would be encapsulated in
the quirk itself where it's more obvious?

so would that mean an alternative solution, removing all the logic using
flags and just embed it in the few quirks that make use of it?

yeah, I guess that would work too. Not sure what solution is preferred
though.

This is useful when it's enough one device to trigger a certain
condition or when the resource in each that applies is global to the
system rather than local to the device.

However we call the quirk handler based on vendor, class, and device,
allowing the specific handler to do additional filtering. In that case
check_dev_quirk() may incorrectly mark the quirk as applied when it's
not. This is particularly bad for intel_graphics_quirks() that uses
PCI_ANY_ID and then compares with a long list of devices. This hasn't
been problematic so far because those devices are integrated GPUs and
there can only be one in the system.  However as Intel starts to
release discrete cards, this condition is no longer true and we fail to
reserve the stolen memory (for the integrated gpu) depending on the bus
topology: if the traversal finds the discrete card first, for which
there is no system stolen memory, we will fail to reserve it for the
integrated card.

s/integrated gpu/integrated GPU/ (to match previous use)

This fixes the stolen memory reservation for an Alderlake-P system with
one additional DG2. In this system we have:

DG2?

that is its name, not an abbreviation. It's one of Intel's discrete
graphics cards. See for example 9e22cfc5e9b9 ("drm/i915/dg2: add DG2 platform info")

maybe reword this as "one additional Intel GPU, DG2"?


	- 00:01.0 Bridge
	  `- 03:00.0 DG2
	- Alderklake-P's integrated graphics

s/Alderklake-P/Alderlake-P/

Might be nice to include the integrated GPU PCI address to be parallel
with the bridge and DG2.

ok


Since we do a depth-first traversal, when we call the handler because of
DG2 we were marking it as already being applied and never reserving the
stolen memory for Alderlake-P.

Here we change the quirk fucntions to return bool in case it applied a
quirk so we only flag it as applied when that really happened. This only
makes a difference for quirks using QFLAG_APPLY_ONCE, so all the others
simply returns true in order to avoid unnecessary complication.

s/fucntions/functions/
s/returns true/return true/

I would consider splitting this into two patches:

 1) Change the quirk signature, make them all return "true", and
 update check_dev_quirk().  This would have no functional impact.

 2) Update intel_graphics_quirks() to return "false" when it doesn't
 reserve the stolen memory.

Then the important change will be in a small patch by itself and will
be easier to understand and revert if that should be necessary.

makes sense. I will take a look on what the other alternative you gave
looks like, too.

thanks for the review

Lucas De Marchi


Signed-off-by: Lucas De Marchi <lucas.demarchi@xxxxxxxxx>
---
 arch/x86/kernel/early-quirks.c | 75 ++++++++++++++++++++++------------
 1 file changed, 49 insertions(+), 26 deletions(-)

diff --git a/arch/x86/kernel/early-quirks.c b/arch/x86/kernel/early-quirks.c
index 391a4e2b8604..5d235fe2a07a 100644
--- a/arch/x86/kernel/early-quirks.c
+++ b/arch/x86/kernel/early-quirks.c
@@ -28,7 +28,7 @@
 #include <asm/irq_remapping.h>
 #include <asm/early_ioremap.h>

-static void __init fix_hypertransport_config(int num, int slot, int func)
+static bool __init fix_hypertransport_config(int num, int slot, int func)
 {
 	u32 htcfg;
 	/*
@@ -51,10 +51,10 @@ static void __init fix_hypertransport_config(int num, int slot, int func)
 		}
 	}

-
+	return true;
 }

-static void __init via_bugs(int  num, int slot, int func)
+static bool __init via_bugs(int  num, int slot, int func)
 {
 #ifdef CONFIG_GART_IOMMU
 	if ((max_pfn > MAX_DMA32_PFN ||  force_iommu) &&
@@ -63,8 +63,12 @@ static void __init via_bugs(int  num, int slot, int func)
 		       "Looks like a VIA chipset. Disabling IOMMU."
 		       " Override with iommu=allowed\n");
 		gart_iommu_aperture_disabled = 1;
+
+		return true;
 	}
 #endif
+
+	return false;
 }

 #ifdef CONFIG_ACPI
@@ -77,7 +81,7 @@ static int __init nvidia_hpet_check(struct acpi_table_header *header)
 #endif /* CONFIG_X86_IO_APIC */
 #endif /* CONFIG_ACPI */

-static void __init nvidia_bugs(int num, int slot, int func)
+static bool __init nvidia_bugs(int num, int slot, int func)
 {
 #ifdef CONFIG_ACPI
 #ifdef CONFIG_X86_IO_APIC
@@ -86,7 +90,7 @@ static void __init nvidia_bugs(int num, int slot, int func)
 	 * Nvidia graphics cards with PCI ports on secondary buses.
 	 */
 	if (num)
-		return;
+		return false;

 	/*
 	 * All timer overrides on Nvidia are
@@ -96,7 +100,7 @@ static void __init nvidia_bugs(int num, int slot, int func)
 	 * at least allow a command line override.
 	 */
 	if (acpi_use_timer_override)
-		return;
+		return false;

 	if (acpi_table_parse(ACPI_SIG_HPET, nvidia_hpet_check)) {
 		acpi_skip_timer_override = 1;
@@ -105,11 +109,14 @@ static void __init nvidia_bugs(int num, int slot, int func)
 		       "timer override.\n");
 		printk(KERN_INFO "If you got timer trouble "
 			"try acpi_use_timer_override\n");
+
+		return true;
 	}
 #endif
 #endif
 	/* RED-PEN skip them on mptables too? */

+	return false;
 }

 #if defined(CONFIG_ACPI) && defined(CONFIG_X86_IO_APIC)
@@ -131,13 +138,13 @@ static u32 __init ati_ixp4x0_rev(int num, int slot, int func)
 	return d;
 }

-static void __init ati_bugs(int num, int slot, int func)
+static bool __init ati_bugs(int num, int slot, int func)
 {
 	u32 d;
 	u8  b;

 	if (acpi_use_timer_override)
-		return;
+		return true;

 	d = ati_ixp4x0_rev(num, slot, func);
 	if (d  < 0x82)
@@ -155,6 +162,8 @@ static void __init ati_bugs(int num, int slot, int func)
 		printk(KERN_INFO "If you got timer trouble "
 		       "try acpi_use_timer_override\n");
 	}
+
+	return true;
 }

 static u32 __init ati_sbx00_rev(int num, int slot, int func)
@@ -167,7 +176,7 @@ static u32 __init ati_sbx00_rev(int num, int slot, int func)
 	return d;
 }

-static void __init ati_bugs_contd(int num, int slot, int func)
+static bool __init ati_bugs_contd(int num, int slot, int func)
 {
 	u32 d, rev;

@@ -181,10 +190,10 @@ static void __init ati_bugs_contd(int num, int slot, int func)
 	 * SB800: revisions 0x40, 0x41, ...
 	 */
 	if (rev >= 0x39)
-		return;
+		return true;

 	if (acpi_use_timer_override)
-		return;
+		return true;

 	/* check for IRQ0 interrupt swap */
 	d = read_pci_config(num, slot, func, 0x64);
@@ -197,18 +206,22 @@ static void __init ati_bugs_contd(int num, int slot, int func)
 		printk(KERN_INFO "If you got timer trouble "
 		       "try acpi_use_timer_override\n");
 	}
+
+	return true;
 }
 #else
-static void __init ati_bugs(int num, int slot, int func)
+static bool __init ati_bugs(int num, int slot, int func)
 {
+	return true;
 }

-static void __init ati_bugs_contd(int num, int slot, int func)
+static bool __init ati_bugs_contd(int num, int slot, int func)
 {
+	return true;
 }
 #endif

-static void __init intel_remapping_check(int num, int slot, int func)
+static bool __init intel_remapping_check(int num, int slot, int func)
 {
 	u8 revision;
 	u16 device;
@@ -226,6 +239,8 @@ static void __init intel_remapping_check(int num, int slot, int func)
 		set_irq_remapping_broken();
 	else if (device == 0x3405 && revision == 0x22)
 		set_irq_remapping_broken();
+
+	return true;
 }

 /*
@@ -585,7 +600,7 @@ intel_graphics_stolen(int num, int slot, int func,
 	e820__update_table(e820_table);
 }

-static void __init intel_graphics_quirks(int num, int slot, int func)
+static bool __init intel_graphics_quirks(int num, int slot, int func)
 {
 	const struct intel_early_ops *early_ops;
 	u16 device;
@@ -603,16 +618,20 @@ static void __init intel_graphics_quirks(int num, int slot, int func)

 		intel_graphics_stolen(num, slot, func, early_ops);

-		return;
+		return true;
 	}
+
+	return false;
 }

-static void __init force_disable_hpet(int num, int slot, int func)
+static bool __init force_disable_hpet(int num, int slot, int func)
 {
 #ifdef CONFIG_HPET_TIMER
 	boot_hpet_disable = true;
 	pr_info("x86/hpet: Will disable the HPET for this platform because it's not reliable\n");
 #endif
+
+	return true;
 }

 #define BCM4331_MMIO_SIZE	16384
@@ -620,7 +639,7 @@ static void __init force_disable_hpet(int num, int slot, int func)
 #define bcma_aread32(reg)	ioread32(mmio + 1 * BCMA_CORE_SIZE + reg)
 #define bcma_awrite32(reg, val)	iowrite32(val, mmio + 1 * BCMA_CORE_SIZE + reg)

-static void __init apple_airport_reset(int bus, int slot, int func)
+static bool __init apple_airport_reset(int bus, int slot, int func)
 {
 	void __iomem *mmio;
 	u16 pmcsr;
@@ -628,7 +647,7 @@ static void __init apple_airport_reset(int bus, int slot, int func)
 	int i;

 	if (!x86_apple_machine)
-		return;
+		return true;

 	/* Card may have been put into PCI_D3hot by grub quirk */
 	pmcsr = read_pci_config_16(bus, slot, func, BCM4331_PM_CAP + PCI_PM_CTRL);
@@ -642,7 +661,7 @@ static void __init apple_airport_reset(int bus, int slot, int func)
 		if ((pmcsr & PCI_PM_CTRL_STATE_MASK) != PCI_D0) {
 			pr_err("pci 0000:%02x:%02x.%d: Cannot power up Apple AirPort card\n",
 			       bus, slot, func);
-			return;
+			return true;
 		}
 	}

@@ -654,7 +673,7 @@ static void __init apple_airport_reset(int bus, int slot, int func)
 	if (!mmio) {
 		pr_err("pci 0000:%02x:%02x.%d: Cannot iomap Apple AirPort card\n",
 		       bus, slot, func);
-		return;
+		return true;
 	}

 	pr_info("Resetting Apple AirPort card (left enabled by EFI)\n");
@@ -671,6 +690,8 @@ static void __init apple_airport_reset(int bus, int slot, int func)
 	udelay(10);

 	early_iounmap(mmio, BCM4331_MMIO_SIZE);
+
+	return true;
 }

 #define QFLAG_APPLY_ONCE 	0x1
@@ -682,7 +703,7 @@ struct chipset {
 	u32 class;
 	u32 class_mask;
 	u32 flags;
-	void (*f)(int num, int slot, int func);
+	bool (*f)(int num, int slot, int func);
 };

 static struct chipset early_qrk[] __initdata = {
@@ -757,11 +778,13 @@ static int __init check_dev_quirk(int num, int slot, int func)
 			(early_qrk[i].device == device)) &&
 			(!((early_qrk[i].class ^ class) &
 			    early_qrk[i].class_mask))) {
-				if ((early_qrk[i].flags &
-				     QFLAG_DONE) != QFLAG_DONE)
-					early_qrk[i].f(num, slot, func);
-				early_qrk[i].flags |= QFLAG_APPLIED;
+			if ((early_qrk[i].flags & QFLAG_DONE) != QFLAG_DONE) {
+				bool applied = early_qrk[i].f(num, slot, func);
+
+				if (applied)
+					early_qrk[i].flags |= QFLAG_APPLIED;
 			}
+		}
 	}

 	type = read_pci_config_byte(num, slot, func,
--
2.34.1







