[PATCH] PCI: Only restore state that was successfully saved around a reset

[PATCH] PCI: Only restore state that was successfully saved around a reset

[lirongqing]

From: Li RongQing <lirongqing@baidu.com>

pci_dev_save_and_disable() returns early when config space is inaccessible,
without saving anything, but pci_dev_restore() calls pci_restore_state()
unconditionally and pci_restore_state() does not look at state_saved. After
the reset makes config space accessible again, the all-zero or stale
saved_config_space[] array is written back to the device, clearing Command,
BARs and, for bridges, the bus number registers.

Clear state_saved when the save is skipped and only restore when a state
was saved.

Fixes: e18d1abc3bff ("PCI: Avoid saving config space state if inaccessible")
Signed-off-by: Li RongQing <lirongqing@baidu.com>
---
 drivers/pci/pci.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 67fe565..ddb86dd 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -5156,8 +5156,10 @@ static void pci_dev_save_and_disable(struct pci_dev *dev)
 	 */
 	pci_set_power_state(dev, PCI_D0);
 
-	if (!pci_dev_config_accessible(dev, "save state"))
+	if (!pci_dev_config_accessible(dev, "save state")) {
+		dev->state_saved = false;
 		return;
+	}
 
 	pci_save_state(dev);
 	/*
@@ -5175,7 +5177,8 @@ static void pci_dev_restore(struct pci_dev *dev)
 	const struct pci_error_handlers *err_handler =
 			dev->driver ? dev->driver->err_handler : NULL;
 
-	pci_restore_state(dev);
+	if (dev->state_saved)
+		pci_restore_state(dev);
 
 	/*
 	 * dev->driver->err_handler->reset_done() is protected against
-- 
2.9.4

[PATCH] PCI: Save the Root Port state before a platform-specific reset

[lirongqing]

From: Li RongQing <lirongqing@baidu.com>

pcibios_reset_secondary_bus() restores the Root Port after
host->reset_root_port() without ever saving it. pci_bus_reset() only locks
the devices below the bridge, so the bridge itself is not in the saved set
either. Save the Root Port before the platform reset.

Fixes: 3fc686d550f6 ("PCI/ERR: Add support for resetting the Root Ports in a platform-specific way")
Signed-off-by: Li RongQing <lirongqing@baidu.com>
---
 drivers/pci/pci.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index ddb86dd..eaccd89 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -4929,6 +4929,7 @@ void __weak pcibios_reset_secondary_bus(struct pci_dev *dev)
 	int ret;
 
 	if (pci_is_root_bus(dev->bus) && host->reset_root_port) {
+		pci_save_state(dev);
 		ret = host->reset_root_port(host, dev);
 		if (ret)
 			pci_err(dev, "Failed to reset Root Port: %d\n", ret);
-- 
2.9.4

Re: [PATCH] PCI: Only restore state that was successfully saved around a reset

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [High] PCI: pci_dev_restore() skips restoring preserved state after
  error reset
It also noticed 1 related pre-existing issue this patch does not
introduce:
- [High] PCI: capability buffer corruption on disconnect in
  pci_save_state() [1]
--

commit 101ae1e1e66a05607699c91780a5db7e8edbbdc0
Author: Li RongQing <lirongqing@baidu.com>

PCI: Only restore state that was successfully saved around a reset

This commit clears dev->state_saved when pci_dev_save_and_disable() skips
saving inaccessible config space and checks dev->state_saved before calling
pci_restore_state() in pci_dev_restore().

> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index d6590805faeb2..2327f47f3fda5 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -5309,8 +5309,10 @@ static void pci_dev_save_and_disable(struct pci_dev *dev)
>  	 */
>  	pci_set_power_state(dev, PCI_D0);
>  
> -	if (!pci_dev_config_accessible(dev, "save state"))
> +	if (!pci_dev_config_accessible(dev, "save state")) {
> +		dev->state_saved = false;
>  		return;
> +	}
>  
>  	pci_save_state(dev);

[Severity: High]
This problem wasn't introduced by this patch, but can pci_save_state()
corrupt the saved PCIe capability state [1] if the device becomes
inaccessible mid-save?

pci_save_state() sets dev->state_saved to true and calls
pci_save_pcie_state(), which reads capability registers into
save_state->cap.data without checking for errors:

drivers/pci/pci.c:pci_save_pcie_state() {
    ...
	cap = (u16 *)&save_state->cap.data[0];
	pcie_capability_read_word(dev, PCI_EXP_DEVCTL, &cap[i++]);
	pcie_capability_read_word(dev, PCI_EXP_LNKCTL, &cap[i++]);
	pcie_capability_read_word(dev, PCI_EXP_SLTCTL, &cap[i++]);
    ...
}

If the device disconnects during this window, these reads store ~0 (or 0 on
explicit read failure) into the persistent save buffer, and
pci_restore_state() later writes those corrupted values back to registers
such as PCI_EXP_DEVCTL.

[ ... ]

> @@ -5328,7 +5330,8 @@ static void pci_dev_restore(struct pci_dev *dev)
>  	const struct pci_error_handlers *err_handler =
>  			dev->driver ? dev->driver->err_handler : NULL;
>  
> -	pci_restore_state(dev);
> +	if (dev->state_saved)
> +		pci_restore_state(dev);

[Severity: High]
Does skipping pci_restore_state() here introduce a regression that leaves
the device unconfigured after a reset recovers it from an error?

Commit a2f1e22390ac ("PCI/ERR: Ensure error recoverability at all times")
saves the initial config space in pci_bus_add_device() and removed the
dev->state_saved check from pci_restore_state() so a valid baseline state
is always available even after a prior restore cleared dev->state_saved.

Commit e18d1abc3bff ("PCI: Avoid saving config space state if inaccessible")
then skipped pci_save_state() in pci_dev_save_and_disable() when config
space is inaccessible (such as during DPC) to avoid overwriting that
preserved state with ~0 before the reset.

Since the hardware reset clears the device's registers back to power-on
defaults, clearing dev->state_saved and skipping pci_restore_state() leaves
BARs, PCI_COMMAND, capabilities, and bridge bus numbers unprogrammed once
the reset makes config space accessible again.

[1] https://sashiko.dev/#/bug/linux-60bae679-5c85-4c49-8042-6817f9cb5c8f

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261110053348.2419-1-lirongqing@baidu.com?part=1

Re: [PATCH] PCI: Save the Root Port state before a platform-specific reset

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [High] PCI: corrupting Root Port saved state in
  pcibios_reset_secondary_bus()
--

commit c84a514dca7805a856128814f9bfb335473ce7f7
Author: Li RongQing <lirongqing@baidu.com>

PCI: Save the Root Port state before a platform-specific reset

This commit adds a pci_save_state() call in pcibios_reset_secondary_bus()
before invoking host->reset_root_port().

> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index d6590805faeb2..b4981e8060e28 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -5082,6 +5082,7 @@ void __weak pcibios_reset_secondary_bus(struct pci_dev *dev)
>  	int ret;
>  
>  	if (pci_is_root_bus(dev->bus) && host->reset_root_port) {
> +		pci_save_state(dev);
>  		ret = host->reset_root_port(host, dev);
>  		if (ret)
>  			pci_err(dev, "Failed to reset Root Port: %d\n", ret);

[Severity: High]
Does calling pci_save_state(dev) here overwrite the Root Port's working
saved state (saved during enumeration in pci_bus_add_device() and
pcie_portdrv_probe()) with post-error or post-link-down register values?

During link-down recovery via pci_host_handle_link_down() ->
pcie_do_recovery() -> pci_host_reset_root_port() -> pci_bus_error_reset(),
the link is already down before pcibios_reset_secondary_bus() runs.  Downstream
Ports clear PCI_EXP_DEVCTL2_LTR_EN on link down (and pci_bridge_reconfigure_ltr()
in pci_restore_pcie_state() is a no-op for Root Ports because
pci_upstream_bridge(dev) is NULL), while controllers such as pcie-dw-rockchip
and pcie-qcom have already reset their non-sticky Root Port registers on the
link-down event.  Saving state at this point captures those cleared registers
and restores them after host->reset_root_port().

Similarly, in the fatal AER recovery path, aer_root_reset() disables Root
Port AER interrupts before resetting the bus:

drivers/pci/pcie/aer.c:aer_root_reset() {
	if (host->native_aer && aer)
		aer_disable_irq(root);
	...
	} else {
		rc = pci_bus_error_reset(dev);
}

Calling pci_save_state(dev) inside pcibios_reset_secondary_bus() records the
IRQ-disabled PCI_ERR_ROOT_COMMAND value into dev->saved_cap_space, so any
subsequent pci_restore_state(dev) will leave Root Port AER interrupts
disabled.  And if host->reset_root_port() fails, dev->state_saved is left set
with the broken state, causing pci_pm_suspend_noirq_common() to skip saving
state and pci_prepare_to_sleep() on system suspend.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261110053348.2419-2-lirongqing@baidu.com?part=1

答复: [外部邮件] Re: [PATCH] PCI: Only restore state that was successfully saved around a reset

[Li,Rongqing]

> [Severity: High]
> Does skipping pci_restore_state() here introduce a regression that leaves the
> device unconfigured after a reset recovers it from an error?
> 
> Commit a2f1e22390ac ("PCI/ERR: Ensure error recoverability at all times") saves
> the initial config space in pci_bus_add_device() and removed the
> dev->state_saved check from pci_restore_state() so a valid baseline
> dev->state
> is always available even after a prior restore cleared dev->state_saved.
> 
> Commit e18d1abc3bff ("PCI: Avoid saving config space state if inaccessible")
> then skipped pci_save_state() in pci_dev_save_and_disable() when config space
> is inaccessible (such as during DPC) to avoid overwriting that preserved state
> with ~0 before the reset.
> 
> Since the hardware reset clears the device's registers back to power-on defaults,
> clearing dev->state_saved and skipping pci_restore_state() leaves BARs,
> PCI_COMMAND, capabilities, and bridge bus numbers unprogrammed once the
> reset makes config space accessible again.
> 
> [1] https://sashiko.dev/#/bug/linux-60bae679-5c85-4c49-8042-6817f9cb5c8f
> 

Sashiko is right — please drop this patch.

thanks

[Li,Rongqing] 

> --
> Sashiko AI
> review · https://sashiko.dev/#/patchset/20261110053348.2419-1-lirongqing
> @baidu.com?part=1

答复: [外部邮件] Re: [PATCH] PCI: Save the Root Port state before a platform-specific reset

[Li,Rongqing]

> 
> [Severity: High]
> Does calling pci_save_state(dev) here overwrite the Root Port's working saved
> state (saved during enumeration in pci_bus_add_device() and
> pcie_portdrv_probe()) with post-error or post-link-down register values?
> 
> During link-down recovery via pci_host_handle_link_down() ->
> pcie_do_recovery() -> pci_host_reset_root_port() -> pci_bus_error_reset(), the
> link is already down before pcibios_reset_secondary_bus() runs.  Downstream
> Ports clear PCI_EXP_DEVCTL2_LTR_EN on link down (and
> pci_bridge_reconfigure_ltr() in pci_restore_pcie_state() is a no-op for Root Ports
> because
> pci_upstream_bridge(dev) is NULL), while controllers such as pcie-dw-rockchip
> and pcie-qcom have already reset their non-sticky Root Port registers on the
> link-down event.  Saving state at this point captures those cleared registers and
> restores them after host->reset_root_port().
> 
> Similarly, in the fatal AER recovery path, aer_root_reset() disables Root Port AER
> interrupts before resetting the bus:
> 
> drivers/pci/pcie/aer.c:aer_root_reset() {
> 	if (host->native_aer && aer)
> 		aer_disable_irq(root);
> 	...
> 	} else {
> 		rc = pci_bus_error_reset(dev);
> }
> 
> Calling pci_save_state(dev) inside pcibios_reset_secondary_bus() records the
> IRQ-disabled PCI_ERR_ROOT_COMMAND value into dev->saved_cap_space, so
> any subsequent pci_restore_state(dev) will leave Root Port AER interrupts
> disabled.  And if host->reset_root_port() fails, dev->state_saved is left set with
> the broken state, causing pci_pm_suspend_noirq_common() to skip saving state
> and pci_prepare_to_sleep() on system suspend.
> 

I will send a v2 to fix the issue found by Sashiko

[Li,Rongqing] 


> --
> Sashiko AI
> review · https://sashiko.dev/#/patchset/20261110053348.2419-2-lirongqing
> @baidu.com?part=1

Re: [PATCH] PCI: Only restore state that was successfully saved around a reset

[Lukas Wunner]

On Tue, Nov 10, 2026 at 01:33:47PM +0800, lirongqing wrote:
> pci_dev_save_and_disable() returns early when config space is inaccessible,
> without saving anything, but pci_dev_restore() calls pci_restore_state()
> unconditionally and pci_restore_state() does not look at state_saved. After
> the reset makes config space accessible again, the all-zero or stale
> saved_config_space[] array is written back to the device, clearing Command,
> BARs and, for bridges, the bus number registers.
> 
> Clear state_saved when the save is skipped and only restore when a state
> was saved.

The state_saved flag is used internally by the PCI core for
suspend/resume.  If a driver calls pci_save_state() on suspend,
the flag is set and that tells the PCI core that the driver already
put the device into a lower power state, obviating the need for the
PCI core to do that.

Please do not use the flag for a completely different purpose.

Thanks,

Lukas

Re: [PATCH] PCI: Save the Root Port state before a platform-specific reset

[Lukas Wunner]

On Tue, Nov 10, 2026 at 01:33:48PM +0800, lirongqing wrote:
> pcibios_reset_secondary_bus() restores the Root Port after
> host->reset_root_port() without ever saving it. pci_bus_reset() only locks
> the devices below the bridge, so the bridge itself is not in the saved set
> either. Save the Root Port before the platform reset.

The state is saved on enumeration in pci_bus_add_device(),
see commit a2f1e22390ac.  Please explain why that is not
sufficient for you.

Thanks,

Lukas

Re: [PATCH] PCI: Only restore state that was successfully saved around a reset

[Farhan Ali]

Hi Li,

On 11/9/2026 9:33 PM, lirongqing wrote:
> From: Li RongQing <lirongqing@baidu.com>
>
> pci_dev_save_and_disable() returns early when config space is inaccessible,
> without saving anything, but pci_dev_restore() calls pci_restore_state()
> unconditionally and pci_restore_state() does not look at state_saved. After
> the reset makes config space accessible again, the all-zero or stale
> saved_config_space[] array is written back to the device, clearing Command,
> BARs and, for bridges, the bus number registers.
>
> Clear state_saved when the save is skipped and only restore when a state
> was saved.

We should always have a valid state of the device saved at enumeration 
due to commit a2f1e22390ac ("PCI/ERR: Ensure error recoverability at all 
times"). In pci_dev_save_disable(), if the device is inaccessible then 
we want to avoid storing a state that can corrupt the device on restore. 
Since we always have a valid state, we would restore to the last good state.

May I ask what device or driver do you hit this issue?

Thanks

Farhan


>
> Fixes: e18d1abc3bff ("PCI: Avoid saving config space state if inaccessible")
> Signed-off-by: Li RongQing <lirongqing@baidu.com>
> ---
>   drivers/pci/pci.c | 7 +++++--
>   1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index 67fe565..ddb86dd 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -5156,8 +5156,10 @@ static void pci_dev_save_and_disable(struct pci_dev *dev)
>   	 */
>   	pci_set_power_state(dev, PCI_D0);
>   
> -	if (!pci_dev_config_accessible(dev, "save state"))
> +	if (!pci_dev_config_accessible(dev, "save state")) {
> +		dev->state_saved = false;
>   		return;
> +	}
>   
>   	pci_save_state(dev);
>   	/*
> @@ -5175,7 +5177,8 @@ static void pci_dev_restore(struct pci_dev *dev)
>   	const struct pci_error_handlers *err_handler =
>   			dev->driver ? dev->driver->err_handler : NULL;
>   
> -	pci_restore_state(dev);
> +	if (dev->state_saved)
> +		pci_restore_state(dev);
>   
>   	/*
>   	 * dev->driver->err_handler->reset_done() is protected against

答复: [外部邮件] Re: [PATCH] PCI: Only restore state that was successfully saved around a reset

[Li,Rongqing]

> > pci_dev_save_and_disable() returns early when config space is
> > inaccessible, without saving anything, but pci_dev_restore() calls
> > pci_restore_state() unconditionally and pci_restore_state() does not
> > look at state_saved. After the reset makes config space accessible
> > again, the all-zero or stale saved_config_space[] array is written
> > back to the device, clearing Command, BARs and, for bridges, the bus number
> registers.
> >
> > Clear state_saved when the save is skipped and only restore when a
> > state was saved.
> 
> We should always have a valid state of the device saved at enumeration due to
> commit a2f1e22390ac ("PCI/ERR: Ensure error recoverability at all times"). In
> pci_dev_save_disable(), if the device is inaccessible then we want to avoid
> storing a state that can corrupt the device on restore.
> Since we always have a valid state, we would restore to the last good state.
> 
You're right, please disregard this patch.

I missed that a2f1e22390ac ("PCI/ERR: Ensure error recoverability at
all times") already saves a valid state at enumeration and deliberately
drops the state_saved check in pci_restore_state(). Skipping the save
when config space is inaccessible keeps that last good state, and the
unconditional restore writes it back after the reset. Gating
pci_dev_restore() on state_saved would instead leave the device at its
power-on defaults. This came from code inspection of the reset path, not 
from a failure on a specific device or driver.

Sorry for the noise.

[Li,Rongqing] 

答复: [????] Re: [PATCH] PCI: Save the Root Port state before a platform-specific reset

[Li,Rongqing]

> The state is saved on enumeration in pci_bus_add_device(), see commit
> a2f1e22390ac.  Please explain why that is not sufficient for you.
> 
It is sufficient. I had missed that a2f1e22390ac keeps the state saved
at enumeration reusable and that pci_restore_state() no longer depends
on a fresh save. Please disregard this patch. Sorry for the noise.
thanks

[Li,Rongqing]