Re: [PATCH] PCI: remove call pci_save_aspm_l1ss_state() from pci_save_pcie_state()

[Bjorn Helgaas <helgaas@kernel.org>]

[+cc Vidya, author of 4ff116d0d5fd]

On Mon, Jul 07, 2025 at 07:52:36PM +0800, weilinghan wrote:
> During the suspend-resume process, PCIe resumes by enabling L1.2
> in the pci_restore_state function due to patch 4ff116d0d5fd.
> However, in the following scenario, the resume process
> becomes very time-consuming:
> 
> 1.The platform has multiple PCI buses.
> 2.The link transition time from L1.2 to L0 exceeds 100 microseconds by
> accessing the configuration space of the EP.
> 3.The PCI framework has async_suspend enabled
> (by calling device_enable_async_suspend(&dev->dev)
> in pci_pm_init(struct pci_dev *dev)).
> 4.On ARM platforms, CONFIG_PCI_LOCKLESS_CONFIG is not enabled, which means
> the pci_bus_read_config_##size interfaces contain locks (spinlock).
> 
> Practical measurements show that enabling L1.2 during the
> resume process introduces an additional delay of approximately
> 150ms in the pci_pm_resume_noirq() function for platforms
> with two PCI buses, compared to when L1.2 is disabled.

We really need an argument for why this change would be correct, not
just the fact that it makes resume faster.  Vidya made the change in
4ff116d0d5fd to fix a problem, and it looks like this patch would
reintroduce the problem.

Nits:

  - Look at previous history and follow the subject line convention.

  - Add "()" after function names, e.g., pci_restore_state().

  - Omit "the ... function"; it's sufficient to say "in
    pci_restore_state()" and "in pci_pm_resume_noirq()".

  - Omit function parameter info since it's not really relevant here.

  - Cite commits as: 4ff116d0d5fd ("PCI/ASPM: Save L1 PM Substates
    Capability for suspend/resume")

  - s/4.0n/4/

> Signed-off-by: weilinghan <weilinghan@xiaomi.com>
> ---
>  drivers/pci/pci.c | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index 9e42090fb108..0834211b0f8c 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -1708,7 +1708,6 @@ static int pci_save_pcie_state(struct pci_dev *dev)
>  	pcie_capability_read_word(dev, PCI_EXP_LNKCTL2, &cap[i++]);
>  	pcie_capability_read_word(dev, PCI_EXP_SLTCTL2, &cap[i++]);
>  
> -	pci_save_aspm_l1ss_state(dev);
>  	pci_save_ltr_state(dev);
>  
>  	return 0;
> @@ -1725,7 +1724,6 @@ static void pci_restore_pcie_state(struct pci_dev *dev)
>  	 * LTR itself in PCI_EXP_DEVCTL2.
>  	 */
>  	pci_restore_ltr_state(dev);
> -	pci_restore_aspm_l1ss_state(dev);
>  
>  	save_state = pci_find_saved_cap(dev, PCI_CAP_ID_EXP);
>  	if (!save_state)
> -- 
> 2.43.0
>