On Wed, Sep 29, 2021 at 02:43:12AM +0200, Saheed O. Bolarinwa wrote:
> From: "Bolarinwa O. Saheed" <refactormyself@xxxxxxxxx>
>
> 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@xxxxxxxxx>
> ---
> 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
>
