diff mbox series

[RFC,v1,2/4] PCI/ASPM: Remove struct pcie_link_state.clkpm_capable

Message ID 20210929004400.25717-3-refactormyself@gmail.com (mailing list archive)
State Superseded
Delegated to: Bjorn Helgaas
Headers show
Series Remove struct pcie_link_state.clkpm_* | expand

Commit Message

Saheed O. Bolarinwa Sept. 29, 2021, 12:43 a.m. UTC 
From: "Bolarinwa O. Saheed" <refactormyself@gmail.com>

The clkpm_capable member of the struct pcie_link_state indicates
if the device is Clock PM capable. This can be calculated when
it is needed.

This patch:
  - removes clkpm_capable from struct pcie_link_state
  - moves the calculation of clkpm_capable into
    pcie_is_clkpm_capable()
  - replaces references to clkpm_capable with a call to
    pcie_is_clkpm_capable()

Signed-off-by: Bolarinwa O. Saheed <refactormyself@gmail.com>
---
 drivers/pci/pcie/aspm.c | 39 ++++++++++++++++++++++-----------------
 1 file changed, 22 insertions(+), 17 deletions(-)

Comments

Bjorn Helgaas Sept. 30, 2021, 4:05 p.m. UTC | #1 
On Wed, Sep 29, 2021 at 02:43:58AM +0200, Saheed O. Bolarinwa wrote:
> From: "Bolarinwa O. Saheed" <refactormyself@gmail.com>
> 
> The clkpm_capable member of the struct pcie_link_state indicates
> if the device is Clock PM capable. This can be calculated when
> it is needed.

... because it comes from Link Capabilities, a read-only register.

> This patch:
>   - removes clkpm_capable from struct pcie_link_state
>   - moves the calculation of clkpm_capable into
>     pcie_is_clkpm_capable()
>   - replaces references to clkpm_capable with a call to
>     pcie_is_clkpm_capable()

No need to say "this patch"; commit logs *always* refer to "this
patch".  Reword in imperative sentence structure, as if giving
commands, e.g.,

  Remove pcie_link_state.clkpm_capable and replace it with
  pcie_is_clkpm_capable(), which computes the value from Link
  Capabilities as needed.

> Signed-off-by: Bolarinwa O. Saheed <refactormyself@gmail.com>
> ---
>  drivers/pci/pcie/aspm.c | 39 ++++++++++++++++++++++-----------------
>  1 file changed, 22 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
> index c23da9a4e2fb..9e65da9a22dd 100644
> --- a/drivers/pci/pcie/aspm.c
> +++ b/drivers/pci/pcie/aspm.c
> @@ -61,7 +61,6 @@ struct pcie_link_state {
>  	u32 aspm_disable:7;		/* Disabled ASPM state */
>  
>  	/* Clock PM state */
> -	u32 clkpm_capable:1;		/* Clock PM capable? */
>  	u32 clkpm_enabled:1;		/* Current Clock PM state */
>  	u32 clkpm_disable:1;		/* Clock PM disabled */
>  
> @@ -146,6 +145,25 @@ static int pcie_get_clkpm_state(struct pci_dev *pdev)
>  	return enabled;
>  }
>  
> +static int pcie_is_clkpm_capable(struct pci_dev *pdev)
> +{
> +	int capable = 1;
> +	u32 reg32;
> +	struct pci_dev *child;
> +	struct pci_bus *linkbus = pdev->subordinate;
> +
> +	/* All functions should have the same cap state, take the worst */
> +	list_for_each_entry(child, &linkbus->devices, bus_list) {
> +		pcie_capability_read_dword(child, PCI_EXP_LNKCAP, &reg32);
> +		if (!(reg32 & PCI_EXP_LNKCAP_CLKPM)) {
> +			capable = 0;
> +			break;
> +		}
> +	}
> +
> +	return capable;

  static bool pcie_is_clkpm_capable()
  {
    list_for_each_entry(child, &linkbus->devices, bus_list) {
      pcie_capability_read_dword(child, PCI_EXP_LNKCAP, &cap);
      if (!(cap & PCI_EXP_LNKCAP_CLKPM))
	return false;
    }
    return true;
  }

> +}
> +
>  static int policy_to_clkpm_state(struct pcie_link_state *link)
>  {
>  	switch (aspm_policy) {
> @@ -177,11 +195,12 @@ static void pcie_set_clkpm_nocheck(struct pcie_link_state *link, int enable)
>  
>  static void pcie_set_clkpm(struct pcie_link_state *link, int enable)
>  {
> +	int capable = pcie_is_clkpm_capable(link->pdev);
>  	/*
>  	 * Don't enable Clock PM if the link is not Clock PM capable
>  	 * or Clock PM is disabled
>  	 */
> -	if (!link->clkpm_capable || link->clkpm_disable)
> +	if (!capable || link->clkpm_disable)

I think I'd just call pcie_is_clkpm_capable() directly in the "if"
condition to avoid the local variable, as you did below in
aspm_ctrl_attrs_are_visible().

>  		enable = 0;
>  	/* Need nothing if the specified equals to current state */
>  	if (link->clkpm_enabled == enable)
> @@ -191,21 +210,7 @@ static void pcie_set_clkpm(struct pcie_link_state *link, int enable)
>  
>  static void pcie_clkpm_cap_init(struct pcie_link_state *link, int blacklist)
>  {
> -	int capable = 1;
> -	u32 reg32;
> -	struct pci_dev *child;
> -	struct pci_bus *linkbus = link->pdev->subordinate;
> -
> -	/* All functions should have the same cap and state, take the worst */
> -	list_for_each_entry(child, &linkbus->devices, bus_list) {
> -		pcie_capability_read_dword(child, PCI_EXP_LNKCAP, &reg32);
> -		if (!(reg32 & PCI_EXP_LNKCAP_CLKPM)) {
> -			capable = 0;
> -			break;
> -		}
> -	}
>  	link->clkpm_enabled = pcie_get_clkpm_state(link->pdev);
> -	link->clkpm_capable = capable;

This makes good sense to me because PCI_EXP_LNKCAP_CLKPM is read-only
and there's no need to cache the boot-time value because we can always
recompute clkpm_capable if we need it.

>  	link->clkpm_disable = blacklist ? 1 : 0;
>  }
>  
> @@ -1346,7 +1351,7 @@ static umode_t aspm_ctrl_attrs_are_visible(struct kobject *kobj,
>  		return 0;
>  
>  	if (n == 0)
> -		return link->clkpm_capable ? a->mode : 0;
> +		return pcie_is_clkpm_capable(link->pdev) ? a->mode : 0;
>  
>  	return link->aspm_capable & aspm_state_map[n - 1] ? a->mode : 0;
>  }
> -- 
> 2.20.1
>
diff mbox series

Patch

diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
index c23da9a4e2fb..9e65da9a22dd 100644
--- a/drivers/pci/pcie/aspm.c
+++ b/drivers/pci/pcie/aspm.c
@@ -61,7 +61,6 @@  struct pcie_link_state {
 	u32 aspm_disable:7;		/* Disabled ASPM state */
 
 	/* Clock PM state */
-	u32 clkpm_capable:1;		/* Clock PM capable? */
 	u32 clkpm_enabled:1;		/* Current Clock PM state */
 	u32 clkpm_disable:1;		/* Clock PM disabled */
 
@@ -146,6 +145,25 @@  static int pcie_get_clkpm_state(struct pci_dev *pdev)
 	return enabled;
 }
 
+static int pcie_is_clkpm_capable(struct pci_dev *pdev)
+{
+	int capable = 1;
+	u32 reg32;
+	struct pci_dev *child;
+	struct pci_bus *linkbus = pdev->subordinate;
+
+	/* All functions should have the same cap state, take the worst */
+	list_for_each_entry(child, &linkbus->devices, bus_list) {
+		pcie_capability_read_dword(child, PCI_EXP_LNKCAP, &reg32);
+		if (!(reg32 & PCI_EXP_LNKCAP_CLKPM)) {
+			capable = 0;
+			break;
+		}
+	}
+
+	return capable;
+}
+
 static int policy_to_clkpm_state(struct pcie_link_state *link)
 {
 	switch (aspm_policy) {
@@ -177,11 +195,12 @@  static void pcie_set_clkpm_nocheck(struct pcie_link_state *link, int enable)
 
 static void pcie_set_clkpm(struct pcie_link_state *link, int enable)
 {
+	int capable = pcie_is_clkpm_capable(link->pdev);
 	/*
 	 * Don't enable Clock PM if the link is not Clock PM capable
 	 * or Clock PM is disabled
 	 */
-	if (!link->clkpm_capable || link->clkpm_disable)
+	if (!capable || link->clkpm_disable)
 		enable = 0;
 	/* Need nothing if the specified equals to current state */
 	if (link->clkpm_enabled == enable)
@@ -191,21 +210,7 @@  static void pcie_set_clkpm(struct pcie_link_state *link, int enable)
 
 static void pcie_clkpm_cap_init(struct pcie_link_state *link, int blacklist)
 {
-	int capable = 1;
-	u32 reg32;
-	struct pci_dev *child;
-	struct pci_bus *linkbus = link->pdev->subordinate;
-
-	/* All functions should have the same cap and state, take the worst */
-	list_for_each_entry(child, &linkbus->devices, bus_list) {
-		pcie_capability_read_dword(child, PCI_EXP_LNKCAP, &reg32);
-		if (!(reg32 & PCI_EXP_LNKCAP_CLKPM)) {
-			capable = 0;
-			break;
-		}
-	}
 	link->clkpm_enabled = pcie_get_clkpm_state(link->pdev);
-	link->clkpm_capable = capable;
 	link->clkpm_disable = blacklist ? 1 : 0;
 }
 
@@ -1346,7 +1351,7 @@  static umode_t aspm_ctrl_attrs_are_visible(struct kobject *kobj,
 		return 0;
 
 	if (n == 0)
-		return link->clkpm_capable ? a->mode : 0;
+		return pcie_is_clkpm_capable(link->pdev) ? a->mode : 0;
 
 	return link->aspm_capable & aspm_state_map[n - 1] ? a->mode : 0;
 }