Re: [PATCH 2/2] drm/amdgpu: Match VFCT using initial PCI bus number

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] drm/amdgpu: amdgpu_acpi_vfct_match() uses post-renumbering initial_bus_number
--

commit 1c0de5f9175b6acb9431f78243e7c9f86e945b98
Author: Mario Limonciello <mario.limonciello@amd.com>

drm/amdgpu: Match VFCT using initial PCI bus number

This commit updates amdgpu_acpi_vfct_match() to match the VFCT header's
PCIBus field against adev->pdev->initial_bus_number instead of accepting
bus mismatches based on device identity alone.

> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_bios.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_bios.c
> index f9315e0472fe9..7b66dc2414250 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_bios.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_bios.c
> @@ -436,10 +436,8 @@ static bool amdgpu_read_disabled_bios(struct amdgpu_device *adev)
>   * VFCT entries contain the PCI bus number as recorded during BIOS POST.
>   * On systems where the kernel renumbers PCI buses (e.g. pci=realloc or
>   * resource conflicts), the runtime bus number may differ from the POST
> - * value.  Match by device identity (vendor + device + function) and use
> - * the bus number as a preference: exact bus match is preferred, but when
> - * the bus numbers disagree we accept the entry if the device identity
> - * matches.
> + * value.  Match using the bus number captured when the device was
> + * discovered, before any bus renumbering.
>   *
>   * Returns: 0 on match, -ENODEV on no match
>   */
> @@ -455,16 +453,7 @@ static int amdgpu_acpi_vfct_match(struct amdgpu_device *adev,
>  	    vhdr->PCIFunction != PCI_FUNC(adev->pdev->devfn))
>  		return -ENODEV;
>  
> -	/* Exact bus number match - preferred */
> -	if (vhdr->PCIBus == adev->pdev->bus->number)
> -		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 vhdr->PCIBus == adev->pdev->initial_bus_number ? 0 : -ENODEV;
>  }

[Severity: High]
Does adev->pdev->initial_bus_number actually hold the BIOS POST bus number
when the PCI core renumbers a bridge?

In drivers/pci/probe.c, pci_alloc_dev() initializes initial_bus_number when
the pci_dev structure is allocated during pci_scan_device():

drivers/pci/probe.c:pci_alloc_dev() {
    ...
    dev->bus = pci_bus_get(bus);
    dev->initial_bus_number = bus->number;
    ...
}

However, when pci_scan_bridge_extend() renumbers a bridge (for example, when
pcibios_assign_all_busses() is true or when the firmware bridge configuration
is broken/conflicting), it skips scanning behind the bridge during pass 0 and
clears the firmware bus numbers in the bridge's PCI_PRIMARY_BUS register:

drivers/pci/probe.c:pci_scan_bridge_extend() {
    ...
        if (!pass) {
            if (pcibios_assign_all_busses() || broken)
                pci_write_config_dword(dev, PCI_PRIMARY_BUS,
                                       buses & PCI_SEC_LATENCY_TIMER_MASK);
            goto out;
        }
    ...
        child = pci_find_bus(pci_domain_nr(bus), next_busnr);
        if (!child) {
            child = pci_add_new_bus(bus, dev, next_busnr);
            ...
        }
        ...
        max = pci_scan_child_bus_extend(child, available_buses);
}

The child pci_bus and its devices are only allocated and scanned in pass 1,
after child->number has already been set to the newly assigned next_busnr in
pci_add_new_bus() -> pci_alloc_child_bus(). Because bus->number is not
changed after child bus creation, pdev->initial_bus_number is always equal to
the post-renumbering runtime pdev->bus->number.

When the kernel renumbers the PCI bus hierarchy leading to the GPU during
amdgpu_device_init() -> amdgpu_get_bios() -> amdgpu_acpi_vfct_bios() ->
amdgpu_acpi_vfct_match(), won't vhdr->PCIBus == adev->pdev->initial_bus_number
evaluate to false and return -ENODEV, causing VFCT VBIOS lookup to fail just
as it did before commit 11c141672045f ("drm/amdgpu: Fix VFCT bus number
matching with soft filter")?

>  
>  static bool amdgpu_acpi_vfct_bios(struct amdgpu_device *adev)

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261019-vfct-regression-v1-0-5a63ddd378dd@amd.com?part=2