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

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

[Lukas Wunner]

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

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

[sashiko-bot]

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

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

[Farhan Ali]

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

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

[Lukas Wunner]

On Tue, Sep 15, 2026 at 10:08:32AM -0700, Farhan Ali wrote:
> 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:
[...]
> 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?

We only enabled error reporting by default starting with v6.0 in 2022,
cf. commit f26e58bf6f54.

And the reset_subordinate sysfs attribute only exists since v6.13 in 2025,
cf. commit 2fa046449a82.

So it's relatively new functionality which apparently wasn't exercised
heavily so far.

It's remarkable though that universally enabling error reporting is now
alerting us to hidden bugs like this.  Commit f26e58bf6f54 cautioned
that the change is invasive but it's clearly useful.

Agreed on updating the comment, I'll have to respin the patch and
also address the sashiko findings regarding power management.

Thanks,

Lukas

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

[Keith Busch]

On Thu, Sep 17, 2026 at 02:38:42PM +0200, Lukas Wunner wrote:
> On Tue, Sep 15, 2026 at 10:08:32AM -0700, Farhan Ali wrote:
> > 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:
> [...]
> > 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?
> 
> We only enabled error reporting by default starting with v6.0 in 2022,
> cf. commit f26e58bf6f54.
> 
> And the reset_subordinate sysfs attribute only exists since v6.13 in 2025,
> cf. commit 2fa046449a82.
> 
> So it's relatively new functionality which apparently wasn't exercised
> heavily so far.

Yeah, we've used it to reset multi-function devices, but a deeper
topology like you described does look problematic here. Your proposal
makes the save state symmetrical with the restore side, so looks correct
to me.