Hi Sinan,
On Sat, Apr 08, 2017 at 12:55:49AM -0400, Sinan Kaya wrote:
> For bridges, have pcie_aspm_init_link_state() allocate a link_state,
> regardless of whether it currently has any children.
>
> pcie_aspm_init_link_state(): Called for bridges (upstream end of
> link) after all children have been enumerated. No longer needs to
> check aspm_support_enabled or pdev->has_secondary_link or the VIA
> quirk: pci_aspm_init() already checked that stuff, so we only need
> to check pdev->link_state here.
>
> Now that we are calling alloc_pcie_link_state() from pci_aspm_init()
> , get rid of pci_function_0 function and detect downstream link in
> pci_aspm_init_upstream() instead when the function number is 0.
>
> Fixes: https://bugzilla.kernel.org/show_bug.cgi?id=194895
> Signed-off-by: Sinan Kaya <ok...@codeaurora.org>
> ---
> drivers/pci/pcie/aspm.c | 72
> ++++++++++++++++++++++++-------------------------
> 1 file changed, 36 insertions(+), 36 deletions(-)
>
> diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
> index a80d64b..e33f84b 100644
> --- a/drivers/pci/pcie/aspm.c
> +++ b/drivers/pci/pcie/aspm.c
> @@ -422,20 +422,6 @@ static void pcie_aspm_check_latency(struct pci_dev
> *endpoint)
> }
> }
>
> -/*
> - * The L1 PM substate capability is only implemented in function 0 in a
> - * multi function device.
> - */
> -static struct pci_dev *pci_function_0(struct pci_bus *linkbus)
> -{
> - struct pci_dev *child;
> -
> - list_for_each_entry(child, &linkbus->devices, bus_list)
> - if (PCI_FUNC(child->devfn) == 0)
> - return child;
> - return NULL;
> -}
> -
> /* Calculate L1.2 PM substate timing parameters */
> static void aspm_calc_l1ss_info(struct pcie_link_state *link,
> struct aspm_register_info *upreg,
> @@ -798,7 +784,6 @@ static struct pcie_link_state
> *alloc_pcie_link_state(struct pci_dev *pdev)
> INIT_LIST_HEAD(&link->children);
> INIT_LIST_HEAD(&link->link);
> link->pdev = pdev;
> - link->downstream = pci_function_0(pdev->subordinate);
>
> /*
> * Root Ports and PCI/PCI-X to PCIe Bridges are roots of PCIe
> @@ -828,11 +813,33 @@ static struct pcie_link_state
> *alloc_pcie_link_state(struct pci_dev *pdev)
>
> static int pci_aspm_init_downstream(struct pci_dev *pdev)
> {
> + struct pcie_link_state *link;
> + struct pci_dev *parent;
> +
> + /*
> + * The L1 PM substate capability is only implemented in function 0 in a
> + * multi function device.
> + */
> + if (PCI_FUNC(pdev->devfn) != 0)
> + return -EINVAL;
> +
> + parent = pdev->bus->self;
> + if (!parent)
> + return -EINVAL;
> +
> + link = parent->link_state;
> + link->downstream = pdev;
> return 0;
> }
>
> static int pci_aspm_init_upstream(struct pci_dev *pdev)
> {
> + struct pcie_link_state *link;
> +
> + link = alloc_pcie_link_state(pdev);
> + if (!link)
> + return -ENOMEM;
This allocates the link_state when we enumerate a Downstream Port
instead of when we enumerate a child device. We want the link_state
lifetime to match that of the Downstream Port, so this seems good.
But we shouldn't at the same time change the link_state deallocation
so it happens when the Downstream Port is removed, not when the child
device is removed?
> return 0;
> }
>
> @@ -843,6 +850,17 @@ static int pci_aspm_init_upstream(struct pci_dev *pdev)
> */
> int pci_aspm_init(struct pci_dev *pdev)
> {
> + if (!aspm_support_enabled)
> + return 0;
> +
> + if (!pci_is_pcie(pdev))
> + return -EINVAL;
> +
> + /* VIA has a strange chipset, root port is under a bridge */
> + if (pci_pcie_type(pdev) == PCI_EXP_TYPE_ROOT_PORT &&
> + pdev->bus->self)
> + return 0;
> +
> if (!pdev->has_secondary_link)
> return pci_aspm_init_downstream(pdev);
>
> @@ -859,33 +877,16 @@ void pcie_aspm_init_link_state(struct pci_dev *pdev)
> struct pcie_link_state *link;
> int blacklist = !!pcie_aspm_sanity_check(pdev);
>
> - if (!aspm_support_enabled)
> - return;
> -
> - if (pdev->link_state)
> - return;
> -
> - /*
> - * We allocate pcie_link_state for the component on the upstream
> - * end of a Link, so there's nothing to do unless this device has a
> - * Link on its secondary side.
> - */
> - if (!pdev->has_secondary_link)
> - return;
> -
> - /* VIA has a strange chipset, root port is under a bridge */
> - if (pci_pcie_type(pdev) == PCI_EXP_TYPE_ROOT_PORT &&
> - pdev->bus->self)
> + if (!pdev->link_state)
> return;
>
> + link = pdev->link_state;
> down_read(&pci_bus_sem);
> if (list_empty(&pdev->subordinate->devices))
> goto out;
>
> mutex_lock(&aspm_lock);
> - link = alloc_pcie_link_state(pdev);
> - if (!link)
> - goto unlock;
> +
> /*
> * Setup initial ASPM state. Note that we need to configure
> * upstream links also because capable state of them can be
> @@ -910,7 +911,6 @@ void pcie_aspm_init_link_state(struct pci_dev *pdev)
> pcie_set_clkpm(link, policy_to_clkpm_state(link));
> }
>
> -unlock:
> mutex_unlock(&aspm_lock);
> out:
> up_read(&pci_bus_sem);
> --
> 1.9.1
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-ker...@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
