[RFC PATCH v2 0/4] Remove unncessary linked list from aspm.c

[RFC PATCH v2 0/4] Remove unncessary linked list from aspm.c

[Saheed O. Bolarinwa]

From: "Bolarinwa O. Saheed" <refactormyself@gmail.com>

An extra linked list is created inside aspm.c to keep track of devices
on which the link state was enabled. However, it is possible to access
them via existing device lists.

This series remove the extra linked list and other related members of
the struct pcie_link_state: `root`, `parent` and `downstream`. All
these are now either calculated or obtained directly when needed.



VERSION CHANGES:
 - v2:
»       - Avoid using BUG_ON()
»       - Create helper function pci_get_parent()
»       - Fix a bug from the previous version

MERGE NOTICE:
These series are based on
»       'commit e4e737bb5c17 ("Linux 5.15-rc2")'

Bolarinwa O. Saheed (4):
  PCI/ASPM: Remove struct pcie_link_state.parent
  PCI/ASPM: Remove struct pcie_link_state.root
  PCI/ASPM: Remove struct pcie_link_state.downstream
  PCI/ASPM: Remove unncessary linked list from aspm.c

 drivers/pci/pcie/aspm.c | 134 +++++++++++++++++++++++-----------------
 1 file changed, 78 insertions(+), 56 deletions(-)

-- 
2.20.1

[RFC PATCH v2 1/4] PCI/ASPM: Remove struct pcie_link_state.parent

[Saheed O. Bolarinwa]

From: "Bolarinwa O. Saheed" <refactormyself@gmail.com>

Information cached in struct pcie_link_state.parent is accessible
via struct pci_dev.

This patch:
 - removes *parent* from the *struct pcie_link_state*
 - creates pci_get_parent() which returns the parent of a pci_dev
 - replaces references to pcie_link_state.parent with a call to
   pci_get_parent()
 - removes BUG_ON(root->parent), instead uses the parent's root

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

diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
index 013a47f587ce..414c04ffe962 100644
--- a/drivers/pci/pcie/aspm.c
+++ b/drivers/pci/pcie/aspm.c
@@ -50,7 +50,6 @@ struct pcie_link_state {
 	struct pci_dev *pdev;		/* Upstream component of the Link */
 	struct pci_dev *downstream;	/* Downstream component, function 0 */
 	struct pcie_link_state *root;	/* pointer to the root port link */
-	struct pcie_link_state *parent;	/* pointer to the parent Link state */
 	struct list_head sibling;	/* node in link_list */
 
 	/* ASPM state */
@@ -139,6 +138,14 @@ static int policy_to_clkpm_state(struct pcie_link_state *link)
 	return 0;
 }
 
+static struct pci_dev *pci_get_parent(struct pci_dev *pdev)
+{
+	if (!pdev || !pdev->bus->parent || !pdev->bus->parent->self)
+		return NULL;
+
+	return pdev->bus->parent->self;
+}
+
 static void pcie_set_clkpm_nocheck(struct pcie_link_state *link, int enable)
 {
 	struct pci_dev *child;
@@ -379,6 +386,7 @@ static void encode_l12_threshold(u32 threshold_us, u32 *scale, u32 *value)
 static void pcie_aspm_check_latency(struct pci_dev *endpoint)
 {
 	u32 latency, l1_switch_latency = 0;
+	struct pci_dev *parent;
 	struct aspm_latency *acceptable;
 	struct pcie_link_state *link;
 
@@ -419,7 +427,8 @@ static void pcie_aspm_check_latency(struct pci_dev *endpoint)
 			link->aspm_capable &= ~ASPM_STATE_L1;
 		l1_switch_latency += 1000;
 
-		link = link->parent;
+		parent = pci_get_parent(link->pdev);
+		link = parent ? parent->link_state : NULL;
 	}
 }
 
@@ -793,9 +802,11 @@ static void pcie_config_aspm_link(struct pcie_link_state *link, u32 state)
 
 static void pcie_config_aspm_path(struct pcie_link_state *link)
 {
+	struct pci_dev *parent;
 	while (link) {
 		pcie_config_aspm_link(link, policy_to_aspm_state(link));
-		link = link->parent;
+		parent = pci_get_parent(link->pdev);
+		link = parent ? parent->link_state : NULL;
 	}
 }
 
@@ -864,16 +875,15 @@ static struct pcie_link_state *alloc_pcie_link_state(struct pci_dev *pdev)
 	    !pdev->bus->parent->self) {
 		link->root = link;
 	} else {
-		struct pcie_link_state *parent;
+		struct pci_dev *parent;
 
-		parent = pdev->bus->parent->self->link_state;
-		if (!parent) {
+		parent = pci_get_parent(pdev);
+		if (!parent->link_state) {
 			kfree(link);
 			return NULL;
 		}
 
-		link->parent = parent;
-		link->root = link->parent->root;
+		link->root = parent->link_state->root;
 	}
 
 	list_add(&link->sibling, &link_list);
@@ -962,7 +972,11 @@ void pcie_aspm_init_link_state(struct pci_dev *pdev)
 static void pcie_update_aspm_capable(struct pcie_link_state *root)
 {
 	struct pcie_link_state *link;
-	BUG_ON(root->parent);
+	struct pci_dev *parent = pci_get_parent(root->pdev);
+
+	if (parent && parent->link_state)
+		root = parent->link_state->root;
+
 	list_for_each_entry(link, &link_list, sibling) {
 		if (link->root != root)
 			continue;
@@ -985,6 +999,7 @@ static void pcie_update_aspm_capable(struct pcie_link_state *root)
 /* @pdev: the endpoint device */
 void pcie_aspm_exit_link_state(struct pci_dev *pdev)
 {
+	struct pci_dev *parent_dev;
 	struct pci_dev *parent = pdev->bus->self;
 	struct pcie_link_state *link, *root, *parent_link;
 
@@ -1002,7 +1017,8 @@ void pcie_aspm_exit_link_state(struct pci_dev *pdev)
 
 	link = parent->link_state;
 	root = link->root;
-	parent_link = link->parent;
+	parent_dev = pci_get_parent(link->pdev);
+	parent_link = parent_dev ? parent_dev->link_state : NULL;
 
 	/* All functions are removed, so just disable ASPM for the link */
 	pcie_config_aspm_link(link, 0);
-- 
2.20.1

[RFC PATCH v2 2/4] PCI/ASPM: Remove struct pcie_link_state.root

[Saheed O. Bolarinwa]

From: "Bolarinwa O. Saheed" <refactormyself@gmail.com>

Information on the root device is cached in
struct pcie_link_state.root it obtained within
alloc_pcie_link_state().

This patch:
 - creates *pcie_get_root()* which return the root pci_dev
   of a pci_dev.
 - removes *root* from the *struct pcie_link_state*.
 - replaces references to struct pcie_link_state.root with
   a call to pcie_get_root().

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

diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
index 414c04ffe962..ad78aaeea444 100644
--- a/drivers/pci/pcie/aspm.c
+++ b/drivers/pci/pcie/aspm.c
@@ -49,7 +49,6 @@ struct aspm_latency {
 struct pcie_link_state {
 	struct pci_dev *pdev;		/* Upstream component of the Link */
 	struct pci_dev *downstream;	/* Downstream component, function 0 */
-	struct pcie_link_state *root;	/* pointer to the root port link */
 	struct list_head sibling;	/* node in link_list */
 
 	/* ASPM state */
@@ -851,6 +850,25 @@ static int pcie_aspm_sanity_check(struct pci_dev *pdev)
 	return 0;
 }
 
+/*
+ * Root Ports and PCI/PCI-X to PCIe Bridges are roots of PCIe
+ * hierarchies.  Note that some PCIe host implementations omit
+ * the root ports entirely, in which case a downstream port on
+ * a switch may become the root of the link state chain for all
+ * its subordinate endpoints.
+ */
+static struct pci_dev *pcie_get_root(struct pci_dev *pdev)
+{
+	struct pci_dev *parent = pci_get_parent(pdev);
+
+	if (pci_pcie_type(pdev) == PCI_EXP_TYPE_ROOT_PORT ||
+	    pci_pcie_type(pdev) == PCI_EXP_TYPE_PCIE_BRIDGE || !parent) {
+		return pdev;
+	} else {
+		return pcie_get_root(parent);
+	}
+}
+
 static struct pcie_link_state *alloc_pcie_link_state(struct pci_dev *pdev)
 {
 	struct pcie_link_state *link;
@@ -863,29 +881,6 @@ static struct pcie_link_state *alloc_pcie_link_state(struct pci_dev *pdev)
 	link->pdev = pdev;
 	link->downstream = pci_function_0(pdev->subordinate);
 
-	/*
-	 * Root Ports and PCI/PCI-X to PCIe Bridges are roots of PCIe
-	 * hierarchies.  Note that some PCIe host implementations omit
-	 * the root ports entirely, in which case a downstream port on
-	 * a switch may become the root of the link state chain for all
-	 * its subordinate endpoints.
-	 */
-	if (pci_pcie_type(pdev) == PCI_EXP_TYPE_ROOT_PORT ||
-	    pci_pcie_type(pdev) == PCI_EXP_TYPE_PCIE_BRIDGE ||
-	    !pdev->bus->parent->self) {
-		link->root = link;
-	} else {
-		struct pci_dev *parent;
-
-		parent = pci_get_parent(pdev);
-		if (!parent->link_state) {
-			kfree(link);
-			return NULL;
-		}
-
-		link->root = parent->link_state->root;
-	}
-
 	list_add(&link->sibling, &link_list);
 	pdev->link_state = link;
 	return link;
@@ -972,21 +967,26 @@ void pcie_aspm_init_link_state(struct pci_dev *pdev)
 static void pcie_update_aspm_capable(struct pcie_link_state *root)
 {
 	struct pcie_link_state *link;
-	struct pci_dev *parent = pci_get_parent(root->pdev);
+	struct pci_dev *dev, *root_dev;
 
-	if (parent && parent->link_state)
-		root = parent->link_state->root;
+	/* Ensure it is the root device */
+	root_dev = pcie_get_root(root->pdev);
+	root = root_dev ? root_dev->link_state : NULL;
 
 	list_for_each_entry(link, &link_list, sibling) {
-		if (link->root != root)
+		dev = pcie_get_root(link->pdev);
+		if (dev->link_state != root)
 			continue;
+
 		link->aspm_capable = link->aspm_support;
 	}
 	list_for_each_entry(link, &link_list, sibling) {
 		struct pci_dev *child;
 		struct pci_bus *linkbus = link->pdev->subordinate;
-		if (link->root != root)
+		dev = pcie_get_root(link->pdev);
+		if (dev->link_state != root)
 			continue;
+
 		list_for_each_entry(child, &linkbus->devices, bus_list) {
 			if ((pci_pcie_type(child) != PCI_EXP_TYPE_ENDPOINT) &&
 			    (pci_pcie_type(child) != PCI_EXP_TYPE_LEG_END))
@@ -999,9 +999,9 @@ static void pcie_update_aspm_capable(struct pcie_link_state *root)
 /* @pdev: the endpoint device */
 void pcie_aspm_exit_link_state(struct pci_dev *pdev)
 {
-	struct pci_dev *parent_dev;
+	struct pci_dev *parent_dev, *root_dev;
 	struct pci_dev *parent = pdev->bus->self;
-	struct pcie_link_state *link, *root, *parent_link;
+	struct pcie_link_state *link, *parent_link;
 
 	if (!parent || !parent->link_state)
 		return;
@@ -1016,7 +1016,7 @@ void pcie_aspm_exit_link_state(struct pci_dev *pdev)
 		goto out;
 
 	link = parent->link_state;
-	root = link->root;
+	root_dev = pcie_get_root(link->pdev);
 	parent_dev = pci_get_parent(link->pdev);
 	parent_link = parent_dev ? parent_dev->link_state : NULL;
 
@@ -1028,7 +1028,7 @@ void pcie_aspm_exit_link_state(struct pci_dev *pdev)
 
 	/* Recheck latencies and configure upstream links */
 	if (parent_link) {
-		pcie_update_aspm_capable(root);
+		pcie_update_aspm_capable(root_dev->link_state);
 		pcie_config_aspm_path(parent_link);
 	}
 out:
@@ -1040,6 +1040,7 @@ void pcie_aspm_exit_link_state(struct pci_dev *pdev)
 void pcie_aspm_pm_state_change(struct pci_dev *pdev)
 {
 	struct pcie_link_state *link = pdev->link_state;
+	struct pci_dev *root = pcie_get_root(pdev);
 
 	if (aspm_disabled || !link)
 		return;
@@ -1049,7 +1050,7 @@ void pcie_aspm_pm_state_change(struct pci_dev *pdev)
 	 */
 	down_read(&pci_bus_sem);
 	mutex_lock(&aspm_lock);
-	pcie_update_aspm_capable(link->root);
+	pcie_update_aspm_capable(root->link_state);
 	pcie_config_aspm_path(link);
 	mutex_unlock(&aspm_lock);
 	up_read(&pci_bus_sem);
-- 
2.20.1

[RFC PATCH v2 3/4] PCI/ASPM: Remove struct pcie_link_state.downstream

[Saheed O. Bolarinwa]

From: "Bolarinwa O. Saheed" <refactormyself@gmail.com>

Information on the downstream component is cached in
struct pcie_link_state.downstream it obtained within
alloc_pcie_link_state() by calling pci_function_0()

This patch:
 - removes *downstream* from *struct pcie_link_state*.
 - replaces references to pcie_link_state.downstream with
   a call to pci_function_0(pdev->subordinate).

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

diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
index ad78aaeea444..56d4fe7d50b5 100644
--- a/drivers/pci/pcie/aspm.c
+++ b/drivers/pci/pcie/aspm.c
@@ -48,7 +48,6 @@ struct aspm_latency {
 
 struct pcie_link_state {
 	struct pci_dev *pdev;		/* Upstream component of the Link */
-	struct pci_dev *downstream;	/* Downstream component, function 0 */
 	struct list_head sibling;	/* node in link_list */
 
 	/* ASPM state */
@@ -460,7 +459,8 @@ static void pci_clear_and_set_dword(struct pci_dev *pdev, int pos,
 static void aspm_calc_l1ss_info(struct pcie_link_state *link,
 				u32 parent_l1ss_cap, u32 child_l1ss_cap)
 {
-	struct pci_dev *child = link->downstream, *parent = link->pdev;
+	struct pci_dev *parent = link->pdev;
+	struct pci_dev *child = pci_function_0(link->pdev->subordinate);
 	u32 val1, val2, scale1, scale2;
 	u32 t_common_mode, t_power_on, l1_2_threshold, scale, value;
 	u32 ctl1 = 0, ctl2 = 0;
@@ -550,7 +550,8 @@ static void aspm_calc_l1ss_info(struct pcie_link_state *link,
 
 static void pcie_aspm_cap_init(struct pcie_link_state *link, int blacklist)
 {
-	struct pci_dev *child = link->downstream, *parent = link->pdev;
+	struct pci_dev *parent = link->pdev;
+	struct pci_dev *child = pci_function_0(link->pdev->subordinate);
 	u32 parent_lnkcap, child_lnkcap;
 	u16 parent_lnkctl, child_lnkctl;
 	u32 parent_l1ss_cap, child_l1ss_cap;
@@ -692,7 +693,8 @@ static void pcie_aspm_cap_init(struct pcie_link_state *link, int blacklist)
 static void pcie_config_aspm_l1ss(struct pcie_link_state *link, u32 state)
 {
 	u32 val, enable_req;
-	struct pci_dev *child = link->downstream, *parent = link->pdev;
+	struct pci_dev *parent = link->pdev;
+	struct pci_dev *child = pci_function_0(link->pdev->subordinate);
 
 	enable_req = (link->aspm_enabled ^ state) & state;
 
@@ -751,7 +753,8 @@ static void pcie_config_aspm_dev(struct pci_dev *pdev, u32 val)
 static void pcie_config_aspm_link(struct pcie_link_state *link, u32 state)
 {
 	u32 upstream = 0, dwstream = 0;
-	struct pci_dev *child = link->downstream, *parent = link->pdev;
+	struct pci_dev *parent = link->pdev;
+	struct pci_dev *child = pci_function_0(link->pdev->subordinate);
 	struct pci_bus *linkbus = parent->subordinate;
 
 	/* Enable only the states that were not explicitly disabled */
@@ -879,7 +882,6 @@ static struct pcie_link_state *alloc_pcie_link_state(struct pci_dev *pdev)
 
 	INIT_LIST_HEAD(&link->sibling);
 	link->pdev = pdev;
-	link->downstream = pci_function_0(pdev->subordinate);
 
 	list_add(&link->sibling, &link_list);
 	pdev->link_state = link;
-- 
2.20.1

[RFC PATCH v2 4/4] PCI/ASPM: Remove unncessary linked list from aspm.c

[Saheed O. Bolarinwa]

From: "Bolarinwa O. Saheed" <refactormyself@gmail.com>

aspm.c defines a linked list - `link_list` and stores each of
its node in struct pcie_link_state.sibling. This linked list
tracks devices for which the struct pcie_link_state object
was successfully created. It is used to loop through the list
for instance to set ASPM policy or update changes. However, it
is possible to access these devices via existing lists defined
inside pci.h

This patch:
- removes link_list and struct pcie_link_state.sibling
- accesses child devices via struct pci_dev.bust_list
- accesses all PCI buses via pci_root_buses on struct pci_bus.node

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

diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
index 56d4fe7d50b5..4bef652dc63c 100644
--- a/drivers/pci/pcie/aspm.c
+++ b/drivers/pci/pcie/aspm.c
@@ -48,7 +48,6 @@ struct aspm_latency {
 
 struct pcie_link_state {
 	struct pci_dev *pdev;		/* Upstream component of the Link */
-	struct list_head sibling;	/* node in link_list */
 
 	/* ASPM state */
 	u32 aspm_support:7;		/* Supported ASPM state */
@@ -76,7 +75,6 @@ struct pcie_link_state {
 static int aspm_disabled, aspm_force;
 static bool aspm_support_enabled = true;
 static DEFINE_MUTEX(aspm_lock);
-static LIST_HEAD(link_list);
 
 #define POLICY_DEFAULT 0	/* BIOS default setting */
 #define POLICY_PERFORMANCE 1	/* high performance */
@@ -880,10 +878,7 @@ static struct pcie_link_state *alloc_pcie_link_state(struct pci_dev *pdev)
 	if (!link)
 		return NULL;
 
-	INIT_LIST_HEAD(&link->sibling);
 	link->pdev = pdev;
-
-	list_add(&link->sibling, &link_list);
 	pdev->link_state = link;
 	return link;
 }
@@ -970,24 +965,22 @@ static void pcie_update_aspm_capable(struct pcie_link_state *root)
 {
 	struct pcie_link_state *link;
 	struct pci_dev *dev, *root_dev;
+	struct pci_bus *rootbus = root->pdev->bus;
 
 	/* Ensure it is the root device */
 	root_dev = pcie_get_root(root->pdev);
 	root = root_dev ? root_dev->link_state : NULL;
 
-	list_for_each_entry(link, &link_list, sibling) {
-		dev = pcie_get_root(link->pdev);
-		if (dev->link_state != root)
+	list_for_each_entry(dev, &rootbus->devices, bus_list) {
+		if (!dev->link_state)
 			continue;
 
-		link->aspm_capable = link->aspm_support;
+		dev->link_state->aspm_capable = link->aspm_support;
 	}
-	list_for_each_entry(link, &link_list, sibling) {
+
+	list_for_each_entry(dev, &rootbus->devices, bus_list) {
 		struct pci_dev *child;
-		struct pci_bus *linkbus = link->pdev->subordinate;
-		dev = pcie_get_root(link->pdev);
-		if (dev->link_state != root)
-			continue;
+		struct pci_bus *linkbus = dev->subordinate;
 
 		list_for_each_entry(child, &linkbus->devices, bus_list) {
 			if ((pci_pcie_type(child) != PCI_EXP_TYPE_ENDPOINT) &&
@@ -1024,7 +1017,6 @@ void pcie_aspm_exit_link_state(struct pci_dev *pdev)
 
 	/* All functions are removed, so just disable ASPM for the link */
 	pcie_config_aspm_link(link, 0);
-	list_del(&link->sibling);
 	/* Clock PM is for endpoint device */
 	free_link_state(link);
 
@@ -1164,6 +1156,8 @@ static int pcie_aspm_set_policy(const char *val,
 {
 	int i;
 	struct pcie_link_state *link;
+	struct pci_bus *bus;
+	struct pci_dev *pdev;
 
 	if (aspm_disabled)
 		return -EPERM;
@@ -1176,9 +1170,18 @@ static int pcie_aspm_set_policy(const char *val,
 	down_read(&pci_bus_sem);
 	mutex_lock(&aspm_lock);
 	aspm_policy = i;
-	list_for_each_entry(link, &link_list, sibling) {
-		pcie_config_aspm_link(link, policy_to_aspm_state(link));
-		pcie_set_clkpm(link, policy_to_clkpm_state(link));
+	list_for_each_entry(bus, &pci_root_buses, node) {
+		list_for_each_entry(pdev, &bus->devices, bus_list) {
+			if (!pci_is_pcie(pdev))
+				break;
+
+			link = pdev->link_state;
+			if (!link)
+				continue;
+
+			pcie_config_aspm_link(link, policy_to_aspm_state(link));
+			pcie_set_clkpm(link, policy_to_clkpm_state(link));
+		}
 	}
 	mutex_unlock(&aspm_lock);
 	up_read(&pci_bus_sem);
-- 
2.20.1

Re: [RFC PATCH v2 1/4] PCI/ASPM: Remove struct pcie_link_state.parent

[Bjorn Helgaas]

On Wed, Sep 29, 2021 at 02:43:12AM +0200, Saheed O. Bolarinwa wrote:
> From: "Bolarinwa O. Saheed" <refactormyself@gmail.com>
> 
> Information cached in struct pcie_link_state.parent is accessible
> via struct pci_dev.
> 
> This patch:
>  - removes *parent* from the *struct pcie_link_state*
>  - creates pci_get_parent() which returns the parent of a pci_dev
>  - replaces references to pcie_link_state.parent with a call to
>    pci_get_parent()
>  - removes BUG_ON(root->parent), instead uses the parent's root
> 
> Signed-off-by: Bolarinwa O. Saheed <refactormyself@gmail.com>
> ---
>  drivers/pci/pcie/aspm.c | 36 ++++++++++++++++++++++++++----------
>  1 file changed, 26 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
> index 013a47f587ce..414c04ffe962 100644
> --- a/drivers/pci/pcie/aspm.c
> +++ b/drivers/pci/pcie/aspm.c
> @@ -50,7 +50,6 @@ struct pcie_link_state {
>  	struct pci_dev *pdev;		/* Upstream component of the Link */
>  	struct pci_dev *downstream;	/* Downstream component, function 0 */
>  	struct pcie_link_state *root;	/* pointer to the root port link */
> -	struct pcie_link_state *parent;	/* pointer to the parent Link state */
>  	struct list_head sibling;	/* node in link_list */
>  
>  	/* ASPM state */
> @@ -139,6 +138,14 @@ static int policy_to_clkpm_state(struct pcie_link_state *link)
>  	return 0;
>  }
>  
> +static struct pci_dev *pci_get_parent(struct pci_dev *pdev)
> +{
> +	if (!pdev || !pdev->bus->parent || !pdev->bus->parent->self)
> +		return NULL;
> +
> +	return pdev->bus->parent->self;
> +}

I LOVE the idea of getting rid of the pcie_link_state.parent pointer.
I think it's dumb to maintain a shadow hierarchy when we already HAVE
a hierarchy in struct pci_dev.

I'm not in love with the pci_get_parent() name because a pci_dev
doesn't really have a "parent."  The closest thing to a parent would
be the bridge upstream from the device, and that's not what this
returns.

This actually has to start from a Downstream Port (not an Endpoint)
because the struct pcie_link_state is always associated with the
upstream end of the link.

And it actually returns the bridge that is *two* levels up, because
that's the upstream end of the next link, so it's more like the
"grandparent" of pdev, not the "parent."

Example from my laptop:

  0a:04.0 Downstream Port (switch A)    to [bus 0c-3d]
  0c:00.0 Upstream Port (switch B)      to [bus 0d-3d]
  0d:01.0 Downstream Port (switch B)    to [bus 0e]
  0e:00.0 Upstream Port (Endpoint)      USB controller

Here there are two links:

  0a:04.0 --- 0c:00.0
  0d:01.0 --- 0e:00.0

and the pcie_link_states are associated with 0a:04.0 and 0d:01.0.

If we start from 0d:01.0, which is the upstream end of the last link:

  "pdev" is a pci_dev of a downstream port, e.g., 0d:01.0.
  "pdev->bus" is the pci_bus pdev is on: [bus 0d].
  "pdev->bus->self" is the bridge leading to "bus": 0c:00.0.
  "pdev->bus->parent" is the parent pci_bus of [bus 0d]: [bus 0c].
  "pdev->bus->parent->self" is the bridge leading to [bus 0c]: 0a:04.0.

Sorry for the rambling, just trying to get this all clear in my head.

Almost all the calls of pci_get_parent() look like this:

  parent = pci_get_parent(link->pdev);
  link = parent ? parent->link_state : NULL;

What if you made something like this:

  struct pcie_link_state *pcie_upstream_link(struct pcie_link_state *link)
  {
    struct pci_dev *bridge;

    bridge = pci_upstream_bridge(link->pdev);
    if (!bridge)
      return NULL;

    bridge = pci_upstream_bridge(bridge);
    return bridge ? bridge->link_state : NULL;
  }

>  static void pcie_set_clkpm_nocheck(struct pcie_link_state *link, int enable)
>  {
>  	struct pci_dev *child;
> @@ -379,6 +386,7 @@ static void encode_l12_threshold(u32 threshold_us, u32 *scale, u32 *value)
>  static void pcie_aspm_check_latency(struct pci_dev *endpoint)
>  {
>  	u32 latency, l1_switch_latency = 0;
> +	struct pci_dev *parent;
>  	struct aspm_latency *acceptable;
>  	struct pcie_link_state *link;
>  
> @@ -419,7 +427,8 @@ static void pcie_aspm_check_latency(struct pci_dev *endpoint)
>  			link->aspm_capable &= ~ASPM_STATE_L1;
>  		l1_switch_latency += 1000;
>  
> -		link = link->parent;
> +		parent = pci_get_parent(link->pdev);
> +		link = parent ? parent->link_state : NULL;
>  	}
>  }
>  
> @@ -793,9 +802,11 @@ static void pcie_config_aspm_link(struct pcie_link_state *link, u32 state)
>  
>  static void pcie_config_aspm_path(struct pcie_link_state *link)
>  {
> +	struct pci_dev *parent;

Missing a blank line here.

>  	while (link) {
>  		pcie_config_aspm_link(link, policy_to_aspm_state(link));
> -		link = link->parent;
> +		parent = pci_get_parent(link->pdev);
> +		link = parent ? parent->link_state : NULL;
>  	}
>  }
>  
> @@ -864,16 +875,15 @@ static struct pcie_link_state *alloc_pcie_link_state(struct pci_dev *pdev)
>  	    !pdev->bus->parent->self) {
>  		link->root = link;
>  	} else {
> -		struct pcie_link_state *parent;
> +		struct pci_dev *parent;
>  
> -		parent = pdev->bus->parent->self->link_state;
> -		if (!parent) {
> +		parent = pci_get_parent(pdev);
> +		if (!parent->link_state) {
>  			kfree(link);
>  			return NULL;
>  		}
>  
> -		link->parent = parent;
> -		link->root = link->parent->root;
> +		link->root = parent->link_state->root;
>  	}
>  
>  	list_add(&link->sibling, &link_list);
> @@ -962,7 +972,11 @@ void pcie_aspm_init_link_state(struct pci_dev *pdev)
>  static void pcie_update_aspm_capable(struct pcie_link_state *root)
>  {
>  	struct pcie_link_state *link;
> -	BUG_ON(root->parent);
> +	struct pci_dev *parent = pci_get_parent(root->pdev);
> +
> +	if (parent && parent->link_state)
> +		root = parent->link_state->root;
> +
>  	list_for_each_entry(link, &link_list, sibling) {
>  		if (link->root != root)
>  			continue;
> @@ -985,6 +999,7 @@ static void pcie_update_aspm_capable(struct pcie_link_state *root)
>  /* @pdev: the endpoint device */
>  void pcie_aspm_exit_link_state(struct pci_dev *pdev)
>  {
> +	struct pci_dev *parent_dev;
>  	struct pci_dev *parent = pdev->bus->self;
>  	struct pcie_link_state *link, *root, *parent_link;
>  
> @@ -1002,7 +1017,8 @@ void pcie_aspm_exit_link_state(struct pci_dev *pdev)
>  
>  	link = parent->link_state;
>  	root = link->root;
> -	parent_link = link->parent;
> +	parent_dev = pci_get_parent(link->pdev);
> +	parent_link = parent_dev ? parent_dev->link_state : NULL;
>  
>  	/* All functions are removed, so just disable ASPM for the link */
>  	pcie_config_aspm_link(link, 0);
> -- 
> 2.20.1
> 

Re: [RFC PATCH v2 4/4] PCI/ASPM: Remove unncessary linked list from aspm.c

[Bjorn Helgaas]

On Wed, Sep 29, 2021 at 02:43:15AM +0200, Saheed O. Bolarinwa wrote:
> From: "Bolarinwa O. Saheed" <refactormyself@gmail.com>
> 
> aspm.c defines a linked list - `link_list` and stores each of
> its node in struct pcie_link_state.sibling. This linked list
> tracks devices for which the struct pcie_link_state object
> was successfully created. It is used to loop through the list
> for instance to set ASPM policy or update changes. However, it
> is possible to access these devices via existing lists defined
> inside pci.h
> 
> This patch:
> - removes link_list and struct pcie_link_state.sibling
> - accesses child devices via struct pci_dev.bust_list
> - accesses all PCI buses via pci_root_buses on struct pci_bus.node

Again, I LOVE the way this is going.  I depise this extra linked list.

> Signed-off-by: Bolarinwa O. Saheed <refactormyself@gmail.com>
> ---
>  drivers/pci/pcie/aspm.c | 39 +++++++++++++++++++++------------------
>  1 file changed, 21 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
> index 56d4fe7d50b5..4bef652dc63c 100644
> --- a/drivers/pci/pcie/aspm.c
> +++ b/drivers/pci/pcie/aspm.c
> @@ -48,7 +48,6 @@ struct aspm_latency {
>  
>  struct pcie_link_state {
>  	struct pci_dev *pdev;		/* Upstream component of the Link */
> -	struct list_head sibling;	/* node in link_list */
>  
>  	/* ASPM state */
>  	u32 aspm_support:7;		/* Supported ASPM state */
> @@ -76,7 +75,6 @@ struct pcie_link_state {
>  static int aspm_disabled, aspm_force;
>  static bool aspm_support_enabled = true;
>  static DEFINE_MUTEX(aspm_lock);
> -static LIST_HEAD(link_list);
>  
>  #define POLICY_DEFAULT 0	/* BIOS default setting */
>  #define POLICY_PERFORMANCE 1	/* high performance */
> @@ -880,10 +878,7 @@ static struct pcie_link_state *alloc_pcie_link_state(struct pci_dev *pdev)
>  	if (!link)
>  		return NULL;
>  
> -	INIT_LIST_HEAD(&link->sibling);
>  	link->pdev = pdev;
> -
> -	list_add(&link->sibling, &link_list);
>  	pdev->link_state = link;
>  	return link;
>  }
> @@ -970,24 +965,22 @@ static void pcie_update_aspm_capable(struct pcie_link_state *root)
>  {
>  	struct pcie_link_state *link;
>  	struct pci_dev *dev, *root_dev;
> +	struct pci_bus *rootbus = root->pdev->bus;
>  
>  	/* Ensure it is the root device */
>  	root_dev = pcie_get_root(root->pdev);
>  	root = root_dev ? root_dev->link_state : NULL;
>  
> -	list_for_each_entry(link, &link_list, sibling) {
> -		dev = pcie_get_root(link->pdev);
> -		if (dev->link_state != root)
> +	list_for_each_entry(dev, &rootbus->devices, bus_list) {
> +		if (!dev->link_state)
>  			continue;
>  
> -		link->aspm_capable = link->aspm_support;
> +		dev->link_state->aspm_capable = link->aspm_support;
>  	}
> -	list_for_each_entry(link, &link_list, sibling) {
> +
> +	list_for_each_entry(dev, &rootbus->devices, bus_list) {
>  		struct pci_dev *child;
> -		struct pci_bus *linkbus = link->pdev->subordinate;
> -		dev = pcie_get_root(link->pdev);
> -		if (dev->link_state != root)
> -			continue;
> +		struct pci_bus *linkbus = dev->subordinate;
>  
>  		list_for_each_entry(child, &linkbus->devices, bus_list) {
>  			if ((pci_pcie_type(child) != PCI_EXP_TYPE_ENDPOINT) &&
> @@ -1024,7 +1017,6 @@ void pcie_aspm_exit_link_state(struct pci_dev *pdev)
>  
>  	/* All functions are removed, so just disable ASPM for the link */
>  	pcie_config_aspm_link(link, 0);
> -	list_del(&link->sibling);
>  	/* Clock PM is for endpoint device */
>  	free_link_state(link);
>  
> @@ -1164,6 +1156,8 @@ static int pcie_aspm_set_policy(const char *val,
>  {
>  	int i;
>  	struct pcie_link_state *link;
> +	struct pci_bus *bus;
> +	struct pci_dev *pdev;
>  
>  	if (aspm_disabled)
>  		return -EPERM;
> @@ -1176,9 +1170,18 @@ static int pcie_aspm_set_policy(const char *val,
>  	down_read(&pci_bus_sem);
>  	mutex_lock(&aspm_lock);
>  	aspm_policy = i;
> -	list_for_each_entry(link, &link_list, sibling) {
> -		pcie_config_aspm_link(link, policy_to_aspm_state(link));
> -		pcie_set_clkpm(link, policy_to_clkpm_state(link));
> +	list_for_each_entry(bus, &pci_root_buses, node) {
> +		list_for_each_entry(pdev, &bus->devices, bus_list) {
> +			if (!pci_is_pcie(pdev))
> +				break;
> +
> +			link = pdev->link_state;
> +			if (!link)
> +				continue;
> +
> +			pcie_config_aspm_link(link, policy_to_aspm_state(link));
> +			pcie_set_clkpm(link, policy_to_clkpm_state(link));
> +		}

IIUC, in the existing code, link_list contains *every* pcie_link_state
in the system, so we update the configuration of all of them.

Here, iterating through pci_root_buses gives us all the root buses (in
the case of multiple host bridges), and on each root bus we look at
every device that has a link_state, so those would typically be Root
Ports.  But I don't think we descend the hierarchy, so in the case of
deeper hierarchies, I don't think we update the lower levels.

Example from my laptop:

  00:1d.6 Root Port                     to [bus 06-3e]
  06:00.0 Upstream Port   (switch A)    to [bus 07-3e]
  07:01.0 Downstream Port (switch A)    to [bus 09-3d]
  09:00.0 Upstream Port   (switch B)    to [bus 0a-3d]
  0a:04.0 Downstream Port (switch B)    to [bus 0c-3d]
  0c:00.0 Upstream Port   (switch C)    to [bus 0d-3d]
  0d:01.0 Downstream Port (switch C)    to [bus 0e]
  0e:00.0 Upstream Port   (Endpoint)    USB controller

Here there are four links:

  00:1d.6 --- 06:00.0
  07:01.0 --- 09:00.0
  0a:04.0 --- 0c:00.0
  0d:01.0 --- 0e:00.0

But I think this patch only looks at the 00:1d.6 --- 06:00.0 link,
doesn't it?

>  	}
>  	mutex_unlock(&aspm_lock);
>  	up_read(&pci_bus_sem);
> -- 
> 2.20.1
> 

Re: [RFC PATCH v2 1/4] PCI/ASPM: Remove struct pcie_link_state.parent

[Saheed Bolarinwa]

On Fri, Oct 1, 2021 at 12:40 AM Bjorn Helgaas <helgaas@kernel.org> wrote:
>
> On Wed, Sep 29, 2021 at 02:43:12AM +0200, Saheed O. Bolarinwa wrote:
> > From: "Bolarinwa O. Saheed" <refactormyself@gmail.com>
> >
> > Information cached in struct pcie_link_state.parent is accessible
> > via struct pci_dev.
> >
> > This patch:
> >  - removes *parent* from the *struct pcie_link_state*
> >  - creates pci_get_parent() which returns the parent of a pci_dev
> >  - replaces references to pcie_link_state.parent with a call to
> >    pci_get_parent()
> >  - removes BUG_ON(root->parent), instead uses the parent's root
> >
> > Signed-off-by: Bolarinwa O. Saheed <refactormyself@gmail.com>
> > ---
> >  drivers/pci/pcie/aspm.c | 36 ++++++++++++++++++++++++++----------
> >  1 file changed, 26 insertions(+), 10 deletions(-)
> >
> > diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
> > index 013a47f587ce..414c04ffe962 100644
> > --- a/drivers/pci/pcie/aspm.c
> > +++ b/drivers/pci/pcie/aspm.c
> > @@ -50,7 +50,6 @@ struct pcie_link_state {
> >       struct pci_dev *pdev;           /* Upstream component of the Link */
> >       struct pci_dev *downstream;     /* Downstream component, function 0 */
> >       struct pcie_link_state *root;   /* pointer to the root port link */
> > -     struct pcie_link_state *parent; /* pointer to the parent Link state */
> >       struct list_head sibling;       /* node in link_list */
> >
> >       /* ASPM state */
> > @@ -139,6 +138,14 @@ static int policy_to_clkpm_state(struct pcie_link_state *link)
> >       return 0;
> >  }
> >
> > +static struct pci_dev *pci_get_parent(struct pci_dev *pdev)
> > +{
> > +     if (!pdev || !pdev->bus->parent || !pdev->bus->parent->self)
> > +             return NULL;
> > +
> > +     return pdev->bus->parent->self;
> > +}
>
> I LOVE the idea of getting rid of the pcie_link_state.parent pointer.
> I think it's dumb to maintain a shadow hierarchy when we already HAVE
> a hierarchy in struct pci_dev.
>
> I'm not in love with the pci_get_parent() name because a pci_dev
> doesn't really have a "parent."  The closest thing to a parent would
> be the bridge upstream from the device, and that's not what this
> returns.
>
> This actually has to start from a Downstream Port (not an Endpoint)
> because the struct pcie_link_state is always associated with the
> upstream end of the link.
>
> And it actually returns the bridge that is *two* levels up, because
> that's the upstream end of the next link, so it's more like the
> "grandparent" of pdev, not the "parent."
>
> Example from my laptop:
>
>   0a:04.0 Downstream Port (switch A)    to [bus 0c-3d]
>   0c:00.0 Upstream Port (switch B)      to [bus 0d-3d]
>   0d:01.0 Downstream Port (switch B)    to [bus 0e]
>   0e:00.0 Upstream Port (Endpoint)      USB controller
>
> Here there are two links:
>
>   0a:04.0 --- 0c:00.0
>   0d:01.0 --- 0e:00.0
>
> and the pcie_link_states are associated with 0a:04.0 and 0d:01.0.
>
> If we start from 0d:01.0, which is the upstream end of the last link:
>
>   "pdev" is a pci_dev of a downstream port, e.g., 0d:01.0.
>   "pdev->bus" is the pci_bus pdev is on: [bus 0d].
>   "pdev->bus->self" is the bridge leading to "bus": 0c:00.0.
>   "pdev->bus->parent" is the parent pci_bus of [bus 0d]: [bus 0c].
>   "pdev->bus->parent->self" is the bridge leading to [bus 0c]: 0a:04.0.
>
> Sorry for the rambling, just trying to get this all clear in my head.
Thanks, it is very helpful for me.

>
> Almost all the calls of pci_get_parent() look like this:
>
>   parent = pci_get_parent(link->pdev);
>   link = parent ? parent->link_state : NULL;
>
> What if you made something like this:
>
>   struct pcie_link_state *pcie_upstream_link(struct pcie_link_state *link)
This is better, especially using pci_upstream_bridge() directly.
By returning struct pci_dev, I was trying to avoid "struct pcie_link_state",
so as to ease the journey towards eliminating it.
Also, will it be helpful to handle NULL values within pci_upstream_bridge()?



>   {
>     struct pci_dev *bridge;
>
>     bridge = pci_upstream_bridge(link->pdev);
>     if (!bridge)
>       return NULL;
>
>     bridge = pci_upstream_bridge(bridge);
>     return bridge ? bridge->link_state : NULL;
>   }
>
> >  static void pcie_set_clkpm_nocheck(struct pcie_link_state *link, int enable)
> >  {
> >       struct pci_dev *child;
> > @@ -379,6 +386,7 @@ static void encode_l12_threshold(u32 threshold_us, u32 *scale, u32 *value)
> >  static void pcie_aspm_check_latency(struct pci_dev *endpoint)
> >  {
> >       u32 latency, l1_switch_latency = 0;
> > +     struct pci_dev *parent;
> >       struct aspm_latency *acceptable;
> >       struct pcie_link_state *link;
> >
> > @@ -419,7 +427,8 @@ static void pcie_aspm_check_latency(struct pci_dev *endpoint)
> >                       link->aspm_capable &= ~ASPM_STATE_L1;
> >               l1_switch_latency += 1000;
> >
> > -             link = link->parent;
> > +             parent = pci_get_parent(link->pdev);
> > +             link = parent ? parent->link_state : NULL;
> >       }
> >  }
> >
> > @@ -793,9 +802,11 @@ static void pcie_config_aspm_link(struct pcie_link_state *link, u32 state)
> >
> >  static void pcie_config_aspm_path(struct pcie_link_state *link)
> >  {
> > +     struct pci_dev *parent;
>
> Missing a blank line here.
>
> >       while (link) {
> >               pcie_config_aspm_link(link, policy_to_aspm_state(link));
> > -             link = link->parent;
> > +             parent = pci_get_parent(link->pdev);
> > +             link = parent ? parent->link_state : NULL;
> >       }
> >  }
> >
> > @@ -864,16 +875,15 @@ static struct pcie_link_state *alloc_pcie_link_state(struct pci_dev *pdev)
> >           !pdev->bus->parent->self) {
> >               link->root = link;
> >       } else {
> > -             struct pcie_link_state *parent;
> > +             struct pci_dev *parent;
> >
> > -             parent = pdev->bus->parent->self->link_state;
> > -             if (!parent) {
> > +             parent = pci_get_parent(pdev);
> > +             if (!parent->link_state) {
> >                       kfree(link);
> >                       return NULL;
> >               }
> >
> > -             link->parent = parent;
> > -             link->root = link->parent->root;
> > +             link->root = parent->link_state->root;
> >       }
> >
> >       list_add(&link->sibling, &link_list);
> > @@ -962,7 +972,11 @@ void pcie_aspm_init_link_state(struct pci_dev *pdev)
> >  static void pcie_update_aspm_capable(struct pcie_link_state *root)
> >  {
> >       struct pcie_link_state *link;
> > -     BUG_ON(root->parent);
> > +     struct pci_dev *parent = pci_get_parent(root->pdev);
> > +
> > +     if (parent && parent->link_state)
> > +             root = parent->link_state->root;
> > +
> >       list_for_each_entry(link, &link_list, sibling) {
> >               if (link->root != root)
> >                       continue;
> > @@ -985,6 +999,7 @@ static void pcie_update_aspm_capable(struct pcie_link_state *root)
> >  /* @pdev: the endpoint device */
> >  void pcie_aspm_exit_link_state(struct pci_dev *pdev)
> >  {
> > +     struct pci_dev *parent_dev;
> >       struct pci_dev *parent = pdev->bus->self;
> >       struct pcie_link_state *link, *root, *parent_link;
> >
> > @@ -1002,7 +1017,8 @@ void pcie_aspm_exit_link_state(struct pci_dev *pdev)
> >
> >       link = parent->link_state;
> >       root = link->root;
> > -     parent_link = link->parent;
> > +     parent_dev = pci_get_parent(link->pdev);
> > +     parent_link = parent_dev ? parent_dev->link_state : NULL;
> >
> >       /* All functions are removed, so just disable ASPM for the link */
> >       pcie_config_aspm_link(link, 0);
> > --
> > 2.20.1
> >

Re: [RFC PATCH v2 4/4] PCI/ASPM: Remove unncessary linked list from aspm.c

[Saheed Bolarinwa]

On Fri, Oct 1, 2021 at 1:00 AM Bjorn Helgaas <helgaas@kernel.org> wrote:
>
> On Wed, Sep 29, 2021 at 02:43:15AM +0200, Saheed O. Bolarinwa wrote:
> > From: "Bolarinwa O. Saheed" <refactormyself@gmail.com>
> >
> > aspm.c defines a linked list - `link_list` and stores each of
> > its node in struct pcie_link_state.sibling. This linked list
> > tracks devices for which the struct pcie_link_state object
> > was successfully created. It is used to loop through the list
> > for instance to set ASPM policy or update changes. However, it
> > is possible to access these devices via existing lists defined
> > inside pci.h
> >
> > This patch:
> > - removes link_list and struct pcie_link_state.sibling
> > - accesses child devices via struct pci_dev.bust_list
> > - accesses all PCI buses via pci_root_buses on struct pci_bus.node
>
> Again, I LOVE the way this is going.  I depise this extra linked list.
>
> > Signed-off-by: Bolarinwa O. Saheed <refactormyself@gmail.com>
> > ---
> >  drivers/pci/pcie/aspm.c | 39 +++++++++++++++++++++------------------
> >  1 file changed, 21 insertions(+), 18 deletions(-)
> >
> > diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
> > index 56d4fe7d50b5..4bef652dc63c 100644
> > --- a/drivers/pci/pcie/aspm.c
> > +++ b/drivers/pci/pcie/aspm.c
> > @@ -48,7 +48,6 @@ struct aspm_latency {
> >
> >  struct pcie_link_state {
> >       struct pci_dev *pdev;           /* Upstream component of the Link */
> > -     struct list_head sibling;       /* node in link_list */
> >
> >       /* ASPM state */
> >       u32 aspm_support:7;             /* Supported ASPM state */
> > @@ -76,7 +75,6 @@ struct pcie_link_state {
> >  static int aspm_disabled, aspm_force;
> >  static bool aspm_support_enabled = true;
> >  static DEFINE_MUTEX(aspm_lock);
> > -static LIST_HEAD(link_list);
> >
> >  #define POLICY_DEFAULT 0     /* BIOS default setting */
> >  #define POLICY_PERFORMANCE 1 /* high performance */
> > @@ -880,10 +878,7 @@ static struct pcie_link_state *alloc_pcie_link_state(struct pci_dev *pdev)
> >       if (!link)
> >               return NULL;
> >
> > -     INIT_LIST_HEAD(&link->sibling);
> >       link->pdev = pdev;
> > -
> > -     list_add(&link->sibling, &link_list);
> >       pdev->link_state = link;
> >       return link;
> >  }
> > @@ -970,24 +965,22 @@ static void pcie_update_aspm_capable(struct pcie_link_state *root)
> >  {
> >       struct pcie_link_state *link;
> >       struct pci_dev *dev, *root_dev;
> > +     struct pci_bus *rootbus = root->pdev->bus;
> >
> >       /* Ensure it is the root device */
> >       root_dev = pcie_get_root(root->pdev);
> >       root = root_dev ? root_dev->link_state : NULL;
> >
> > -     list_for_each_entry(link, &link_list, sibling) {
> > -             dev = pcie_get_root(link->pdev);
> > -             if (dev->link_state != root)
> > +     list_for_each_entry(dev, &rootbus->devices, bus_list) {
> > +             if (!dev->link_state)
> >                       continue;
> >
> > -             link->aspm_capable = link->aspm_support;
> > +             dev->link_state->aspm_capable = link->aspm_support;
> >       }
> > -     list_for_each_entry(link, &link_list, sibling) {
> > +
> > +     list_for_each_entry(dev, &rootbus->devices, bus_list) {
> >               struct pci_dev *child;
> > -             struct pci_bus *linkbus = link->pdev->subordinate;
> > -             dev = pcie_get_root(link->pdev);
> > -             if (dev->link_state != root)
> > -                     continue;
> > +             struct pci_bus *linkbus = dev->subordinate;
> >
> >               list_for_each_entry(child, &linkbus->devices, bus_list) {
> >                       if ((pci_pcie_type(child) != PCI_EXP_TYPE_ENDPOINT) &&
> > @@ -1024,7 +1017,6 @@ void pcie_aspm_exit_link_state(struct pci_dev *pdev)
> >
> >       /* All functions are removed, so just disable ASPM for the link */
> >       pcie_config_aspm_link(link, 0);
> > -     list_del(&link->sibling);
> >       /* Clock PM is for endpoint device */
> >       free_link_state(link);
> >
> > @@ -1164,6 +1156,8 @@ static int pcie_aspm_set_policy(const char *val,
> >  {
> >       int i;
> >       struct pcie_link_state *link;
> > +     struct pci_bus *bus;
> > +     struct pci_dev *pdev;
> >
> >       if (aspm_disabled)
> >               return -EPERM;
> > @@ -1176,9 +1170,18 @@ static int pcie_aspm_set_policy(const char *val,
> >       down_read(&pci_bus_sem);
> >       mutex_lock(&aspm_lock);
> >       aspm_policy = i;
> > -     list_for_each_entry(link, &link_list, sibling) {
> > -             pcie_config_aspm_link(link, policy_to_aspm_state(link));
> > -             pcie_set_clkpm(link, policy_to_clkpm_state(link));
> > +     list_for_each_entry(bus, &pci_root_buses, node) {
> > +             list_for_each_entry(pdev, &bus->devices, bus_list) {
> > +                     if (!pci_is_pcie(pdev))
> > +                             break;
> > +
> > +                     link = pdev->link_state;
> > +                     if (!link)
> > +                             continue;
> > +
> > +                     pcie_config_aspm_link(link, policy_to_aspm_state(link));
> > +                     pcie_set_clkpm(link, policy_to_clkpm_state(link));
> > +             }
>
> IIUC, in the existing code, link_list contains *every* pcie_link_state
> in the system, so we update the configuration of all of them.
>
> Here, iterating through pci_root_buses gives us all the root buses (in
> the case of multiple host bridges), and on each root bus we look at
> every device that has a link_state, so those would typically be Root
> Ports.  But I don't think we descend the hierarchy, so in the case of
> deeper hierarchies, I don't think we update the lower levels.
>
> Example from my laptop:
>
>   00:1d.6 Root Port                     to [bus 06-3e]
>   06:00.0 Upstream Port   (switch A)    to [bus 07-3e]
>   07:01.0 Downstream Port (switch A)    to [bus 09-3d]
>   09:00.0 Upstream Port   (switch B)    to [bus 0a-3d]
>   0a:04.0 Downstream Port (switch B)    to [bus 0c-3d]
>   0c:00.0 Upstream Port   (switch C)    to [bus 0d-3d]
>   0d:01.0 Downstream Port (switch C)    to [bus 0e]
>   0e:00.0 Upstream Port   (Endpoint)    USB controller
>
> Here there are four links:
>
>   00:1d.6 --- 06:00.0
>   07:01.0 --- 09:00.0
>   0a:04.0 --- 0c:00.0
>   0d:01.0 --- 0e:00.0
>
> But I think this patch only looks at the 00:1d.6 --- 06:00.0 link,
> doesn't it?
Thank you, that is correct.
I should have used pdev->subordinate to traverse the hierarchy via the bridges.
>
> >       }
> >       mutex_unlock(&aspm_lock);
> >       up_read(&pci_bus_sem);
> > --
> > 2.20.1
> >