Re: [PATCH v4 2/2] PCI: Put PCIe bridges with downstream devices into D3 at hibernate

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

On 9/16/26 00:23, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [High] pci: config space corruption on hibernation abort due to missing D3 guard
> --
> 
> commit 99f9dafa720163cc3dfac6ce20da883f888fb896
> Author: Mario Limonciello (AMD) <superm1@kernel.org>
> 
> PCI: Put PCIe bridges with downstream devices into D3 at hibernate
> 
> During S3 suspend, pci_pm_suspend_noirq() puts PCIe bridges with
> downstream devices into a low-power state. This commit aligns the
> hibernate poweroff_noirq flow to use pci_pm_suspend_noirq_common(),
> ensuring bridges with active children are also put into D3.
> 
>> diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c
>> --- a/drivers/pci/pci-driver.c
>> +++ b/drivers/pci/pci-driver.c
> [ ... ]
>> @@ -1280,8 +1283,9 @@ static int pci_pm_poweroff_noirq(struct device *dev)
>>   			return error;
>>   	}
>>   
>> -	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]
> Could this lead to configuration space corruption if a hibernation aborts?
> 
> If a driver's poweroff_noirq callback puts the device into a low-power
> state (like D3) but fails to save its state, calling
> pci_pm_suspend_noirq_common() here will unconditionally invoke
> pci_save_state().

That seems wrong.  It's gated by if (!pci_dev->state_saved) in the helper..

> 
> Because the device is already in D3, reading the configuration registers
> in pci_save_state() will populate saved_config_space with 0xFFFFFFFF
> garbage.

I think this is true; but only for D3cold, not D3hot.

I'm not sure how exactly this state could actually happen.

> 
> If the hibernation process subsequently aborts, the PM core executes
> pci_pm_restore_noirq(), which will blindly write this 0xFFFFFFFF garbage
> back to the device's configuration registers (BARs, command registers, etc).
> This will likely cause a system crash upon subsequent memory-mapped I/O
> access.
> 

I guess this is reasonable (pci_pm_default_resume_early() -> 
pci_restore_state()).

I suppose I didn't really consider a poweroff abort, which is what would 
be relevant in this case.

> Does pci_pm_poweroff_noirq() need the same explicit D3 safety guard that
> the S3 suspend_noirq path uses to prevent reading config space from a
> powered-down device?
> 

This does sound reasonable at least for the 3rd case.  I'm still not 
sold on the other two that Sashiko called out.