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

[Bjorn Helgaas <helgaas@kernel.org>]

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.