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