| Message ID | 20210916085206.2268-2-refactormyself@gmail.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
| Series | PCI/ASPM: Remove unncessary linked list in aspm.c | expand |
On Thu, Sep 16, 2021 at 10:52:03AM +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* > - adjusts all references to it to access the information directly > > Signed-off-by: Bolarinwa O. Saheed <refactormyself@gmail.com> > --- > OPINION: the checkpatch.pl scring warns on this line: > `BUG_ON(root->pdev->bus->parent->self);` > however, I think if a root device reports a parent, that is serious! Do you mean this warning? WARNING: Missing a blank line after declarations #967: FILE: drivers/pci/pcie/aspm.c:967: + struct pcie_link_state *link; + BUG_ON(root->pdev->bus->parent->self); That's just complaining about a blank line, so no big deal. You could resolve that by adding the blank line in this patch. The fact that we use BUG_ON() at all *is* a real problem. See the comments at the BUG() definition. We should rework this so that condition is either impossible and we can just remove the BUG_ON(), or we can deal with it gracefully. But this would be material for a different patch. > drivers/pci/pcie/aspm.c | 18 +++++++++++------- > 1 file changed, 11 insertions(+), 7 deletions(-) > > diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c > index 013a47f587ce..48b83048aa30 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 */ > @@ -379,6 +378,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 +419,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 = link->pdev->bus->parent->self; > + link = !parent ? NULL : parent->link_state; I love the direction of this patch, but this chain of pointers (link->pdev->bus->parent->self) is a little over the top and is repeated several times here. Can we simplify it a bit by making a helper function? It's similar but not quite the same as pci_upstream_bridge(). And maybe reverse the condition to avoid the negation? link = parent ? parent->link_state : NULL; > } > } > > @@ -793,9 +794,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 = link->pdev->bus->parent->self; > + link = !parent ? NULL : parent->link_state; > } > } > > @@ -872,8 +875,7 @@ static struct pcie_link_state *alloc_pcie_link_state(struct pci_dev *pdev) > return NULL; > } > > - link->parent = parent; > - link->root = link->parent->root; > + link->root = parent->root; > } > > list_add(&link->sibling, &link_list); > @@ -962,7 +964,7 @@ 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); > + BUG_ON(root->pdev->bus->parent->self); > list_for_each_entry(link, &link_list, sibling) { > if (link->root != root) > continue; > @@ -985,6 +987,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 +1005,8 @@ void pcie_aspm_exit_link_state(struct pci_dev *pdev) > > link = parent->link_state; > root = link->root; > - parent_link = link->parent; > + parent_dev = link->pdev->bus->parent->self; > + parent_link = !parent_dev ? NULL : parent_dev->link_state; > > /* 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..48b83048aa30 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 */ @@ -379,6 +378,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 +419,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 = link->pdev->bus->parent->self; + link = !parent ? NULL : parent->link_state; } } @@ -793,9 +794,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 = link->pdev->bus->parent->self; + link = !parent ? NULL : parent->link_state; } } @@ -872,8 +875,7 @@ static struct pcie_link_state *alloc_pcie_link_state(struct pci_dev *pdev) return NULL; } - link->parent = parent; - link->root = link->parent->root; + link->root = parent->root; } list_add(&link->sibling, &link_list); @@ -962,7 +964,7 @@ 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); + BUG_ON(root->pdev->bus->parent->self); list_for_each_entry(link, &link_list, sibling) { if (link->root != root) continue; @@ -985,6 +987,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 +1005,8 @@ void pcie_aspm_exit_link_state(struct pci_dev *pdev) link = parent->link_state; root = link->root; - parent_link = link->parent; + parent_dev = link->pdev->bus->parent->self; + parent_link = !parent_dev ? NULL : parent_dev->link_state; /* All functions are removed, so just disable ASPM for the link */ pcie_config_aspm_link(link, 0);