Re: [PATCH] PCI/ASPM: Disable L1 before disabling L1ss

[Bjorn Helgaas <helgaas@kernel.org>]

On Tue, Oct 01, 2024 at 07:05:18PM +0530, Ajay Agarwal wrote:
> The current sequence in the driver for L1ss update is as follows.
> 
> Disable L1ss
> Disable L1
> Enable L1ss as required
> Enable L1 if required
> 
> PCIe spec r6.2, section 5.5.4, recommends that setting either
> or both of the enable bits for ASPM L1 PM Substates must be done
> while ASPM L1 is disabled. My interpretation here is that
> clearing L1ss should also be done when L1 is disabled. Thereby,
> change the sequence as follows.
> 
> Disable L1
> Disable L1ss
> Enable L1ss as required
> Enable L1 if required

This seems reasonable to me.  If you have seen a failure that is fixed
by this change, please include some details here.

> Signed-off-by: Ajay Agarwal <ajayagarwal@google.com>
> ---
>  drivers/pci/pcie/aspm.c | 22 ++++++++--------------
>  1 file changed, 8 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
> index cee2365e54b8..d37f66f9e9c8 100644
> --- a/drivers/pci/pcie/aspm.c
> +++ b/drivers/pci/pcie/aspm.c
> @@ -848,16 +848,14 @@ static void pcie_aspm_cap_init(struct pcie_link_state *link, int blacklist)
>  /* Configure the ASPM L1 substates */
>  static void pcie_config_aspm_l1ss(struct pcie_link_state *link, u32 state)
>  {
> -	u32 val, enable_req;
> +	u32 val;
>  	struct pci_dev *child = link->downstream, *parent = link->pdev;
>  
> -	enable_req = (link->aspm_enabled ^ state) & state;
> -
>  	/*
>  	 * Here are the rules specified in the PCIe spec for enabling L1SS:
>  	 * - When enabling L1.x, enable bit at parent first, then at child
>  	 * - When disabling L1.x, disable bit at child first, then at parent
> -	 * - When enabling ASPM L1.x, need to disable L1
> +	 * - When enabling/disabling ASPM L1.x, need to disable L1
>  	 *   (at child followed by parent).
>  	 * - The ASPM/PCIPM L1.2 must be disabled while programming timing
>  	 *   parameters

Since you're updating this comment already, can you add the spec
citation here, e.g., "PCIe r6.2, sec 5.5.4"?

> @@ -866,21 +864,17 @@ static void pcie_config_aspm_l1ss(struct pcie_link_state *link, u32 state)
>  	 * what is needed.
>  	 */
>  
> +	/* Disable L1, and it gets enabled later in pcie_config_aspm_link() */
> +	pcie_capability_clear_word(child, PCI_EXP_LNKCTL,
> +				   PCI_EXP_LNKCTL_ASPM_L1);
> +	pcie_capability_clear_word(parent, PCI_EXP_LNKCTL,
> +				   PCI_EXP_LNKCTL_ASPM_L1);

I think it would be nice to have the disable and enable in the same
place.  Could we move this to pcie_config_aspm_link()?  I'm not sure
the pcie_config_aspm_dev() wrapper adds a lot of value, but we could
at least do both ends using the same wrapper.

pcie_config_aspm_link() configures all children; here we only do one
child.  I suspect we should do disable L1 for all of them here, and
doing both in pcie_config_aspm_link() would make that clearer.

pcie_config_aspm_link() has a comment ("Spec 2.0 ...") about the
configuration order, but I'd like to update that, add a section
reference, and make sure we do the disable in the right order.

>  	/* Disable all L1 substates */
>  	pci_clear_and_set_config_dword(child, child->l1ss + PCI_L1SS_CTL1,
>  				       PCI_L1SS_CTL1_L1SS_MASK, 0);
>  	pci_clear_and_set_config_dword(parent, parent->l1ss + PCI_L1SS_CTL1,
>  				       PCI_L1SS_CTL1_L1SS_MASK, 0);
> -	/*
> -	 * If needed, disable L1, and it gets enabled later
> -	 * in pcie_config_aspm_link().
> -	 */
> -	if (enable_req & (PCIE_LINK_STATE_L1_1 | PCIE_LINK_STATE_L1_2)) {
> -		pcie_capability_clear_word(child, PCI_EXP_LNKCTL,
> -					   PCI_EXP_LNKCTL_ASPM_L1);
> -		pcie_capability_clear_word(parent, PCI_EXP_LNKCTL,
> -					   PCI_EXP_LNKCTL_ASPM_L1);
> -	}
>  
>  	val = 0;
>  	if (state & PCIE_LINK_STATE_L1_1)
> -- 
> 2.46.1.824.gd892dcdcdd-goog
>