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@xxxxxxxxx>
> > > ---
> > >
> > > 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.
