| Message ID | 20210929004315.22558-2-refactormyself@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
| Series | Remove unncessary linked list from aspm.c | expand |
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 >
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 > >
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);