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
