Re: [PATCH v2] PCI/PM: Restore BARs after powering up during enumeration

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

On 10/12/26 06:34, Rafael J. Wysocki (Intel) wrote:
> On Mon, Oct 12, 2026 at 12:47 PM Keng-Yu Lin <keng-yu.lin@hpe.com> wrote:
>>
>> Commit 4d4c10f763d7 ("PCI: Explicitly put devices into D0 when
>> initializing") added pci_pm_power_up_and_verify_state() to pci_pm_init().
>> The helper ignores pci_power_up()'s return value indicating that BARs
>> need restoring.  Its resume callers restore saved configuration
>> afterwards; pci_pm_init() does not.
>>
>> D3hot-to-D0 may reset a device and lose its BARs (PCI PM 1.2, sec 5.4.1).
>> After enumeration powers it up, pci_enable_device() reads D0 and skips
>> the full power-up path.  Previously, a device still in D3hot at enable
>> time took that path, including BAR restoration.
>>
>> On AMD Venice SP7, two AMD xHCI controllers [1022:17d2] are in runtime
>> D3hot with No_Soft_Reset clear before kdump.  Diagnostic reads on
>> v7.3-rc6-37 show the following for 0000:01:00.4 in the crash kernel:
>>
>>                                     PMCSR      BAR0
>>    before the platform power call   0003       f4100004
>>    after the platform power call    0003       f4100004
>>    after native D0 power-up         0000       00000004
>>
>> The upper BAR DWORD stays zero.  pci_power_up() returns 1, but the BAR
>> address remains zero while dev->resource[0] still contains 0xf4100000.
>> Without BAR restoration, the last console messages are around:
>>
>>    pci 0000:01:00.4: enabling device (0000 -> 0002)
>>
>> Restoring both controllers' BARs gives matching readbacks and allows
>> kdump to save a vmcore and reboot.  An AMD Turin xHCI [1022:1557] shows
>> the same BAR loss; restoring its BAR also allows kdump to complete.
>>
>> Use pci_set_full_power_state() in pci_pm_init() to power up the device
>> and restore its BARs when needed.  It already checks the native power
>> state before restoring and updates ASPM when link state is present.
>> Leave pci_pm_power_up_and_verify_state() unchanged for the resume
>> callers, which restore saved configuration afterwards.
>>
>> Read the initial power state first, as pci_enable_device_flags() does.
> 
> Is this really necessary or even useful before pci_set_full_power_state()?
> 
> pci_power_up() updates dev->current_state regardless of its previous
> value AFAICS.
> 

To add to this comment; I would expect that if it's an ACPI device that 
it is currently in ACPI_STATE_UNKNOWN at the time this function is 
called.  So it should be a no-op for those devices.

I'd say you can just update current state since you're forcing it to D0.

>> Otherwise, PCI_UNKNOWN can request a BAR restore even for devices
>> already in D0.  Keep the final pci_update_current_state() call so the
>> final cached state still accounts for platform power and failed config
>> reads.
>>
>> Suggested-by: Mario Limonciello <mario.limonciello@amd.com>
>> Fixes: 4d4c10f763d7 ("PCI: Explicitly put devices into D0 when initializing")
>> Cc: stable@vger.kernel.org
>> Signed-off-by: Keng-Yu Lin <keng-yu.lin@hpe.com>
>> ---
>>
>> Changes in v2:
>> - Use pci_set_full_power_state() in pci_pm_init(), following Mario's
>>    suggestion to reuse the existing BAR restore code.
>> - Leave the shared resume helper and its interface unchanged.
>> - Keep the initial state readback and the final platform-aware refresh.
>>
>> Unlike v1, BAR restoration uses the full helper's native PMCSR check.
>> It also updates ASPM when link state exists.
>>
>> Tested v2 with kdump on both systems.  Normal boots made no BAR writes
>> from this path.  The crash kernels wrote only the cleared xHCI BARs:
>> two on Venice SP7 and one on Turin.  All readbacks matched.  kdump saved
>> 2.53 GB and 1.10 GB vmcores respectively before rebooting.
>>
>> v1: https://lore.kernel.org/r/20261008133708.947908-1-keng-yu.lin@hpe.com/
>> Review: https://lore.kernel.org/r/04f5463b-f123-4079-9286-06a241976d3a@amd.com/
>>
>>   drivers/pci/pci.c | 4 +++-
>>   1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
>> index 8cc6a89130b5..d3a5871e1b36 100644
>> --- a/drivers/pci/pci.c
>> +++ b/drivers/pci/pci.c
>> @@ -3255,7 +3255,9 @@ void pci_pm_init(struct pci_dev *dev)
>>          }
>>
>>   poweron:
>> -       pci_pm_power_up_and_verify_state(dev);
>> +       pci_update_current_state(dev, dev->current_state);
>> +       pci_set_full_power_state(dev, false);
>> +       pci_update_current_state(dev, PCI_D0);
>>          pm_runtime_forbid(&dev->dev);
>>
>>          /*
>>
>> base-commit: 7b63ef2d55f24519e7e9e5f4d15dbea03f126e40
>> --
>> 2.43.0