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

[Farhan Ali <alifm@linux.ibm.com>]

Hi Lukas,

On 9/15/2026 6:35 AM, Lukas Wunner wrote:
> Riana reports an AER splat when issuing a Secondary Bus Reset through the
> "reset_subordinate" sysfs attribute:
>
>    AER: Multiple Uncorrectable (Non-Fatal) error message received from 0000:01:00.0
>    PCIe Bus Error: severity=Uncorrectable (Non-Fatal), type=Transaction Layer, (Requester ID)
>      device [8086:e2ff] error status/mask=00100000/00400000
>      [20] UnsupReq               (First)
>    AER:   TLP Header: 0x40000001 0x0000000f 0x81190008 0x00000000
>
> The Secondary Bus Reset is issued at the Root Port.  Underneath is a
> Switch with two Endpoints.  Riana has identified an ordering issue in
> pci_bus_save_and_disable_locked() as root cause of the AER splat:
> The function calls pci_dev_save_and_disable() for the Switch Upstream Port
> first, then recurses down to its subordinate devices.
>
> pci_dev_save_and_disable() quiesces a device by clearing its Command
> register (in accordance with PCIe r7.0 sec 6.6.2, "Implementation Note:
> Avoiding data corruption from stale completions").
>
> On devices with a Type 1 Configuration Space Header (such as the Switch
> Upstream Port), clearing the Command register causes MMIO accesses to
> subordinate devices to be handled as Unsupported Requests, as seen in
> the AER splat above (PCIe r7.0 sec 7.5.1.1.3 bit 1).
>
> The drivers of subordinate devices are only informed afterwards of the
> impending reset via their ->reset_prepare() callback.  They get no chance
> to stop access to MMIO space before the Switch Upstream Port disables
> access.
>
> Reorder pci_bus_save_and_disable_locked() to recurse first and only then
> disable the Switch Upstream Port.  This mirrors the order in
> pci_slot_restore_locked() and avoids the AER splat.
>
> Do the same in pci_slot_save_and_disable_locked() for consistency.
>
> Fixes: 090a3c5322e9 ("PCI: Add pci_reset_slot() and pci_reset_bus()")
> Reported-by: Riana Tauro <riana.tauro@intel.com> # off-list
> Tested-by: Riana Tauro <riana.tauro@intel.com>
> Signed-off-by: Lukas Wunner <lukas@wunner.de>
> Cc: stable@vger.kernel.org # v3.12+
> ---
>   drivers/pci/pci.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index b2879a6..ce9f74d 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);
>   	}
>   }
>   

Since we are changing the order, maybe we should update comment for the 
functions to reflect that. The change does make sense to me, but I am 
curious why we didn't see this before?

Thanks

Farhan

> @@ -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);
>   	}
>   }
>