diff mbox series

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

Message ID 20220112233043.1865454-1-lucas.demarchi@intel.com (mailing list archive)
State Superseded
Delegated to: Bjorn Helgaas
Headers show
Series [v4] x86/quirks: Replace QFLAG_APPLY_ONCE with static locals | expand

Commit Message

Lucas De Marchi Jan. 12, 2022, 11:30 p.m. UTC 
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(-)

Comments

Bjorn Helgaas Jan. 13, 2022, 12:08 a.m. UTC | #1 
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.
Lucas De Marchi Jan. 13, 2022, 12:21 a.m. UTC | #2 
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.
Bjorn Helgaas Jan. 13, 2022, 1:06 a.m. UTC | #3 
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.
Lucas De Marchi Jan. 13, 2022, 1:28 a.m. UTC | #4 
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
Rodrigo Vivi Jan. 13, 2022, 8:28 p.m. UTC | #5 
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
diff mbox series

Patch

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,