Re: [PATCH] PCI: Fix order of device disablement on reset

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] pci: bottom-up save and disable fails if parent bridge is in D3hot
- [Low] pci: stale documentation for save_and_disable_locked traversal
--

commit 0c082dfc1a7a6d8233dea3dd350ae1d9557bf0fb
Author: Lukas Wunner <lukas@wunner.de>
PCI: Fix order of device disablement on reset

This commit reverses the device disablement traversal order in
pci_bus_save_and_disable_locked() and pci_slot_save_and_disable_locked() to
recurse before disabling. This bottom-up approach is intended to avoid AER
splats caused by disabling access to the Switch Upstream Port before stopping
access to MMIO space on subordinate devices.

> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index b2879a6be5f80..ce9f74d659206 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -5508,9 +5508,9 @@ static void pci_bus_save_and_disable_locked(struct pci_bus *bus)
>  	struct pci_dev *dev;
>  
>  	list_for_each_entry(dev, &bus->devices, bus_list) {
> -		pci_dev_save_and_disable(dev);
>  		if (dev->subordinate)
>  			pci_bus_save_and_disable_locked(dev->subordinate);
> +		pci_dev_save_and_disable(dev);

[Severity: High]
Does this bottom-up reordering cause a regression if the parent bridge is in a
low-power state?

If the reset is triggered via the sysfs reset_subordinate attribute while the
parent bridge (e.g., Switch Upstream Port) is in a state like D3hot (via
runtime PM), the parent bridge is not woken up by the sysfs handler.

By calling pci_dev_save_and_disable() for the child first, the code will
attempt to access the child's Configuration Space before the parent bridge
is woken up:

pci_dev_save_and_disable() {
    ...
    pci_set_power_state(dev, PCI_D0);

    if (!pci_dev_config_accessible(dev, "save state"))
        return;

    pci_save_state(dev);
    ...
}

Since the parent bridge is still suspended, its secondary bus is inaccessible.
Could this cause the child's Configuration Space accesses to silently fail,
resulting in the child's saved state being corrupted?

If the state is corrupted, when pci_bus_restore_locked() later restores it,
will it permanently break the downstream devices until a cold reboot?

The original top-down traversal worked because it woke up the parent bridge
first, making the secondary bus accessible for the child's Configuration Space
accesses.

[Severity: Low]
Is the comment block above this function now inaccurate? It states:

/*
 * Save and disable devices from the top of the tree down while holding
 * the @dev mutex lock for the entire tree.
 */

Should this be updated to reflect the new bottom-up traversal?

>  	}
>  }
>  
> @@ -5543,9 +5543,9 @@ static void pci_slot_save_and_disable_locked(struct pci_slot *slot)
>  	list_for_each_entry(dev, &slot->bus->devices, bus_list) {
>  		if (!dev->slot || dev->slot != slot)
>  			continue;
> -		pci_dev_save_and_disable(dev);
>  		if (dev->subordinate)
>  			pci_bus_save_and_disable_locked(dev->subordinate);
> +		pci_dev_save_and_disable(dev);

[Severity: High]
Does the same problematic bottom-up reordering apply here for slot resets,
potentially corrupting the child's saved state if the parent bridge is
suspended?

[Severity: Low]
Does the documentation block for pci_slot_save_and_disable_locked() also need
to be updated to reflect the new bottom-up traversal?

>  	}
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/4c908eaab127d82ecfdc1e9948964b2e641d41d8.1789478646.git.lukas@wunner.de?part=1