Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Thu, 30 Sep 2021 18:00:47 -0500
From:   Bjorn Helgaas <helgaas@kernel.org>
To:     "Saheed O. Bolarinwa" <refactormyself@gmail.com>
Cc:     linux-pci@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [RFC PATCH v2 4/4] PCI/ASPM: Remove unncessary linked list from
 aspm.c
Message-ID: <20210930230047.GA921465@bhelgaas>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20210929004315.22558-5-refactormyself@gmail.com>
Precedence: bulk

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
>