On Sun, Nov 1, 2026 at 7:11 AM Matthias Kesenheimer <[email protected]> wrote: > > When multiple cards share the same PCI device ID (e.g. RX 6600 and > RX 6600 XT both use 0x73ff), the VFCT table contains separate VBIOS > images for each card distinguished by their bus number. > > The current implementation iterates VFCT entries and returns on the > first device-identity match, which may not have an exact bus match. > This causes cards to load the wrong VBIOS image (e.g. RX 6600 loading > RX 6600 XT's VBIOS), resulting in incorrect clock speeds and > potential system hangs. > > Walk the VFCT table in a single pass, preferring an exact bus match. > amdgpu_acpi_vfct_match() returns 0 on an exact bus match, 1 on a > device-identity-only match, or -ENODEV on no match. The walk records > the matched entry - an exact bus match ends the walk immediately, > otherwise the first device-identity match is kept as a fallback. The > VBIOS is then loaded and validated once, after the walk, and the > bus-mismatch notice is emitted only when that fallback is actually > used. > > Changes since v2: > - Load and validate the VBIOS once, after the walk, instead of > duplicating the kmemdup()/check_atom_bios()/amdgpu_bios_release() > sequence between the exact-match and fallback branches. > - Only emit the bus-mismatch notice when the device-identity fallback > is actually used, avoiding a false notice on multi-GPU systems where > an exact bus match exists. > > Signed-off-by: Matthias Kesenheimer <[email protected]>
This should be fixed properly with this series: https://patchwork.freedesktop.org/series/176969/ Alex > --- > drivers/gpu/drm/amd/amdgpu/amdgpu_bios.c | 66 ++++++++++++++++-------- > 1 file changed, 44 insertions(+), 22 deletions(-) > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_bios.c > b/drivers/gpu/drm/amd/amdgpu/amdgpu_bios.c > index f9315e0472fe..d69f8df87249 100644 > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_bios.c > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_bios.c > @@ -441,7 +441,8 @@ static bool amdgpu_read_disabled_bios(struct > amdgpu_device *adev) > * the bus numbers disagree we accept the entry if the device identity > * matches. > * > - * Returns: 0 on match, -ENODEV on no match > + * Returns: 0 on exact bus match, 1 on device-identity match (bus > + * mismatch), -ENODEV on no match > */ > static int amdgpu_acpi_vfct_match(struct amdgpu_device *adev, > VFCT_IMAGE_HEADER *vhdr) > @@ -460,11 +461,7 @@ static int amdgpu_acpi_vfct_match(struct amdgpu_device > *adev, > return 0; > > /* Bus mismatch but device identity matches (PCI renumbering case) */ > - dev_notice(adev->dev, > - "VFCT bus number mismatch: table %u != runtime %u, > matching by device identity (vendor 0x%04x device 0x%04x)\n", > - vhdr->PCIBus, adev->pdev->bus->number, > - adev->pdev->vendor, adev->pdev->device); > - return 0; > + return 1; > } > > static bool amdgpu_acpi_vfct_bios(struct amdgpu_device *adev) > @@ -474,6 +471,7 @@ static bool amdgpu_acpi_vfct_bios(struct amdgpu_device > *adev) > UEFI_ACPI_VFCT *vfct; > unsigned int offset; > bool r = false; > + GOP_VBIOS_CONTENT *vbios = NULL; > > if (!ACPI_SUCCESS(acpi_get_table("VFCT", 1, &hdr))) > return false; > @@ -487,8 +485,9 @@ static bool amdgpu_acpi_vfct_bios(struct amdgpu_device > *adev) > offset = vfct->VBIOSImageOffset; > > while (offset < tbl_size) { > - GOP_VBIOS_CONTENT *vbios = (GOP_VBIOS_CONTENT *)((char *)hdr > + offset); > - VFCT_IMAGE_HEADER *vhdr = &vbios->VbiosHeader; > + GOP_VBIOS_CONTENT *entry = (GOP_VBIOS_CONTENT *)((char *)hdr > + offset); > + VFCT_IMAGE_HEADER *vhdr = &entry->VbiosHeader; > + int match; > > offset += sizeof(VFCT_IMAGE_HEADER); > if (offset > tbl_size) { > @@ -502,23 +501,46 @@ static bool amdgpu_acpi_vfct_bios(struct amdgpu_device > *adev) > goto out; > } > > - if (vhdr->ImageLength && > - !amdgpu_acpi_vfct_match(adev, vhdr)) { > - adev->bios = kmemdup(&vbios->VbiosContent, > - vhdr->ImageLength, > - GFP_KERNEL); > - > - if (!check_atom_bios(adev, vhdr->ImageLength)) { > - amdgpu_bios_release(adev); > - goto out; > - } > - adev->bios_size = vhdr->ImageLength; > - r = true; > - goto out; > + if (!vhdr->ImageLength) > + continue; > + > + match = amdgpu_acpi_vfct_match(adev, vhdr); > + if (match < 0) > + continue; > + > + if (match == 0) { > + /* Exact bus match wins - use it and stop scanning */ > + vbios = entry; > + break; > } > + > + /* match == 1: record the first device-identity match as a > fallback */ > + if (!vbios) > + vbios = entry; > } > > - dev_info(adev->dev, "ACPI VFCT table present but broken (too short > #2),skipping\n"); > + /* Load and validate the matched VBIOS once */ > + if (vbios) { > + VFCT_IMAGE_HEADER *vhdr = &vbios->VbiosHeader; > + > + if (vhdr->PCIBus != adev->pdev->bus->number) > + dev_notice(adev->dev, > + "VFCT bus number mismatch: table %u != > runtime %u, matching by device identity (vendor 0x%04x device 0x%04x)\n", > + vhdr->PCIBus, adev->pdev->bus->number, > + adev->pdev->vendor, adev->pdev->device); > + adev->bios = kmemdup(&vbios->VbiosContent, > + vhdr->ImageLength, > + GFP_KERNEL); > + > + if (!check_atom_bios(adev, vhdr->ImageLength)) { > + amdgpu_bios_release(adev); > + goto out; > + } > + adev->bios_size = vhdr->ImageLength; > + r = true; > + } else { > + dev_info(adev->dev, "ACPI VFCT table present but broken (too > short #2),skipping\n"); > + } > > out: > acpi_put_table(hdr); > -- > 2.56.0 > > > On 31. Oct 2026, at 21:51, [email protected] wrote: > > > > Thank you for your contribution! Sashiko AI review found 3 potential > > issue(s) to consider: > > - [Medium] drm/amdgpu: changelog and Signed-off-by placed below --- > > separator > > - [Low] drm/amdgpu: false bus-mismatch notice in amdgpu_acpi_vfct_match() > > - [Low] drm/amdgpu: duplicated VBIOS load logic in amdgpu_acpi_vfct_bios() > > -- > > > > commit 02e5f189d060ccb33fa88a19690736b51eba300f > > Author: Matthias Kesenheimer <[email protected]> > > > > drm/amdgpu: prefer exact bus match in VFCT VBIOS lookup > > > > This commit modifies amdgpu_acpi_vfct_bios() and amdgpu_acpi_vfct_match() > > to walk the ACPI VFCT table in a single pass, preferring an exact PCI bus > > number match and falling back to the first device-identity match only if no > > exact bus match is found. > > > >> drm/amdgpu: prefer exact bus match in VFCT VBIOS lookup > >> > >> Thanks for the review. Addressing both points: > >> - The unused fallback_vbios/fallback_vhdr are now actually used: the > >> first device-identity match is recorded during the pass and loaded only if > >> no exact bus match is found. > >> - Collapsed the two passes into a single table walk, keeping all > >> matching logic in amdgpu_acpi_vfct_match() (returns 0 exact-bus / 1 > >> identity-only / -ENODEV none). Matching behavior is unchanged from v1. > >> > >> The patch works on a live system. > > > > [Severity: Medium] > > It looks like the v2 review reply was placed above the --- separator, while > > the actual commit description and Signed-off-by tag were placed below it. > > When applied via git am, everything below --- is stripped, leaving the > > commit without its changelog explanation or Signed-off-by tag. Could the > > commit description and Signed-off-by be moved above the --- line, with the > > v2 changelog notes below it? > > > >> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_bios.c > >> b/drivers/gpu/drm/amd/amdgpu/amdgpu_bios.c > >> index f9315e0472fe9..ef9fd98eb9e1a 100644 > >> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_bios.c > >> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_bios.c > >> @@ -441,7 +441,8 @@ static bool amdgpu_read_disabled_bios(struct > >> amdgpu_device *adev) > >> * the bus numbers disagree we accept the entry if the device identity > >> * matches. > >> * > >> - * Returns: 0 on match, -ENODEV on no match > >> + * Returns: 0 on exact bus match, 1 on device-identity match (bus > >> + * mismatch), -ENODEV on no match > >> */ > >> static int amdgpu_acpi_vfct_match(struct amdgpu_device *adev, > >> VFCT_IMAGE_HEADER *vhdr) > >> @@ -464,7 +465,7 @@ static int amdgpu_acpi_vfct_match(struct amdgpu_device > >> *adev, > >> "VFCT bus number mismatch: table %u != runtime %u, matching by device > >> identity (vendor 0x%04x device 0x%04x)\n", > >> vhdr->PCIBus, adev->pdev->bus->number, > >> adev->pdev->vendor, adev->pdev->device); > >> - return 0; > >> + return 1; > >> } > > > > [Severity: Low] > > Will amdgpu_acpi_vfct_match() log a false bus-mismatch notice when probing > > the second GPU on a system with multiple identical PCI device IDs? > > > > When amdgpu_acpi_vfct_bios() walks the VFCT table for the second card, > > amdgpu_acpi_vfct_match() first encounters the entry for the first card on a > > different bus and immediately prints: > > > > dev_notice(adev->dev, > > "VFCT bus number mismatch: table %u != runtime %u, matching > > by device identity (vendor 0x%04x device 0x%04x)\n", > > ...); > > > > before returning 1, even though a subsequent VFCT entry matches the second > > card's bus number exactly and is used instead. > > > > Would it be better to move this dev_notice() out of amdgpu_acpi_vfct_match() > > and emit it in amdgpu_acpi_vfct_bios() only when the fallback entry is > > actually selected after the loop? > > > >> > >> static bool amdgpu_acpi_vfct_bios(struct amdgpu_device *adev) > >> @@ -474,6 +475,8 @@ static bool amdgpu_acpi_vfct_bios(struct amdgpu_device > >> *adev) > >> UEFI_ACPI_VFCT *vfct; > >> unsigned int offset; > >> bool r = false; > >> + GOP_VBIOS_CONTENT *fallback_vbios = NULL; > >> + VFCT_IMAGE_HEADER *fallback_vhdr = NULL; > >> > >> if (!ACPI_SUCCESS(acpi_get_table("VFCT", 1, &hdr))) > >> return false; > >> @@ -489,6 +492,7 @@ static bool amdgpu_acpi_vfct_bios(struct amdgpu_device > >> *adev) > >> while (offset < tbl_size) { > >> GOP_VBIOS_CONTENT *vbios = (GOP_VBIOS_CONTENT *)((char *)hdr + offset); > >> VFCT_IMAGE_HEADER *vhdr = &vbios->VbiosHeader; > >> + int match; > >> > >> offset += sizeof(VFCT_IMAGE_HEADER); > >> if (offset > tbl_size) { > >> @@ -502,8 +506,15 @@ static bool amdgpu_acpi_vfct_bios(struct > >> amdgpu_device *adev) > >> goto out; > >> } > >> > >> - if (vhdr->ImageLength && > >> - !amdgpu_acpi_vfct_match(adev, vhdr)) { > >> + if (!vhdr->ImageLength) > >> + continue; > >> + > >> + match = amdgpu_acpi_vfct_match(adev, vhdr); > >> + if (match < 0) > >> + continue; > >> + > >> + /* Exact bus match - use immediately */ > >> + if (match == 0) { > >> adev->bios = kmemdup(&vbios->VbiosContent, > >> vhdr->ImageLength, > >> GFP_KERNEL); > >> @@ -516,9 +527,29 @@ static bool amdgpu_acpi_vfct_bios(struct > >> amdgpu_device *adev) > >> r = true; > >> goto out; > >> } > >> + > >> + /* Device-identity match (bus mismatch) - record as fallback */ > >> + if (!fallback_vbios) { > >> + fallback_vbios = vbios; > >> + fallback_vhdr = vhdr; > >> + } > >> } > >> > >> - dev_info(adev->dev, "ACPI VFCT table present but broken (too short > >> #2),skipping\n"); > >> + /* No exact bus match - use the first device-identity match */ > >> + if (fallback_vbios) { > >> + adev->bios = kmemdup(&fallback_vbios->VbiosContent, > >> + fallback_vhdr->ImageLength, > >> + GFP_KERNEL); > >> + > >> + if (!check_atom_bios(adev, fallback_vhdr->ImageLength)) { > >> + amdgpu_bios_release(adev); > >> + goto out; > >> + } > >> + adev->bios_size = fallback_vhdr->ImageLength; > >> + r = true; > >> + } else { > >> + dev_info(adev->dev, "ACPI VFCT table present but broken (too short > >> #2),skipping\n"); > >> + } > > > > [Severity: Low] > > As reported by Sashiko on v1 [1], this isn't a bug, but the kmemdup(), > > check_atom_bios(), and amdgpu_bios_release() sequence in > > amdgpu_acpi_vfct_bios() is still duplicated between the exact-match branch > > inside the loop and the fallback_vbios branch after the loop. > > > > Since vhdr is &vbios->VbiosHeader, could the loop simply record the matched > > GOP_VBIOS_CONTENT pointer (breaking immediately when match == 0) and run the > > allocation and validation once after the loop? > > > > [1] https://lore.kernel.org/r/[email protected] > > > > -- > > Sashiko AI review · > > https://sashiko.dev/#/patchset/[email protected]?part=1 >
