n Thu, Jan 13, 2022 at 04:28:39PM -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".
Who is "we"?
Please use passive voice in your commit message: no "we" or "I", etc,
and describe your changes in imperative mood.
Bottom line is: personal pronouns are ambiguous in text, especially with
so many parties/companies/etc developing the kernel so let's avoid them
please.
> 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
>
> So, stop using the QFLAG_APPLY_ONCE flag, replacing it with a static
> local variable. We can set this variable in the right place, inside
> intel_graphics_quirks(), only when the quirk was actually applied, i.e.
> when we find the integrated GPU based on the intel_early_ids table.
>
> Cc: stable@xxxxxxxxxxxxxxx
> Signed-off-by: Lucas De Marchi <lucas.demarchi@xxxxxxxxx>
> ---
>
> v5: apply fix before the refactor
>
> arch/x86/kernel/early-quirks.c | 8 +++++++-
> 1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/arch/x86/kernel/early-quirks.c b/arch/x86/kernel/early-quirks.c
> index 1ca3a56fdc2d..de9a76eb544e 100644
> --- a/arch/x86/kernel/early-quirks.c
> +++ b/arch/x86/kernel/early-quirks.c
> @@ -589,10 +589,14 @@ 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;
> +
> device = read_pci_config_16(num, slot, func, PCI_DEVICE_ID);
>
> for (i = 0; i < ARRAY_SIZE(intel_early_ids); i++) {
> @@ -605,6 +609,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;
> }
So I wonder: why can't you simply pass in a static struct chipset *
pointer into the early_qrk[i].f function and in there you can set
QFLAG_APPLIED or so, so that you can mark that the quirk is applied by
using the nice, per-quirk flags someone has already added instead of
this ugly static variable?
Patch 3 especially makes me go, huh?
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
