Re: [RFC PATCH 2/3 v2] PCI/ASPM: Remove struct pcie_link_state.acceptable

[Bjorn Helgaas]

On Thu, Sep 16, 2021 at 10:49:25AM +0200, Saheed O. Bolarinwa wrote:
> The acceptable latencies for each device on the bus are calculated within
> pcie_aspm_cap_init() and cached in struct pcie_link_state.acceptable. They
> are only used in pcie_aspm_check_latency() to validate actual latencies.
> Thus, it is possible to avoid caching these values.
> 
> This patch:
>   - removes `acceptable` from struct pcie_link_state
>   - calculates the acceptable latency for each device directly
>   - removes the calculations done within pcie_aspm_cap_init()
> 
> Signed-off-by: Saheed O. Bolarinwa <refactormyself@xxxxxxxxx>
> ---
>  drivers/pci/pcie/aspm.c | 27 ++++++++-------------------
>  1 file changed, 8 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
> index 9e85dfc56657..0c0c055823f1 100644
> --- a/drivers/pci/pcie/aspm.c
> +++ b/drivers/pci/pcie/aspm.c
> @@ -65,12 +65,6 @@ struct pcie_link_state {
>  	u32 clkpm_enabled:1;		/* Current Clock PM state */
>  	u32 clkpm_default:1;		/* Default Clock PM state by BIOS */
>  	u32 clkpm_disable:1;		/* Clock PM disabled */
> -
> -	/*
> -	 * Endpoint acceptable latencies. A pcie downstream port only
> -	 * has one slot under it, so at most there are 8 functions.
> -	 */
> -	struct aspm_latency acceptable[8];
>  };
>  
>  static int aspm_disabled, aspm_force;
> @@ -389,7 +383,7 @@ static struct pci_dev *pci_function_0(struct pci_bus *linkbus)
>  
>  static void pcie_aspm_check_latency(struct pci_dev *endpoint)
>  {
> -	u32 latency, lnkcap_up, lnkcap_dw, l1_switch_latency = 0;
> +	u32 reg32, latency, encoding, lnkcap_up, lnkcap_dw, l1_switch_latency = 0;
>  	struct pci_dev *downstream;
>  	struct aspm_latency latency_up, latency_dw;
>  	struct aspm_latency *acceptable;
> @@ -402,7 +396,13 @@ static void pcie_aspm_check_latency(struct pci_dev *endpoint)
>  
>  	link = endpoint->bus->self->link_state;
>  	downstream = pci_function_0(link->pdev->subordinate);
> -	acceptable = &link->acceptable[PCI_FUNC(endpoint->devfn)];
> +	pcie_capability_read_dword(endpoint, PCI_EXP_DEVCAP, &reg32);

I think you can use endpoint->devcap here, can't you?

> +	/* Calculate endpoint L0s acceptable latency */
> +	encoding = (reg32 & PCI_EXP_DEVCAP_L0S) >> 6;
> +	acceptable->l0s = calc_l0s_acceptable(encoding);
> +	/* Calculate endpoint L1 acceptable latency */
> +	encoding = (reg32 & PCI_EXP_DEVCAP_L1) >> 9;
> +	acceptable->l1 = calc_l1_acceptable(encoding);

I think it's a little weird that we call pcie_aspm_check_latency() for
all the functions in a multi-function device.  It's true that they can
all have different acceptable latencies, but they're all on the
downstream end of the same link, so they will all see the same exit
latencies.

I would think we could compute the minimum latency across all the
functions and do a single check to see whether the link can meet that.
But that would be material for a future patch, not this one.

>  	while (link) {
>  		/* Read direction exit latencies */
> @@ -664,22 +664,11 @@ static void pcie_aspm_cap_init(struct pcie_link_state *link, int blacklist)
>  
>  	/* Get and check endpoint acceptable latencies */
>  	list_for_each_entry(child, &linkbus->devices, bus_list) {
> -		u32 reg32, encoding;
> -		struct aspm_latency *acceptable =
> -			&link->acceptable[PCI_FUNC(child->devfn)];
>  
>  		if (pci_pcie_type(child) != PCI_EXP_TYPE_ENDPOINT &&
>  		    pci_pcie_type(child) != PCI_EXP_TYPE_LEG_END)
>  			continue;
>  
> -		pcie_capability_read_dword(child, PCI_EXP_DEVCAP, &reg32);
> -		/* Calculate endpoint L0s acceptable latency */
> -		encoding = (reg32 & PCI_EXP_DEVCAP_L0S) >> 6;
> -		acceptable->l0s = calc_l0s_acceptable(encoding);
> -		/* Calculate endpoint L1 acceptable latency */
> -		encoding = (reg32 & PCI_EXP_DEVCAP_L1) >> 9;
> -		acceptable->l1 = calc_l1_acceptable(encoding);
> -
>  		pcie_aspm_check_latency(child);
>  	}
>  }
> -- 
> 2.20.1
>