diff mbox series

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

Message ID 20210916084926.32614-3-refactormyself@gmail.com (mailing list archive)
State Superseded
Delegated to: Bjorn Helgaas
Headers show
Series PCI/ASPM: Remove struct aspm_latency | expand

Commit Message

Saheed O. Bolarinwa Sept. 16, 2021, 8:49 a.m. UTC
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@gmail.com>
---
 drivers/pci/pcie/aspm.c | 27 ++++++++-------------------
 1 file changed, 8 insertions(+), 19 deletions(-)

Comments

Bjorn Helgaas Sept. 30, 2021, 11:52 p.m. UTC | #1
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@gmail.com>
> ---
>  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
>
diff mbox series

Patch

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);
+	/* 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);
 
 	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);
 	}
 }