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)?) thoughts?
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.
Alderlake is the platform in which it was reproduced, but could very well be any other platform with Intel integrated graphics + Intel discrete graphics. So I described the concrete reproducer in the commit message rather than giving the impression this would be the only case the current logic is broken. thanks, Lucas De Marchi
Bjorn
