Re: [PATCH] PCI/ASPM: Avoid restore error L1ss cap.data to parent downstream PCIe port

["Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>]

On Mon, 20 Oct 2025, weipengliang wrote:

> The function pci_restore_aspm_l1ss_state will restore L1ss cap.data

Please add () after any function name. You can ten remove "The function" 
as it's obvious (or at least place "function" word after its name if you 
want to still write "The xx() function").

> in the upstream resume flow, to both downstream & upstream port.
> When the upstream port is suspended and the suspend flow is interrupted,
> the downstream port has not been suspended yet, so the L1ss cap.data is
> not correct.

What interrupts it, where? Both upstream and downstream port L1SS config 
are saved in the upstream port's pci_save_aspm_l1ss_state() so if upstream 
port is already suspended, why wasn't the downstream port's L1SS saved?

> Expectially the first Suspend/Reusme flow, the downstrem

Resume

downstream

> L1ss cap.data will be initialize to zero.
> When the Suspend/Resume flow is interrupted the time between

Please don't use "Suspend/Resume flow" like this. Both are not 
interrupted.

> upstream port has suspended but the downstream port hasn't,
> it will restore zero to the downstream port in the
> upstream port Resume flow.

This seems to repeat what was said in the earlier sentence, what is the 
difference or should the duplicate be removed?

The structure too here needs improvements. Please first state the issue, 
then explain the solution.

> Signed-off-by: weipengliang <weipengliang@xiaomi.com>
> ---
>  drivers/pci/pcie/aspm.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
> index 7cc8281e7011..173e0eb10b0d 100644
> --- a/drivers/pci/pcie/aspm.c
> +++ b/drivers/pci/pcie/aspm.c
> @@ -97,6 +97,9 @@ void pci_save_aspm_l1ss_state(struct pci_dev *pdev)
>  	if (!pdev->l1ss || !parent->l1ss)
>  		return;
>  
> +	if (!parent->state_saved)

There disconnection from the changelog's text to this check.

This is pci_save_aspm_l1ss_state(), not restore.

I'm also not convinced parent->state_saved is a robust indicator of what 
has been saved as ASPM config is saved at a different time than when rest 
of ->state_saved is set. We were bitten by this difference in timing 
earlier.

Not directly related to this patch, perhaps dev->state_saved = true; in 
pci_save_state() should be moved as the last line before return to avoid 
claiming state is saved before it truly is 100% saved.

> +		return;
> +
>  	/*
>  	 * Save L1 substate configuration. The ASPM L0s/L1 configuration
>  	 * in PCI_EXP_LNKCTL_ASPMC is saved by pci_save_pcie_state().
> 

-- 
 i.