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

[Mario Limonciello <mario.limonciello@amd.com>]

Based on the feedback (the robot was correct!) I'm going to drop this 
patch from the series and adjust patch 3/3 slightly.

On 8/31/26 17:06, sashiko-bot@kernel.org wrote:
> 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.
> 

The robot looks right here.

>> +	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.


The robot looks right here too.

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