[PATCH v2 1/2] x86/quirks: Fix logic to apply quirk once

[PATCH v2 1/2] x86/quirks: Fix logic to apply quirk once

[Lucas De Marchi]

When using QFLAG_APPLY_ONCE we make sure the quirk is called only once.
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: the quirk was called, but may not have been applied due to the
additional filter.

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.

This fixes the stolen memory reservation for an Alderlake-P system with
one additional Intel discrete GPU (DG2 in this case, but applies for
any of them). In this system we have:

	- 00:01.0 Bridge
	  `- 03:00.0 DG2
	- 00:02.0 Alderlake-P's integrated GPU

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.

Since there are just a few quirks using the QFLAG_APPLY_ONCE logic and
that is even the only flag, just use a static local variable in the
quirk function itself. This allows to mark the quirk as applied only
when it really is. As pointed out by Bjorn Helgaas, this is also more in
line with the PCI fixups as done by pci_do_fixups().

Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
---

v2: instead of changing all quirks to return if it was successfully
applied, remove the flag infra and use a static local variable to mark
quirks already applied (suggested by Bjorn Helgaas).

 arch/x86/kernel/early-quirks.c | 60 ++++++++++++++++++++--------------
 1 file changed, 36 insertions(+), 24 deletions(-)

diff --git a/arch/x86/kernel/early-quirks.c b/arch/x86/kernel/early-quirks.c
index 391a4e2b8604..102ecd0a910e 100644
--- a/arch/x86/kernel/early-quirks.c
+++ b/arch/x86/kernel/early-quirks.c
@@ -57,12 +57,18 @@ static void __init fix_hypertransport_config(int num, int slot, int func)
 static void __init via_bugs(int  num, int slot, int func)
 {
 #ifdef CONFIG_GART_IOMMU
+	static bool quirk_applied __initdata;
+
+	if (quirk_applied)
+		return;
+
 	if ((max_pfn > MAX_DMA32_PFN ||  force_iommu) &&
 	    !gart_iommu_aperture_allowed) {
 		printk(KERN_INFO
 		       "Looks like a VIA chipset. Disabling IOMMU."
 		       " Override with iommu=allowed\n");
 		gart_iommu_aperture_disabled = 1;
+		quirk_applied = true;
 	}
 #endif
 }
@@ -81,6 +87,11 @@ static void __init nvidia_bugs(int num, int slot, int func)
 {
 #ifdef CONFIG_ACPI
 #ifdef CONFIG_X86_IO_APIC
+	static bool quirk_applied __initdata;
+
+	if (quirk_applied)
+		return;
+
 	/*
 	 * Only applies to Nvidia root ports (bus 0) and not to
 	 * Nvidia graphics cards with PCI ports on secondary buses.
@@ -105,6 +116,7 @@ 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");
+		quirk_applied = true;
 	}
 #endif
 #endif
@@ -559,7 +571,7 @@ static const struct pci_device_id intel_early_ids[] __initconst = {
 struct resource intel_graphics_stolen_res __ro_after_init = DEFINE_RES_MEM(0, 0);
 EXPORT_SYMBOL(intel_graphics_stolen_res);
 
-static void __init
+static bool __init
 intel_graphics_stolen(int num, int slot, int func,
 		      const struct intel_early_ops *early_ops)
 {
@@ -570,7 +582,7 @@ intel_graphics_stolen(int num, int slot, int func,
 	base = early_ops->stolen_base(num, slot, func, size);
 
 	if (!size || !base)
-		return;
+		return false;
 
 	end = base + size - 1;
 
@@ -583,14 +595,20 @@ intel_graphics_stolen(int num, int slot, int func,
 	/* Mark this space as reserved */
 	e820__range_add(base, size, E820_TYPE_RESERVED);
 	e820__update_table(e820_table);
+
+	return true;
 }
 
 static void __init intel_graphics_quirks(int num, int slot, int func)
 {
+	static bool quirk_applied __initdata;
 	const struct intel_early_ops *early_ops;
 	u16 device;
 	int i;
 
+	if (quirk_applied)
+		return;
+
 	device = read_pci_config_16(num, slot, func, PCI_DEVICE_ID);
 
 	for (i = 0; i < ARRAY_SIZE(intel_early_ids); i++) {
@@ -601,7 +619,8 @@ static void __init intel_graphics_quirks(int num, int slot, int func)
 
 		early_ops = (typeof(early_ops))driver_data;
 
-		intel_graphics_stolen(num, slot, func, early_ops);
+		quirk_applied = intel_graphics_stolen(num, slot, func,
+						      early_ops);
 
 		return;
 	}
@@ -673,37 +692,33 @@ static void __init apple_airport_reset(int bus, int slot, int func)
 	early_iounmap(mmio, BCM4331_MMIO_SIZE);
 }
 
-#define QFLAG_APPLY_ONCE 	0x1
-#define QFLAG_APPLIED		0x2
-#define QFLAG_DONE		(QFLAG_APPLY_ONCE|QFLAG_APPLIED)
 struct chipset {
 	u32 vendor;
 	u32 device;
 	u32 class;
 	u32 class_mask;
-	u32 flags;
 	void (*f)(int num, int slot, int func);
 };
 
 static struct chipset early_qrk[] __initdata = {
 	{ PCI_VENDOR_ID_NVIDIA, PCI_ANY_ID,
-	  PCI_CLASS_BRIDGE_PCI, PCI_ANY_ID, QFLAG_APPLY_ONCE, nvidia_bugs },
+	  PCI_CLASS_BRIDGE_PCI, PCI_ANY_ID, nvidia_bugs },
 	{ PCI_VENDOR_ID_VIA, PCI_ANY_ID,
-	  PCI_CLASS_BRIDGE_PCI, PCI_ANY_ID, QFLAG_APPLY_ONCE, via_bugs },
+	  PCI_CLASS_BRIDGE_PCI, PCI_ANY_ID, via_bugs },
 	{ PCI_VENDOR_ID_AMD, PCI_DEVICE_ID_AMD_K8_NB,
-	  PCI_CLASS_BRIDGE_HOST, PCI_ANY_ID, 0, fix_hypertransport_config },
+	  PCI_CLASS_BRIDGE_HOST, PCI_ANY_ID, fix_hypertransport_config },
 	{ PCI_VENDOR_ID_ATI, PCI_DEVICE_ID_ATI_IXP400_SMBUS,
-	  PCI_CLASS_SERIAL_SMBUS, PCI_ANY_ID, 0, ati_bugs },
+	  PCI_CLASS_SERIAL_SMBUS, PCI_ANY_ID, ati_bugs },
 	{ PCI_VENDOR_ID_ATI, PCI_DEVICE_ID_ATI_SBX00_SMBUS,
-	  PCI_CLASS_SERIAL_SMBUS, PCI_ANY_ID, 0, ati_bugs_contd },
+	  PCI_CLASS_SERIAL_SMBUS, PCI_ANY_ID, ati_bugs_contd },
 	{ PCI_VENDOR_ID_INTEL, 0x3403, PCI_CLASS_BRIDGE_HOST,
-	  PCI_BASE_CLASS_BRIDGE, 0, intel_remapping_check },
+	  PCI_BASE_CLASS_BRIDGE, intel_remapping_check },
 	{ PCI_VENDOR_ID_INTEL, 0x3405, PCI_CLASS_BRIDGE_HOST,
-	  PCI_BASE_CLASS_BRIDGE, 0, intel_remapping_check },
+	  PCI_BASE_CLASS_BRIDGE, intel_remapping_check },
 	{ PCI_VENDOR_ID_INTEL, 0x3406, PCI_CLASS_BRIDGE_HOST,
-	  PCI_BASE_CLASS_BRIDGE, 0, intel_remapping_check },
+	  PCI_BASE_CLASS_BRIDGE, intel_remapping_check },
 	{ PCI_VENDOR_ID_INTEL, PCI_ANY_ID, PCI_CLASS_DISPLAY_VGA, PCI_ANY_ID,
-	  QFLAG_APPLY_ONCE, intel_graphics_quirks },
+	  intel_graphics_quirks },
 	/*
 	 * HPET on the current version of the Baytrail platform has accuracy
 	 * problems: it will halt in deep idle state - so we disable it.
@@ -713,9 +728,9 @@ static struct chipset early_qrk[] __initdata = {
 	 *    http://www.intel.com/content/dam/www/public/us/en/documents/datasheets/atom-z8000-datasheet-vol-1.pdf
 	 */
 	{ PCI_VENDOR_ID_INTEL, 0x0f00,
-		PCI_CLASS_BRIDGE_HOST, PCI_ANY_ID, 0, force_disable_hpet},
+		PCI_CLASS_BRIDGE_HOST, PCI_ANY_ID, force_disable_hpet},
 	{ PCI_VENDOR_ID_BROADCOM, 0x4331,
-	  PCI_CLASS_NETWORK_OTHER, PCI_ANY_ID, 0, apple_airport_reset},
+	  PCI_CLASS_NETWORK_OTHER, PCI_ANY_ID, apple_airport_reset},
 	{}
 };
 
@@ -756,12 +771,9 @@ static int __init check_dev_quirk(int num, int slot, int func)
 			((early_qrk[i].device == PCI_ANY_ID) ||
 			(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;
-			}
+			    early_qrk[i].class_mask)))
+				early_qrk[i].f(num, slot, func);
+
 	}
 
 	type = read_pci_config_byte(num, slot, func,

base-commit: c9e6606c7fe92b50a02ce51dda82586ebdf99b48
-- 
2.34.1

[PATCH v2 2/2] x86/quirks: better wrap quirk conditions

[Lucas De Marchi]

Remove extra parenthesis and wrap lines so it's easier to read what are
the conditions being checked. The call to the hook also had an extra
indentation: remove here to conform to coding style.

Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
---
 arch/x86/kernel/early-quirks.c | 14 ++++++--------
 1 file changed, 6 insertions(+), 8 deletions(-)

diff --git a/arch/x86/kernel/early-quirks.c b/arch/x86/kernel/early-quirks.c
index 102ecd0a910e..03256f802aba 100644
--- a/arch/x86/kernel/early-quirks.c
+++ b/arch/x86/kernel/early-quirks.c
@@ -766,14 +766,12 @@ static int __init check_dev_quirk(int num, int slot, int func)
 	device = read_pci_config_16(num, slot, func, PCI_DEVICE_ID);
 
 	for (i = 0; early_qrk[i].f != NULL; i++) {
-		if (((early_qrk[i].vendor == PCI_ANY_ID) ||
-			(early_qrk[i].vendor == vendor)) &&
-			((early_qrk[i].device == PCI_ANY_ID) ||
-			(early_qrk[i].device == device)) &&
-			(!((early_qrk[i].class ^ class) &
-			    early_qrk[i].class_mask)))
-				early_qrk[i].f(num, slot, func);
-
+		if ((early_qrk[i].vendor == PCI_ANY_ID ||
+		     early_qrk[i].vendor == vendor) &&
+		    (early_qrk[i].device == PCI_ANY_ID ||
+		     early_qrk[i].device == device) &&
+		    !((early_qrk[i].class ^ class) & early_qrk[i].class_mask))
+			early_qrk[i].f(num, slot, func);
 	}
 
 	type = read_pci_config_byte(num, slot, func,
-- 
2.34.1

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

[Bjorn Helgaas]

On Wed, Jan 05, 2022 at 04:36:53PM -0800, Lucas De Marchi wrote:
> When using QFLAG_APPLY_ONCE we make sure the quirk is called only once.
> 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: the quirk was called, but may not have been applied due to the
> additional filter.
> 
> 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.
> 
> This fixes the stolen memory reservation for an Alderlake-P system with
> one additional Intel discrete GPU (DG2 in this case, but applies for
> any of them). In this system we have:
> 
> 	- 00:01.0 Bridge
> 	  `- 03:00.0 DG2
> 	- 00:02.0 Alderlake-P's integrated GPU
> 
> 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.
> 
> Since there are just a few quirks using the QFLAG_APPLY_ONCE logic and
> that is even the only flag, just use a static local variable in the
> quirk function itself. This allows to mark the quirk as applied only
> when it really is. As pointed out by Bjorn Helgaas, this is also more in
> line with the PCI fixups as done by pci_do_fixups().
> 
> Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
> ---
> 
> v2: instead of changing all quirks to return if it was successfully
> applied, remove the flag infra and use a static local variable to mark
> quirks already applied (suggested by Bjorn Helgaas).
> 
>  arch/x86/kernel/early-quirks.c | 60 ++++++++++++++++++++--------------
>  1 file changed, 36 insertions(+), 24 deletions(-)
> 
> diff --git a/arch/x86/kernel/early-quirks.c b/arch/x86/kernel/early-quirks.c
> index 391a4e2b8604..102ecd0a910e 100644
> --- a/arch/x86/kernel/early-quirks.c
> +++ b/arch/x86/kernel/early-quirks.c
> @@ -57,12 +57,18 @@ static void __init fix_hypertransport_config(int num, int slot, int func)
>  static void __init via_bugs(int  num, int slot, int func)
>  {
>  #ifdef CONFIG_GART_IOMMU
> +	static bool quirk_applied __initdata;
> +
> +	if (quirk_applied)
> +		return;

IMO this probably is better than using QFLAG_APPLY_ONCE, etc.

But this patch has the mechanical changes related to QFLAG_APPLY_ONCE,
which I think are useful but not very interesting and not a fix for
something that's broken, mixed together with the *important* change
that actually fixes a problem on Alderlake.

Those two things need to be separate patches.  A patch that fixes a
problem should be as small as possible so it's easy to understand and
backport.

The subject line of this patch doesn't say anything at all about
Alderlake.  Splitting into two patches will make the subject lines
more useful.

Bjorn

Re: [PATCH v2 2/2] x86/quirks: better wrap quirk conditions

[Bjorn Helgaas]

On Wed, Jan 05, 2022 at 04:36:54PM -0800, Lucas De Marchi wrote:
> Remove extra parenthesis and wrap lines so it's easier to read what are
> the conditions being checked. The call to the hook also had an extra
> indentation: remove here to conform to coding style.

It's nice when your subject lines are consistent.  These look like:

  x86/quirks: Fix logic to apply quirk once
  x86/quirks: better wrap quirk conditions

The second isn't capitalized like the first.  Obviously if you split
the first patch, you'll have three subject lines, and one will mention
Alderlake.

Bjorn

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

[Lucas De Marchi]

On Thu, Jan 06, 2022 at 03:58:50PM -0600, Bjorn Helgaas wrote:
>On Wed, Jan 05, 2022 at 04:36:53PM -0800, Lucas De Marchi wrote:
>> When using QFLAG_APPLY_ONCE we make sure the quirk is called only once.
>> 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: the quirk was called, but may not have been applied due to the
>> additional filter.
>>
>> 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.
>>
>> This fixes the stolen memory reservation for an Alderlake-P system with
>> one additional Intel discrete GPU (DG2 in this case, but applies for
>> any of them). In this system we have:
>>
>> 	- 00:01.0 Bridge
>> 	  `- 03:00.0 DG2
>> 	- 00:02.0 Alderlake-P's integrated GPU
>>
>> 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.
>>
>> Since there are just a few quirks using the QFLAG_APPLY_ONCE logic and
>> that is even the only flag, just use a static local variable in the
>> quirk function itself. This allows to mark the quirk as applied only
>> when it really is. As pointed out by Bjorn Helgaas, this is also more in
>> line with the PCI fixups as done by pci_do_fixups().
>>
>> Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
>> ---
>>
>> v2: instead of changing all quirks to return if it was successfully
>> applied, remove the flag infra and use a static local variable to mark
>> quirks already applied (suggested by Bjorn Helgaas).
>>
>>  arch/x86/kernel/early-quirks.c | 60 ++++++++++++++++++++--------------
>>  1 file changed, 36 insertions(+), 24 deletions(-)
>>
>> diff --git a/arch/x86/kernel/early-quirks.c b/arch/x86/kernel/early-quirks.c
>> index 391a4e2b8604..102ecd0a910e 100644
>> --- a/arch/x86/kernel/early-quirks.c
>> +++ b/arch/x86/kernel/early-quirks.c
>> @@ -57,12 +57,18 @@ static void __init fix_hypertransport_config(int num, int slot, int func)
>>  static void __init via_bugs(int  num, int slot, int func)
>>  {
>>  #ifdef CONFIG_GART_IOMMU
>> +	static bool quirk_applied __initdata;
>> +
>> +	if (quirk_applied)
>> +		return;
>
>IMO this probably is better than using QFLAG_APPLY_ONCE, etc.
>
>But this patch has the mechanical changes related to QFLAG_APPLY_ONCE,
>which I think are useful but not very interesting and not a fix for
>something that's broken, mixed together with the *important* change
>that actually fixes a problem on Alderlake.

In general I agree with the statement and also ask people to follow that
logic, but here I thought it was small enough to be considered as a
whole.  Here is what I could do to split it up further in a way that is
not going in a different direction:

1) add the static local only to intel graphics quirk  and remove the
flag from this item
2 and 3) add the static local to other functions and remove the flag
from those items
4) remove the flag from the table, the defines and its usage.
5) fix the coding style (to be clear, it's already wrong, not
something wrong introduced here... maybe could be squashed in (4)?)

thoughts?

>
>Those two things need to be separate patches.  A patch that fixes a
>problem should be as small as possible so it's easy to understand and
>backport.
>
>The subject line of this patch doesn't say anything at all about
>Alderlake.  Splitting into two patches will make the subject lines
>more useful.

Alderlake is the platform in which it was reproduced, but could very
well be any other platform with Intel integrated graphics + Intel
discrete graphics. So I described the concrete reproducer in the commit
message rather than giving the impression this would be the only case
the current logic is broken.

thanks,
Lucas De Marchi

>
>Bjorn

Re: [PATCH v2 2/2] x86/quirks: better wrap quirk conditions

[Lucas De Marchi]

On Thu, Jan 06, 2022 at 04:23:25PM -0600, Bjorn Helgaas wrote:
>On Wed, Jan 05, 2022 at 04:36:54PM -0800, Lucas De Marchi wrote:
>> Remove extra parenthesis and wrap lines so it's easier to read what are
>> the conditions being checked. The call to the hook also had an extra
>> indentation: remove here to conform to coding style.
>
>It's nice when your subject lines are consistent.  These look like:
>
>  x86/quirks: Fix logic to apply quirk once
>  x86/quirks: better wrap quirk conditions
>
>The second isn't capitalized like the first.  Obviously if you split

trying to maintain the entropy from

	git log --oneline --no-merges -- arch/x86/kernel/early-quirks.c

:). Jokes aside, yeah, my bad.

>the first patch, you'll have three subject lines, and one will mention
>Alderlake.

See my reply to the first patch - Alderlake is only the reproducer, but
it's broken in other platforms as well, as long as it's paired with an
Intel discrete gpu in the "right" pci slot.

It would be weird to send a patch "Fix xyz in Alderlake" and then
telling people with the same problem in Icelake that they are
missing that fix. So I went with the approach: 1) what is the generic
problem; 2) where it was initially reproduced.

thanks
Lucas De Marchi

>
>Bjorn

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

[Bjorn Helgaas]

On Thu, Jan 06, 2022 at 02:30:47PM -0800, Lucas De Marchi wrote:
> On Thu, Jan 06, 2022 at 03:58:50PM -0600, Bjorn Helgaas wrote:
> > On Wed, Jan 05, 2022 at 04:36:53PM -0800, Lucas De Marchi wrote:
> > > When using QFLAG_APPLY_ONCE we make sure the quirk is called only once.
> > > 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: the quirk was called, but may not have been applied due to the
> > > additional filter.
> > > 
> > > 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.
> > > 
> > > This fixes the stolen memory reservation for an Alderlake-P system with
> > > one additional Intel discrete GPU (DG2 in this case, but applies for
> > > any of them). In this system we have:
> > > 
> > > 	- 00:01.0 Bridge
> > > 	  `- 03:00.0 DG2
> > > 	- 00:02.0 Alderlake-P's integrated GPU
> > > 
> > > 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.
> > > 
> > > Since there are just a few quirks using the QFLAG_APPLY_ONCE logic and
> > > that is even the only flag, just use a static local variable in the
> > > quirk function itself. This allows to mark the quirk as applied only
> > > when it really is. As pointed out by Bjorn Helgaas, this is also more in
> > > line with the PCI fixups as done by pci_do_fixups().
> > > 
> > > Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
> > > ---
> > > 
> > > v2: instead of changing all quirks to return if it was successfully
> > > applied, remove the flag infra and use a static local variable to mark
> > > quirks already applied (suggested by Bjorn Helgaas).
> > > 
> > >  arch/x86/kernel/early-quirks.c | 60 ++++++++++++++++++++--------------
> > >  1 file changed, 36 insertions(+), 24 deletions(-)
> > > 
> > > diff --git a/arch/x86/kernel/early-quirks.c b/arch/x86/kernel/early-quirks.c
> > > index 391a4e2b8604..102ecd0a910e 100644
> > > --- a/arch/x86/kernel/early-quirks.c
> > > +++ b/arch/x86/kernel/early-quirks.c
> > > @@ -57,12 +57,18 @@ static void __init fix_hypertransport_config(int num, int slot, int func)
> > >  static void __init via_bugs(int  num, int slot, int func)
> > >  {
> > >  #ifdef CONFIG_GART_IOMMU
> > > +	static bool quirk_applied __initdata;
> > > +
> > > +	if (quirk_applied)
> > > +		return;
> > 
> > IMO this probably is better than using QFLAG_APPLY_ONCE, etc.
> > 
> > But this patch has the mechanical changes related to QFLAG_APPLY_ONCE,
> > which I think are useful but not very interesting and not a fix for
> > something that's broken, mixed together with the *important* change
> > that actually fixes a problem on Alderlake.
> 
> In general I agree with the statement and also ask people to follow that
> logic, but here I thought it was small enough to be considered as a
> whole.  Here is what I could do to split it up further in a way that is
> not going in a different direction:
> 
> 1) add the static local only to intel graphics quirk  and remove the
> flag from this item
> 2 and 3) add the static local to other functions and remove the flag
> from those items
> 4) remove the flag from the table, the defines and its usage.
> 5) fix the coding style (to be clear, it's already wrong, not
> something wrong introduced here... maybe could be squashed in (4)?)

I would make this two patches:

  1) Add static locals to all QFLAG_APPLY_ONCE quirks and remove
     QFLAG_APPLY_ONCE etc.  This patch is mechanical and shouldn't
     break or fix anything.

  2) Update the intel graphics quirk to change the way it sets the
     static local.  Make the subject be something about fixing the
     problem with Intel integrated + discrete graphics.