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

[sashiko-bot@kernel.org]

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