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

[Rafael J. Wysocki (Intel)]

On Mon, Oct 12, 2026 at 12:47 PM Keng-Yu Lin <keng-yu.lin@xxxxxxx> 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.

> 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@xxxxxxx>
> Fixes: 4d4c10f763d7 ("PCI: Explicitly put devices into D0 when initializing")
> Cc: stable@xxxxxxxxxxxxxxx
> Signed-off-by: Keng-Yu Lin <keng-yu.lin@xxxxxxx>
> ---
>
> 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@xxxxxxx/
> Review: https://lore.kernel.org/r/04f5463b-f123-4079-9286-06a241976d3a@xxxxxxx/
>
>  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