Re: [PATCH v5 2/2] PCI: Align hibernate poweroff flow with suspend flow for bridges

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] pci: missing secondary bus wait in restore_noirq on hibernation abort
--

commit 2ba69e2c07c28dfe82ab9450d201b37b5e9b14d7
Author: Mario Limonciello (AMD) <superm1@kernel.org>

    PCI: Align hibernate poweroff flow with suspend flow for bridges
    
    During S3 suspend, pci_pm_suspend_noirq() puts PCIe bridges with
    downstream devices into a low-power state (D3hot or D3cold) when the
    platform allows it.  The hibernate poweroff_noirq path never did this:
    it only called pci_prepare_to_sleep() for devices with no subordinate,
    so bridges with active children were left in D0.
    
    On many designs the platform firmware leaves bridges alone when the
    system enters S4. This prevents being able to meet various energy
    certification criteria for different parts of the world, particularly
    in designs with a dGPU.

> diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c
> index 8334214f8c1ed..8d293ca66a0d9 100644
> --- a/drivers/pci/pci-driver.c
> +++ b/drivers/pci/pci-driver.c

[ ... ]

> @@ -1272,16 +1275,26 @@ static int pci_pm_poweroff_noirq(struct device *dev)
>  	}
>  
>  	if (pm->poweroff_noirq) {
> +		pci_power_t prev = pci_dev->current_state;
>  		int error;
>  
>  		error = pm->poweroff_noirq(dev);
>  		suspend_report_result(dev, pm->poweroff_noirq, error);
>  		if (error)
>  			return error;
> +
> +		if (!pci_dev->state_saved && pci_dev->current_state != PCI_D0
> +		    && pci_dev->current_state != PCI_UNKNOWN) {
> +			pci_WARN_ONCE(pci_dev, pci_dev->current_state != prev,
> +				      "PCI PM: State of device not saved by %pS\n",
> +				      pm->poweroff_noirq);
> +			goto Fixup;
> +		}
>  	}
>  
> -	if (!pci_dev->state_saved && !pci_has_subordinate(pci_dev))
> -		pci_prepare_to_sleep(pci_dev);
> +	skip_bus_pm = pci_pm_suspend_noirq_common(pci_dev);

[Severity: High]
Since this change puts PCIe bridges into low-power states (D3hot/D3cold) just
like S3 suspend, does the recovery path for an aborted poweroff also need
updating?

If hibernation aborts after this point, the system recovers by calling
dpm_resume_end(PMSG_RESTORE), which invokes pci_pm_restore_noirq() for all
devices.

While pci_pm_resume_noirq() properly checks if a bridge was previously in
D3cold and calls pci_pm_bridge_power_up_actions(), pci_pm_restore_noirq() lacks
this check:

drivers/pci/pci-driver.c:pci_pm_restore_noirq() {
	...
	pci_pm_default_resume_early(pci_dev);
	pci_fixup_device(pci_fixup_resume_early, pci_dev);

	if (pci_has_legacy_pm_support(pci_dev))
		return 0;
	...
}

Without a secondary bus wait, downstream devices accessed during their
subsequent restore_noirq callbacks might hit an untrained link, potentially
resulting in Master Aborts and device state corruption.

> +	if (skip_bus_pm)
> +		goto Fixup;
>  
>  	/*
>  	 * The reason for doing this here is the same as for the analogous code

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260916205340.2466679-1-mario.limonciello@amd.com?part=2