Re: [PATCH 1/1] PCI/ASPM: Fix L1SS saving

[Bjorn Helgaas]

[+cc Rafael]

On Fri, Jan 31, 2025 at 05:29:13PM +0200, Ilpo Järvinen wrote:
> The commit 1db806ec06b7 ("PCI/ASPM: Save parent L1SS config in
> pci_save_aspm_l1ss_state()") aimed to perform L1SS config save for both
> the Upstream Port and its upstream bridge when handling an Upstream
> Port, which matches what the L1SS restore side does. However,
> parent->state_saved can be set true at an earlier time when the
> upstream bridge saved other parts of its state. 

So I guess the scenario is that we got here because some driver called
pci_save_state(pdev):

  pci_save_state
    dev->state_saved = true                <--
    pci_save_pcie_state
      pci_save_aspm_l1ss_state
        if (pcie_downstream_port(pdev))
          return
        # save pdev L1SS state here
        if (parent->state_saved)           <--
          return
        # save parent L1SS state here

and the problem is that we previously called pci_save_state(parent),
which set "parent->state_saved = true" but did not save its L1SS state
because pci_save_aspm_l1ss_state() is a no-op for Downstream Ports,
right?

But I would think this would be a very common situation because
pcie_portdrv_probe() calls pci_save_state() for Downstream Ports when
pciehp, AER, PME, etc, are enabled, and this would happen before the
pci_save_state() calls from Endpoint drivers.

So I'm a little surprised that this didn't blow up for everybody
immediately.  Is there something that makes this unusual?

> Then later when
> attempting to save the L1SS config while handling the Upstream Port,
> parent->state_saved is true in pci_save_aspm_l1ss_state() resulting in
> early return and skipping saving bridge's L1SS config because it is
> assumed to be already saved. Later on restore, junk is written into
> L1SS config which causes issues with some devices.
> 
> Remove parent->state_saved check and unconditionally save L1SS config
> also for the upstream bridge from an Upstream Port which ought to be
> harmless from correctness point of view. With the Upstream Port check
> now present, saving the L1SS config more than once for the bridge is no
> longer a problem (unlike when the parent->state_saved check got
> introduced into the fix during its development).
> 
> Fixes: 1db806ec06b7 ("PCI/ASPM: Save parent L1SS config in pci_save_aspm_l1ss_state()")
> Closes: https://bugzilla.kernel.org/show_bug.cgi?id=219731
> Reported-by: Niklāvs Koļesņikovs <pinkflames.linux@xxxxxxxxx>
> Tested-by: Niklāvs Koļesņikovs <pinkflames.linux@xxxxxxxxx>
> Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@xxxxxxxxxxxxxxx>
> ---
>  drivers/pci/pcie/aspm.c | 3 ---
>  1 file changed, 3 deletions(-)
> 
> diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
> index e0bc90597dca..da3e7edcf49d 100644
> --- a/drivers/pci/pcie/aspm.c
> +++ b/drivers/pci/pcie/aspm.c
> @@ -108,9 +108,6 @@ void pci_save_aspm_l1ss_state(struct pci_dev *pdev)
>  	pci_read_config_dword(pdev, pdev->l1ss + PCI_L1SS_CTL2, cap++);
>  	pci_read_config_dword(pdev, pdev->l1ss + PCI_L1SS_CTL1, cap++);
>  
> -	if (parent->state_saved)
> -		return;
> -
>  	/*
>  	 * Save parent's L1 substate configuration so we have it for
>  	 * pci_restore_aspm_l1ss_state(pdev) to restore.
> 
> base-commit: 72deda0abee6e705ae71a93f69f55e33be5bca5c
> -- 
> 2.39.5
>