Re: [PATCH v3] drm/amdgpu: prefer exact bus match in VFCT VBIOS lookup

From: Alex Deucher

Date: Mon Nov 02 2026 - 10:48:47 EST

On Sun, Nov 1, 2026 at 7:11 AM Matthias Kesenheimer
<m.kesenheimer@xxxxxxx> 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 <m.kesenheimer@xxxxxxx>

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, sashiko-bot@xxxxxxxxxx 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 <m.kesenheimer@xxxxxxx>
> >
> > 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/6C5E74D6-8F98-48BC-9173-7E85D243B3D3@xxxxxxx
> >
> > --
> > Sashiko AI review · https://sashiko.dev/#/patchset/2630FE08-2F46-4D29-AF3C-DD9EF2DE581E@xxxxxxx?part=1
>