Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [Low] drm/amdgpu: unused variables in amdgpu_acpi_vfct_bios() break -Werror builds - [Low] drm/amdgpu: duplicated VFCT loop in amdgpu_acpi_vfct_bios() and dead code --
commit 50a61733895d9903e897b70fe18231ffdb69f9ae Author: Matthias Kesenheimer <[email protected]> drm/amdgpu: prefer exact bus match in VFCT VBIOS lookup This commit updates amdgpu_acpi_vfct_bios() to perform two passes over the ACPI VFCT table, preferring an exact PCI bus match in the first pass before falling back to device-identity matching in the second pass. > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_bios.c > b/drivers/gpu/drm/amd/amdgpu/amdgpu_bios.c > index f9315e0472fe9..0cee0872100a5 100644 > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_bios.c > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_bios.c > @@ -474,6 +474,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; [Severity: Low] Are fallback_vbios and fallback_vhdr left over from an earlier single-pass version of this patch? Neither pointer is referenced anywhere in amdgpu_acpi_vfct_bios(), which triggers -Wunused-variable warnings (and breaks builds with CONFIG_WERROR=y or CONFIG_DRM_AMDGPU_WERROR=y) when CONFIG_ACPI is enabled. > > if (!ACPI_SUCCESS(acpi_get_table("VFCT", 1, &hdr))) > return false; > @@ -486,6 +488,44 @@ static bool amdgpu_acpi_vfct_bios(struct amdgpu_device > *adev) > vfct = (UEFI_ACPI_VFCT *)hdr; > offset = vfct->VBIOSImageOffset; > > + /* First pass: look for exact bus match */ > + while (offset < tbl_size) { > + GOP_VBIOS_CONTENT *vbios = (GOP_VBIOS_CONTENT *)((char *)hdr + > offset); > + VFCT_IMAGE_HEADER *vhdr = &vbios->VbiosHeader; > + > + offset += sizeof(VFCT_IMAGE_HEADER); > + if (offset > tbl_size) { > + dev_info(adev->dev, "ACPI VFCT image header > truncated,skipping\n"); > + goto out; > + } > + > + offset += vhdr->ImageLength; > + if (offset > tbl_size) { > + dev_info(adev->dev, "ACPI VFCT image > truncated,skipping\n"); > + goto out; > + } > + > + if (vhdr->ImageLength && vhdr->PCIBus == > adev->pdev->bus->number && > + vhdr->VendorID == adev->pdev->vendor && > + vhdr->DeviceID == adev->pdev->device && > + vhdr->PCIDevice == PCI_SLOT(adev->pdev->devfn) && > + vhdr->PCIFunction == PCI_FUNC(adev->pdev->devfn)) { > + 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; > + } > + } [Severity: Low] This isn't a bug, but the first pass duplicates the entire VFCT table walk, truncation checks, and VBIOS loading logic of the second pass, while also open-coding the VendorID, DeviceID, PCIDevice, and PCIFunction checks from amdgpu_acpi_vfct_match(). Because any entry with a matching bus and device identity now jumps to out in the first pass, the exact bus check in amdgpu_acpi_vfct_match() also becomes unreachable dead code during the second pass: drivers/gpu/drm/amd/amdgpu/amdgpu_bios.c:amdgpu_acpi_vfct_match() { ... /* Exact bus number match - preferred */ if (vhdr->PCIBus == adev->pdev->bus->number) return 0; ... } Would it be simpler to record the first device-identity match in fallback_vbios and fallback_vhdr during a single pass over the table (and load it after the loop if no exact bus match was found), avoiding the duplicated table walk and keeping the matching logic in one place? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
