Re: [PATCH v3 2/3] PCI/PM: Run bridge power up actions as part of restore phase

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] The condition `prev_state == PCI_D3cold` in `pci_pm_restore_noirq()` will always evaluate to false for devices active before hibernation, causing bridge power-up actions to be incorrectly skipped.
- [High] `skip_bus_pm` evaluates to stale or uninitialized values during hibernation restore.
--

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

PCI/PM: Run bridge power up actions as part of restore phase

This commit adds bridge power up actions to the hibernate restore phase in
pci_pm_restore_noirq(), gated by the previous power state and whether bus PM
was skipped. This aims to ensure mandatory link training delays are observed
before downstream devices are accessed, mirroring the S3 suspend path.

> diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c
> index e3d352c7b31d4..d15b3eb55ec53 100644
> --- a/drivers/pci/pci-driver.c
> +++ b/drivers/pci/pci-driver.c
> @@ -1297,10 +1297,15 @@ static int pci_pm_restore_noirq(struct device *dev)
>  {
>  	struct pci_dev *pci_dev = to_pci_dev(dev);
>  	const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
> +	pci_power_t prev_state = pci_dev->current_state;

[Severity: High]
Will prev_state always evaluate to PCI_UNKNOWN instead of PCI_D3cold?

During hibernation, the memory snapshot is taken in the FREEZE phase.
pci_pm_freeze() resumes devices to PCI_D0, and pci_pm_freeze_noirq()
subsequently sets their current_state to PCI_UNKNOWN.

This PCI_UNKNOWN state is what gets saved in the memory snapshot and restored
here. Although devices are put in PCI_D3cold during the POWEROFF phase, this
occurs after the snapshot has already been captured and written to disk.

> +	bool skip_bus_pm = pci_dev->skip_bus_pm;

[Severity: High]
Could pci_dev->skip_bus_pm evaluate to a stale or uninitialized value here?

The hibernation snapshot is captured during the FREEZE phase, where neither
pci_pm_freeze() nor pci_pm_freeze_noirq() initialize or update skip_bus_pm.
The snapshot merely captures whatever stale value it had, such as from a prior
S3 suspend.

While skip_bus_pm is initialized in pci_pm_poweroff(), that function runs in
the POWEROFF phase after the hibernation snapshot has been completed, so the
initialized value is never saved to the image.

>  
>  	pci_pm_default_resume_early(pci_dev);
>  	pci_fixup_device(pci_fixup_resume_early, pci_dev);
>  
> +	if (!skip_bus_pm && prev_state == PCI_D3cold)
> +		pci_pm_bridge_power_up_actions(pci_dev);

[Severity: High]
Since prev_state is PCI_UNKNOWN and skip_bus_pm may be stale, is it possible
that this condition will always fail and cause the required bridge retraining
to be skipped?

This could lead to accessing downstream devices before the bridge link is
retrained, resulting in PCIe errors or system crashes on hibernation resume.

> +
>  	if (pci_has_legacy_pm_support(pci_dev))
>  		return 0;
>

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