[PATCH v3 1/3] x86/quirks: Replace QFLAG_APPLY_ONCE with static locals

[PATCH v3 1/3] x86/quirks: Replace QFLAG_APPLY_ONCE with static locals

[Lucas De Marchi]

The flags are only used to mark a quirk to be called once and nothing
else. Also, that logic may not be appropriate if the quirk wants to
do additional filtering and set quirk ass applied by itself.

So replace the uses of QFLAG_APPLY_ONCE with static local variables in
the few quirks that use this logic and remove all the flags logic.

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

v3: Keep in this patch only the mechanical change to move from
QFLAG_APPLY_ONCE to static locals. Differently than v2, we don't try to
set quirk_applied in nvidia_bugs() and via_bugs() only when the
global resource is set - this patch is a mechanical move only and
shouldn't change any behavior. For intel_graphics_quirks(), we will move
it to the right place in a subsequent patch.

 arch/x86/kernel/early-quirks.c | 55 +++++++++++++++++++++-------------
 1 file changed, 34 insertions(+), 21 deletions(-)

diff --git a/arch/x86/kernel/early-quirks.c b/arch/x86/kernel/early-quirks.c
index 391a4e2b8604..8b689c2b8cc7 100644
--- a/arch/x86/kernel/early-quirks.c
+++ b/arch/x86/kernel/early-quirks.c
@@ -57,6 +57,13 @@ 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;
+
+	quirk_applied = true;
+
 	if ((max_pfn > MAX_DMA32_PFN ||  force_iommu) &&
 	    !gart_iommu_aperture_allowed) {
 		printk(KERN_INFO
@@ -81,6 +88,13 @@ 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;
+
+	quirk_applied = true;
+
 	/*
 	 * Only applies to Nvidia root ports (bus 0) and not to
 	 * Nvidia graphics cards with PCI ports on secondary buses.
@@ -587,10 +601,16 @@ intel_graphics_stolen(int num, int slot, int func,
 
 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;
+
+	quirk_applied = true;
+
 	device = read_pci_config_16(num, slot, func, PCI_DEVICE_ID);
 
 	for (i = 0; i < ARRAY_SIZE(intel_early_ids); i++) {
@@ -673,37 +693,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 +729,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 +772,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,
-- 
2.34.1

[PATCH v3 2/3] x86/quirks: Improve line wrap on 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 8b689c2b8cc7..df34963e23bf 100644
--- a/arch/x86/kernel/early-quirks.c
+++ b/arch/x86/kernel/early-quirks.c
@@ -767,14 +767,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

[PATCH v3 3/3] x86/quirks: Fix stolen detection with integrated + discrete GPU

[Lucas De Marchi]

early_pci_scan_bus() does a depth-first traversal, possibly calling
the quirk functions for each device based on vendor, device and class
from early_qrk table. intel_graphics_quirks() however uses PCI_ANY_ID
and does additional filtering in the quirk.

If there is an Intel integrated + discrete GPU the quirk may be called
first for the discrete GPU based on the PCI topology. Then we will fail
to reserve the system stolen memory for the integrated GPU, because we
will already have marked the quirk as "applied".

This was reproduced in a setup with Alderlake-P (integrated) + DG2
(discrete), with the following PCI topology:

	- 00:01.0 Bridge
	  `- 03:00.0 DG2
	- 00:02.0 Integrated GPU

Move the setting of quirk_applied in intel_graphics_quirks() so it's
mark as applied only when we find the integrated GPU based on the
intel_early_ids table.

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

v3: now that we do the refactor before the fix, we can do a single line
change to fix intel_graphics_quirks(). Also, we don't change
intel_graphics_stolen() anymore as we did in v2: we don't have to check
other devices anymore if there was a previous match causing
intel_graphics_stolen() to be called (there can only be one integrated
GPU reserving the stolen memory).

 arch/x86/kernel/early-quirks.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kernel/early-quirks.c b/arch/x86/kernel/early-quirks.c
index df34963e23bf..932f9087c324 100644
--- a/arch/x86/kernel/early-quirks.c
+++ b/arch/x86/kernel/early-quirks.c
@@ -609,8 +609,6 @@ static void __init intel_graphics_quirks(int num, int slot, int func)
 	if (quirk_applied)
 		return;
 
-	quirk_applied = true;
-
 	device = read_pci_config_16(num, slot, func, PCI_DEVICE_ID);
 
 	for (i = 0; i < ARRAY_SIZE(intel_early_ids); i++) {
@@ -623,6 +621,8 @@ static void __init intel_graphics_quirks(int num, int slot, int func)
 
 		intel_graphics_stolen(num, slot, func, early_ops);
 
+		quirk_applied = true;
+
 		return;
 	}
 }
-- 
2.34.1

Re: [PATCH v3 1/3] x86/quirks: Replace QFLAG_APPLY_ONCE with static locals

[Bjorn Helgaas]

On Fri, Jan 07, 2022 at 01:05:14PM -0800, Lucas De Marchi wrote:
> The flags are only used to mark a quirk to be called once and nothing
> else. Also, that logic may not be appropriate if the quirk wants to
> do additional filtering and set quirk ass applied by itself.

s/ass applied/as applied/

> So replace the uses of QFLAG_APPLY_ONCE with static local variables in
> the few quirks that use this logic and remove all the flags logic.
> 
> Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>

Reviewed-by: Bjorn Helgaas <bhelgaas@google.com>

> ---
> 
> v3: Keep in this patch only the mechanical change to move from
> QFLAG_APPLY_ONCE to static locals. Differently than v2, we don't try to
> set quirk_applied in nvidia_bugs() and via_bugs() only when the
> global resource is set - this patch is a mechanical move only and
> shouldn't change any behavior. For intel_graphics_quirks(), we will move
> it to the right place in a subsequent patch.
> 
>  arch/x86/kernel/early-quirks.c | 55 +++++++++++++++++++++-------------
>  1 file changed, 34 insertions(+), 21 deletions(-)
> 
> diff --git a/arch/x86/kernel/early-quirks.c b/arch/x86/kernel/early-quirks.c
> index 391a4e2b8604..8b689c2b8cc7 100644
> --- a/arch/x86/kernel/early-quirks.c
> +++ b/arch/x86/kernel/early-quirks.c
> @@ -57,6 +57,13 @@ 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;
> +
> +	quirk_applied = true;
> +
>  	if ((max_pfn > MAX_DMA32_PFN ||  force_iommu) &&
>  	    !gart_iommu_aperture_allowed) {
>  		printk(KERN_INFO
> @@ -81,6 +88,13 @@ 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;
> +
> +	quirk_applied = true;
> +
>  	/*
>  	 * Only applies to Nvidia root ports (bus 0) and not to
>  	 * Nvidia graphics cards with PCI ports on secondary buses.
> @@ -587,10 +601,16 @@ intel_graphics_stolen(int num, int slot, int func,
>  
>  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;
> +
> +	quirk_applied = true;
> +
>  	device = read_pci_config_16(num, slot, func, PCI_DEVICE_ID);
>  
>  	for (i = 0; i < ARRAY_SIZE(intel_early_ids); i++) {
> @@ -673,37 +693,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 +729,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 +772,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,
> -- 
> 2.34.1
> 

Re: [PATCH v3 3/3] x86/quirks: Fix stolen detection with integrated + discrete GPU

[Bjorn Helgaas]

On Fri, Jan 07, 2022 at 01:05:16PM -0800, Lucas De Marchi wrote:
> early_pci_scan_bus() does a depth-first traversal, possibly calling
> the quirk functions for each device based on vendor, device and class
> from early_qrk table. intel_graphics_quirks() however uses PCI_ANY_ID
> and does additional filtering in the quirk.
> 
> If there is an Intel integrated + discrete GPU the quirk may be called
> first for the discrete GPU based on the PCI topology. Then we will fail
> to reserve the system stolen memory for the integrated GPU, because we
> will already have marked the quirk as "applied".
> 
> This was reproduced in a setup with Alderlake-P (integrated) + DG2
> (discrete), with the following PCI topology:
> 
> 	- 00:01.0 Bridge
> 	  `- 03:00.0 DG2
> 	- 00:02.0 Integrated GPU
> 
> Move the setting of quirk_applied in intel_graphics_quirks() so it's
> mark as applied only when we find the integrated GPU based on the
> intel_early_ids table.
> 
> Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>

I don't know the details of stolen memory, but the implementation of
this quirk looks good to me.  Very nice that it's now very clear
exactly what the change is.

> ---
> 
> v3: now that we do the refactor before the fix, we can do a single line
> change to fix intel_graphics_quirks(). Also, we don't change
> intel_graphics_stolen() anymore as we did in v2: we don't have to check
> other devices anymore if there was a previous match causing
> intel_graphics_stolen() to be called (there can only be one integrated
> GPU reserving the stolen memory).
> 
>  arch/x86/kernel/early-quirks.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/x86/kernel/early-quirks.c b/arch/x86/kernel/early-quirks.c
> index df34963e23bf..932f9087c324 100644
> --- a/arch/x86/kernel/early-quirks.c
> +++ b/arch/x86/kernel/early-quirks.c
> @@ -609,8 +609,6 @@ static void __init intel_graphics_quirks(int num, int slot, int func)
>  	if (quirk_applied)
>  		return;
>  
> -	quirk_applied = true;
> -
>  	device = read_pci_config_16(num, slot, func, PCI_DEVICE_ID);
>  
>  	for (i = 0; i < ARRAY_SIZE(intel_early_ids); i++) {
> @@ -623,6 +621,8 @@ static void __init intel_graphics_quirks(int num, int slot, int func)
>  
>  		intel_graphics_stolen(num, slot, func, early_ops);
>  
> +		quirk_applied = true;
> +
>  		return;
>  	}
>  }
> -- 
> 2.34.1
> 

Re: [Intel-gfx] [PATCH v3 3/3] x86/quirks: Fix stolen detection with integrated + discrete GPU

[Rodrigo Vivi]

On Fri, Jan 07, 2022 at 08:57:32PM -0600, Bjorn Helgaas wrote:
> On Fri, Jan 07, 2022 at 01:05:16PM -0800, Lucas De Marchi wrote:
> > early_pci_scan_bus() does a depth-first traversal, possibly calling
> > the quirk functions for each device based on vendor, device and class
> > from early_qrk table. intel_graphics_quirks() however uses PCI_ANY_ID
> > and does additional filtering in the quirk.
> > 
> > If there is an Intel integrated + discrete GPU the quirk may be called
> > first for the discrete GPU based on the PCI topology. Then we will fail
> > to reserve the system stolen memory for the integrated GPU, because we
> > will already have marked the quirk as "applied".
> > 
> > This was reproduced in a setup with Alderlake-P (integrated) + DG2
> > (discrete), with the following PCI topology:
> > 
> > 	- 00:01.0 Bridge
> > 	  `- 03:00.0 DG2
> > 	- 00:02.0 Integrated GPU
> > 
> > Move the setting of quirk_applied in intel_graphics_quirks() so it's
> > mark as applied only when we find the integrated GPU based on the
> > intel_early_ids table.
> > 
> > Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
> 
> I don't know the details of stolen memory, but the implementation of
> this quirk looks good to me.  Very nice that it's now very clear
> exactly what the change is.


Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>

Bjorn, ack to merge through drm-intel?


> 
> > ---
> > 
> > v3: now that we do the refactor before the fix, we can do a single line
> > change to fix intel_graphics_quirks(). Also, we don't change
> > intel_graphics_stolen() anymore as we did in v2: we don't have to check
> > other devices anymore if there was a previous match causing
> > intel_graphics_stolen() to be called (there can only be one integrated
> > GPU reserving the stolen memory).
> > 
> >  arch/x86/kernel/early-quirks.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/arch/x86/kernel/early-quirks.c b/arch/x86/kernel/early-quirks.c
> > index df34963e23bf..932f9087c324 100644
> > --- a/arch/x86/kernel/early-quirks.c
> > +++ b/arch/x86/kernel/early-quirks.c
> > @@ -609,8 +609,6 @@ static void __init intel_graphics_quirks(int num, int slot, int func)
> >  	if (quirk_applied)
> >  		return;
> >  
> > -	quirk_applied = true;
> > -
> >  	device = read_pci_config_16(num, slot, func, PCI_DEVICE_ID);
> >  
> >  	for (i = 0; i < ARRAY_SIZE(intel_early_ids); i++) {
> > @@ -623,6 +621,8 @@ static void __init intel_graphics_quirks(int num, int slot, int func)
> >  
> >  		intel_graphics_stolen(num, slot, func, early_ops);
> >  
> > +		quirk_applied = true;
> > +
> >  		return;
> >  	}
> >  }
> > -- 
> > 2.34.1
> > 

Re: [Intel-gfx] [PATCH v3 2/3] x86/quirks: Improve line wrap on quirk conditions

[Rodrigo Vivi]

On Fri, Jan 07, 2022 at 01:05:15PM -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.
> 
> Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>

Reviewed-by: Rodrigo Vivi <rodrigo.vivi@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 8b689c2b8cc7..df34963e23bf 100644
> --- a/arch/x86/kernel/early-quirks.c
> +++ b/arch/x86/kernel/early-quirks.c
> @@ -767,14 +767,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: [Intel-gfx] [PATCH v3 3/3] x86/quirks: Fix stolen detection with integrated + discrete GPU

[Bjorn Helgaas]

On Mon, Jan 10, 2022 at 12:11:36PM -0500, Rodrigo Vivi wrote:
> On Fri, Jan 07, 2022 at 08:57:32PM -0600, Bjorn Helgaas wrote:
> > On Fri, Jan 07, 2022 at 01:05:16PM -0800, Lucas De Marchi wrote:
> > > early_pci_scan_bus() does a depth-first traversal, possibly calling
> > > the quirk functions for each device based on vendor, device and class
> > > from early_qrk table. intel_graphics_quirks() however uses PCI_ANY_ID
> > > and does additional filtering in the quirk.
> > > 
> > > If there is an Intel integrated + discrete GPU the quirk may be called
> > > first for the discrete GPU based on the PCI topology. Then we will fail
> > > to reserve the system stolen memory for the integrated GPU, because we
> > > will already have marked the quirk as "applied".
> > > 
> > > This was reproduced in a setup with Alderlake-P (integrated) + DG2
> > > (discrete), with the following PCI topology:
> > > 
> > > 	- 00:01.0 Bridge
> > > 	  `- 03:00.0 DG2
> > > 	- 00:02.0 Integrated GPU
> > > 
> > > Move the setting of quirk_applied in intel_graphics_quirks() so it's
> > > mark as applied only when we find the integrated GPU based on the
> > > intel_early_ids table.
> > > 
> > > Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
> > 
> > I don't know the details of stolen memory, but the implementation of
> > this quirk looks good to me.  Very nice that it's now very clear
> > exactly what the change is.
> 
> 
> Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> 
> Bjorn, ack to merge through drm-intel?

This is a bit of a shared area between the x86 folks and me, but
certainly no objection from me.

Acked-by: Bjorn Helgaas <bhelgaas@google.com>

> > > ---
> > > 
> > > v3: now that we do the refactor before the fix, we can do a single line
> > > change to fix intel_graphics_quirks(). Also, we don't change
> > > intel_graphics_stolen() anymore as we did in v2: we don't have to check
> > > other devices anymore if there was a previous match causing
> > > intel_graphics_stolen() to be called (there can only be one integrated
> > > GPU reserving the stolen memory).
> > > 
> > >  arch/x86/kernel/early-quirks.c | 4 ++--
> > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/arch/x86/kernel/early-quirks.c b/arch/x86/kernel/early-quirks.c
> > > index df34963e23bf..932f9087c324 100644
> > > --- a/arch/x86/kernel/early-quirks.c
> > > +++ b/arch/x86/kernel/early-quirks.c
> > > @@ -609,8 +609,6 @@ static void __init intel_graphics_quirks(int num, int slot, int func)
> > >  	if (quirk_applied)
> > >  		return;
> > >  
> > > -	quirk_applied = true;
> > > -
> > >  	device = read_pci_config_16(num, slot, func, PCI_DEVICE_ID);
> > >  
> > >  	for (i = 0; i < ARRAY_SIZE(intel_early_ids); i++) {
> > > @@ -623,6 +621,8 @@ static void __init intel_graphics_quirks(int num, int slot, int func)
> > >  
> > >  		intel_graphics_stolen(num, slot, func, early_ops);
> > >  
> > > +		quirk_applied = true;
> > > +
> > >  		return;
> > >  	}
> > >  }
> > > -- 
> > > 2.34.1
> > > 

Re: [Intel-gfx] [PATCH v3 3/3] x86/quirks: Fix stolen detection with integrated + discrete GPU

[Rodrigo Vivi]

On Mon, Jan 10, 2022 at 11:32:11AM -0600, Bjorn Helgaas wrote:
> On Mon, Jan 10, 2022 at 12:11:36PM -0500, Rodrigo Vivi wrote:
> > On Fri, Jan 07, 2022 at 08:57:32PM -0600, Bjorn Helgaas wrote:
> > > On Fri, Jan 07, 2022 at 01:05:16PM -0800, Lucas De Marchi wrote:
> > > > early_pci_scan_bus() does a depth-first traversal, possibly calling
> > > > the quirk functions for each device based on vendor, device and class
> > > > from early_qrk table. intel_graphics_quirks() however uses PCI_ANY_ID
> > > > and does additional filtering in the quirk.
> > > > 
> > > > If there is an Intel integrated + discrete GPU the quirk may be called
> > > > first for the discrete GPU based on the PCI topology. Then we will fail
> > > > to reserve the system stolen memory for the integrated GPU, because we
> > > > will already have marked the quirk as "applied".
> > > > 
> > > > This was reproduced in a setup with Alderlake-P (integrated) + DG2
> > > > (discrete), with the following PCI topology:
> > > > 
> > > > 	- 00:01.0 Bridge
> > > > 	  `- 03:00.0 DG2
> > > > 	- 00:02.0 Integrated GPU
> > > > 
> > > > Move the setting of quirk_applied in intel_graphics_quirks() so it's
> > > > mark as applied only when we find the integrated GPU based on the
> > > > intel_early_ids table.
> > > > 
> > > > Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
> > > 
> > > I don't know the details of stolen memory, but the implementation of
> > > this quirk looks good to me.  Very nice that it's now very clear
> > > exactly what the change is.
> > 
> > 
> > Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > 
> > Bjorn, ack to merge through drm-intel?
> 
> This is a bit of a shared area between the x86 folks and me, but
> certainly no objection from me.

Lucas brought a good point about patch 1 touching more stuff than i915 one,
so to minimize conflicts maybe the x86 tree would be better...
also works for us if you prefer.

> 
> Acked-by: Bjorn Helgaas <bhelgaas@google.com>
> 
> > > > ---
> > > > 
> > > > v3: now that we do the refactor before the fix, we can do a single line
> > > > change to fix intel_graphics_quirks(). Also, we don't change
> > > > intel_graphics_stolen() anymore as we did in v2: we don't have to check
> > > > other devices anymore if there was a previous match causing
> > > > intel_graphics_stolen() to be called (there can only be one integrated
> > > > GPU reserving the stolen memory).
> > > > 
> > > >  arch/x86/kernel/early-quirks.c | 4 ++--
> > > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > > > 
> > > > diff --git a/arch/x86/kernel/early-quirks.c b/arch/x86/kernel/early-quirks.c
> > > > index df34963e23bf..932f9087c324 100644
> > > > --- a/arch/x86/kernel/early-quirks.c
> > > > +++ b/arch/x86/kernel/early-quirks.c
> > > > @@ -609,8 +609,6 @@ static void __init intel_graphics_quirks(int num, int slot, int func)
> > > >  	if (quirk_applied)
> > > >  		return;
> > > >  
> > > > -	quirk_applied = true;
> > > > -
> > > >  	device = read_pci_config_16(num, slot, func, PCI_DEVICE_ID);
> > > >  
> > > >  	for (i = 0; i < ARRAY_SIZE(intel_early_ids); i++) {
> > > > @@ -623,6 +621,8 @@ static void __init intel_graphics_quirks(int num, int slot, int func)
> > > >  
> > > >  		intel_graphics_stolen(num, slot, func, early_ops);
> > > >  
> > > > +		quirk_applied = true;
> > > > +
> > > >  		return;
> > > >  	}
> > > >  }
> > > > -- 
> > > > 2.34.1
> > > > 

[PATCH v4] x86/quirks: Replace QFLAG_APPLY_ONCE with static locals

[Lucas De Marchi]

The flags are only used to mark a quirk to be called once and nothing
else. Also, that logic may not be appropriate if the quirk wants to
do additional filtering and set quirk as applied by itself.

So replace the uses of QFLAG_APPLY_ONCE with static local variables in
the few quirks that use this logic and remove all the flags logic.

Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
Reviewed-by: Bjorn Helgaas <bhelgaas@google.com>
---

v4: Fix typo in commit message

 arch/x86/kernel/early-quirks.c | 55 +++++++++++++++++++++-------------
 1 file changed, 34 insertions(+), 21 deletions(-)

diff --git a/arch/x86/kernel/early-quirks.c b/arch/x86/kernel/early-quirks.c
index 1ca3a56fdc2d..bab2a255b701 100644
--- a/arch/x86/kernel/early-quirks.c
+++ b/arch/x86/kernel/early-quirks.c
@@ -57,6 +57,13 @@ 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;
+
+	quirk_applied = true;
+
 	if ((max_pfn > MAX_DMA32_PFN ||  force_iommu) &&
 	    !gart_iommu_aperture_allowed) {
 		printk(KERN_INFO
@@ -81,6 +88,13 @@ 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;
+
+	quirk_applied = true;
+
 	/*
 	 * Only applies to Nvidia root ports (bus 0) and not to
 	 * Nvidia graphics cards with PCI ports on secondary buses.
@@ -589,10 +603,16 @@ intel_graphics_stolen(int num, int slot, int func,
 
 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;
+
+	quirk_applied = true;
+
 	device = read_pci_config_16(num, slot, func, PCI_DEVICE_ID);
 
 	for (i = 0; i < ARRAY_SIZE(intel_early_ids); i++) {
@@ -675,37 +695,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.
@@ -715,9 +731,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},
 	{}
 };
 
@@ -758,12 +774,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,
-- 
2.34.1

Re: [Intel-gfx] [PATCH v4] x86/quirks: Replace QFLAG_APPLY_ONCE with static locals

[Bjorn Helgaas]

On Wed, Jan 12, 2022 at 03:30:43PM -0800, Lucas De Marchi wrote:
> The flags are only used to mark a quirk to be called once and nothing
> else. Also, that logic may not be appropriate if the quirk wants to
> do additional filtering and set quirk as applied by itself.
> 
> So replace the uses of QFLAG_APPLY_ONCE with static local variables in
> the few quirks that use this logic and remove all the flags logic.
> 
> Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
> Reviewed-by: Bjorn Helgaas <bhelgaas@google.com>

Only occurred to me now, but another, less intrusive approach would be
to just remove QFLAG_APPLY_ONCE from intel_graphics_quirks() and do
its bookkeeping internally, e.g.,

diff --git a/arch/x86/kernel/early-quirks.c b/arch/x86/kernel/early-quirks.c
index 391a4e2b8604..7b655004e5fd 100644
--- a/arch/x86/kernel/early-quirks.c
+++ b/arch/x86/kernel/early-quirks.c
@@ -587,10 +587,14 @@ intel_graphics_stolen(int num, int slot, int func,
 
 static void __init intel_graphics_quirks(int num, int slot, int func)
 {
+	static bool stolen __initdata = false;
 	const struct intel_early_ops *early_ops;
 	u16 device;
 	int i;
 
+	if (stolen)
+		return;
+
 	device = read_pci_config_16(num, slot, func, PCI_DEVICE_ID);
 
 	for (i = 0; i < ARRAY_SIZE(intel_early_ids); i++) {
@@ -602,6 +606,7 @@ 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);
+		stolen = true;
 
 		return;
 	}
@@ -703,7 +708,7 @@ static struct chipset early_qrk[] __initdata = {
 	{ PCI_VENDOR_ID_INTEL, 0x3406, PCI_CLASS_BRIDGE_HOST,
 	  PCI_BASE_CLASS_BRIDGE, 0, intel_remapping_check },
 	{ PCI_VENDOR_ID_INTEL, PCI_ANY_ID, PCI_CLASS_DISPLAY_VGA, PCI_ANY_ID,
-	  QFLAG_APPLY_ONCE, intel_graphics_quirks },
+	  0, 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.

Re: [Intel-gfx] [PATCH v4] x86/quirks: Replace QFLAG_APPLY_ONCE with static locals

[Lucas De Marchi]

On Wed, Jan 12, 2022 at 06:08:05PM -0600, Bjorn Helgaas wrote:
>On Wed, Jan 12, 2022 at 03:30:43PM -0800, Lucas De Marchi wrote:
>> The flags are only used to mark a quirk to be called once and nothing
>> else. Also, that logic may not be appropriate if the quirk wants to
>> do additional filtering and set quirk as applied by itself.
>>
>> So replace the uses of QFLAG_APPLY_ONCE with static local variables in
>> the few quirks that use this logic and remove all the flags logic.
>>
>> Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
>> Reviewed-by: Bjorn Helgaas <bhelgaas@google.com>
>
>Only occurred to me now, but another, less intrusive approach would be
>to just remove QFLAG_APPLY_ONCE from intel_graphics_quirks() and do
>its bookkeeping internally, e.g.,

that is actually what I suggested after your comment in v2: this would
be the first patch with "minimal fix". But then to keep it consistent
with the other calls to follow up with additional patches on top
converting them as well.  Maybe what I wrote wasn't clear in the
direction? Copying it here:

	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)?)

Lucas De Marchi

>
>diff --git a/arch/x86/kernel/early-quirks.c b/arch/x86/kernel/early-quirks.c
>index 391a4e2b8604..7b655004e5fd 100644
>--- a/arch/x86/kernel/early-quirks.c
>+++ b/arch/x86/kernel/early-quirks.c
>@@ -587,10 +587,14 @@ intel_graphics_stolen(int num, int slot, int func,
>
> static void __init intel_graphics_quirks(int num, int slot, int func)
> {
>+	static bool stolen __initdata = false;
> 	const struct intel_early_ops *early_ops;
> 	u16 device;
> 	int i;
>
>+	if (stolen)
>+		return;
>+
> 	device = read_pci_config_16(num, slot, func, PCI_DEVICE_ID);
>
> 	for (i = 0; i < ARRAY_SIZE(intel_early_ids); i++) {
>@@ -602,6 +606,7 @@ 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);
>+		stolen = true;
>
> 		return;
> 	}
>@@ -703,7 +708,7 @@ static struct chipset early_qrk[] __initdata = {
> 	{ PCI_VENDOR_ID_INTEL, 0x3406, PCI_CLASS_BRIDGE_HOST,
> 	  PCI_BASE_CLASS_BRIDGE, 0, intel_remapping_check },
> 	{ PCI_VENDOR_ID_INTEL, PCI_ANY_ID, PCI_CLASS_DISPLAY_VGA, PCI_ANY_ID,
>-	  QFLAG_APPLY_ONCE, intel_graphics_quirks },
>+	  0, 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.

Re: [Intel-gfx] [PATCH v4] x86/quirks: Replace QFLAG_APPLY_ONCE with static locals

[Bjorn Helgaas]

On Wed, Jan 12, 2022 at 04:21:28PM -0800, Lucas De Marchi wrote:
> On Wed, Jan 12, 2022 at 06:08:05PM -0600, Bjorn Helgaas wrote:
> > On Wed, Jan 12, 2022 at 03:30:43PM -0800, Lucas De Marchi wrote:
> > > The flags are only used to mark a quirk to be called once and nothing
> > > else. Also, that logic may not be appropriate if the quirk wants to
> > > do additional filtering and set quirk as applied by itself.
> > > 
> > > So replace the uses of QFLAG_APPLY_ONCE with static local variables in
> > > the few quirks that use this logic and remove all the flags logic.
> > > 
> > > Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
> > > Reviewed-by: Bjorn Helgaas <bhelgaas@google.com>
> > 
> > Only occurred to me now, but another, less intrusive approach would be
> > to just remove QFLAG_APPLY_ONCE from intel_graphics_quirks() and do
> > its bookkeeping internally, e.g.,
> 
> that is actually what I suggested after your comment in v2: this would
> be the first patch with "minimal fix". But then to keep it consistent
> with the other calls to follow up with additional patches on top
> converting them as well.  Maybe what I wrote wasn't clear in the
> direction? Copying it here:
> 
> 	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)?)

Oh, sorry, I guess I just skimmed over that without really
comprehending it.

Although the patch below is basically just 1 from above and doesn't
require any changes to the other functions or the flags themselves
(2-4 above).

> > diff --git a/arch/x86/kernel/early-quirks.c b/arch/x86/kernel/early-quirks.c
> > index 391a4e2b8604..7b655004e5fd 100644
> > --- a/arch/x86/kernel/early-quirks.c
> > +++ b/arch/x86/kernel/early-quirks.c
> > @@ -587,10 +587,14 @@ intel_graphics_stolen(int num, int slot, int func,
> > 
> > static void __init intel_graphics_quirks(int num, int slot, int func)
> > {
> > +	static bool stolen __initdata = false;
> > 	const struct intel_early_ops *early_ops;
> > 	u16 device;
> > 	int i;
> > 
> > +	if (stolen)
> > +		return;
> > +
> > 	device = read_pci_config_16(num, slot, func, PCI_DEVICE_ID);
> > 
> > 	for (i = 0; i < ARRAY_SIZE(intel_early_ids); i++) {
> > @@ -602,6 +606,7 @@ 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);
> > +		stolen = true;
> > 
> > 		return;
> > 	}
> > @@ -703,7 +708,7 @@ static struct chipset early_qrk[] __initdata = {
> > 	{ PCI_VENDOR_ID_INTEL, 0x3406, PCI_CLASS_BRIDGE_HOST,
> > 	  PCI_BASE_CLASS_BRIDGE, 0, intel_remapping_check },
> > 	{ PCI_VENDOR_ID_INTEL, PCI_ANY_ID, PCI_CLASS_DISPLAY_VGA, PCI_ANY_ID,
> > -	  QFLAG_APPLY_ONCE, intel_graphics_quirks },
> > +	  0, 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.

Re: [Intel-gfx] [PATCH v4] x86/quirks: Replace QFLAG_APPLY_ONCE with static locals

[Lucas De Marchi]

On Wed, Jan 12, 2022 at 07:06:45PM -0600, Bjorn Helgaas wrote:
>On Wed, Jan 12, 2022 at 04:21:28PM -0800, Lucas De Marchi wrote:
>> On Wed, Jan 12, 2022 at 06:08:05PM -0600, Bjorn Helgaas wrote:
>> > On Wed, Jan 12, 2022 at 03:30:43PM -0800, Lucas De Marchi wrote:
>> > > The flags are only used to mark a quirk to be called once and nothing
>> > > else. Also, that logic may not be appropriate if the quirk wants to
>> > > do additional filtering and set quirk as applied by itself.
>> > >
>> > > So replace the uses of QFLAG_APPLY_ONCE with static local variables in
>> > > the few quirks that use this logic and remove all the flags logic.
>> > >
>> > > Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
>> > > Reviewed-by: Bjorn Helgaas <bhelgaas@google.com>
>> >
>> > Only occurred to me now, but another, less intrusive approach would be
>> > to just remove QFLAG_APPLY_ONCE from intel_graphics_quirks() and do
>> > its bookkeeping internally, e.g.,
>>
>> that is actually what I suggested after your comment in v2: this would
>> be the first patch with "minimal fix". But then to keep it consistent
>> with the other calls to follow up with additional patches on top
>> converting them as well.  Maybe what I wrote wasn't clear in the
>> direction? Copying it here:
>>
>> 	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)?)
>
>Oh, sorry, I guess I just skimmed over that without really
>comprehending it.
>
>Although the patch below is basically just 1 from above and doesn't
>require any changes to the other functions or the flags themselves
>(2-4 above).

Yes, but I would do the rest of the conversion anyway. It would be odd
to be inconsistent with just a few functions. So in the end I think we
would achieve the same goal.

I would really prefer this approach, having the bug fix first, if I was
concerned about having to backport this to linux-stable beyond 5.10.y
(we have a trivial conflict on 5.10).

However given this situation is new (Intel GPU + Intel Discrete GPU)
rare (it also needs a PCI topology in a certain way to reproduce it),
I'm not too concerned. Not even sure if it's worth submitting to
linux-stable.

I'll wait others to chime in on one way vs the other.

thanks
Lucas De Marchi

Re: [Intel-gfx] [PATCH v4] x86/quirks: Replace QFLAG_APPLY_ONCE with static locals

[Rodrigo Vivi]

On Wed, Jan 12, 2022 at 05:28:29PM -0800, Lucas De Marchi wrote:
> On Wed, Jan 12, 2022 at 07:06:45PM -0600, Bjorn Helgaas wrote:
> > On Wed, Jan 12, 2022 at 04:21:28PM -0800, Lucas De Marchi wrote:
> > > On Wed, Jan 12, 2022 at 06:08:05PM -0600, Bjorn Helgaas wrote:
> > > > On Wed, Jan 12, 2022 at 03:30:43PM -0800, Lucas De Marchi wrote:
> > > > > The flags are only used to mark a quirk to be called once and nothing
> > > > > else. Also, that logic may not be appropriate if the quirk wants to
> > > > > do additional filtering and set quirk as applied by itself.
> > > > >
> > > > > So replace the uses of QFLAG_APPLY_ONCE with static local variables in
> > > > > the few quirks that use this logic and remove all the flags logic.
> > > > >
> > > > > Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
> > > > > Reviewed-by: Bjorn Helgaas <bhelgaas@google.com>
> > > >
> > > > Only occurred to me now, but another, less intrusive approach would be
> > > > to just remove QFLAG_APPLY_ONCE from intel_graphics_quirks() and do
> > > > its bookkeeping internally, e.g.,
> > > 
> > > that is actually what I suggested after your comment in v2: this would
> > > be the first patch with "minimal fix". But then to keep it consistent
> > > with the other calls to follow up with additional patches on top
> > > converting them as well.  Maybe what I wrote wasn't clear in the
> > > direction? Copying it here:
> > > 
> > > 	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)?)
> > 
> > Oh, sorry, I guess I just skimmed over that without really
> > comprehending it.
> > 
> > Although the patch below is basically just 1 from above and doesn't
> > require any changes to the other functions or the flags themselves
> > (2-4 above).
> 
> Yes, but I would do the rest of the conversion anyway. It would be odd
> to be inconsistent with just a few functions. So in the end I think we
> would achieve the same goal.
> 
> I would really prefer this approach, having the bug fix first, if I was
> concerned about having to backport this to linux-stable beyond 5.10.y
> (we have a trivial conflict on 5.10).
> 
> However given this situation is new (Intel GPU + Intel Discrete GPU)
> rare (it also needs a PCI topology in a certain way to reproduce it),
> I'm not too concerned. Not even sure if it's worth submitting to
> linux-stable.

+1 on the minimal fix approach first and send that to stable 5.10+.
We will hit this case for sure.

also +1 on the discussed ideas as a follow up.

> 
> I'll wait others to chime in on one way vs the other.
> 
> thanks
> Lucas De Marchi