| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Mon, 11 Oct 2021 08:03:08 +0200
From: Saheed Bolarinwa <refactormyself@...il.com>
To: Bjorn Helgaas <helgaas@...nel.org>
Cc: linux-pci@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [RFC PATCH v2 4/4] PCI/ASPM: Remove unncessary linked list from aspm.c
On Fri, Oct 1, 2021 at 1:00 AM Bjorn Helgaas <helgaas@...nel.org> wrote:
>
> On Wed, Sep 29, 2021 at 02:43:15AM +0200, Saheed O. Bolarinwa wrote:
> > From: "Bolarinwa O. Saheed" <refactormyself@...il.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@...il.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
> >
Powered by blists - more mailing lists