Re: [PATCH v7 3/5] PCI/ASPM: Add and use helper pcie_aspm_get_link

[Bjorn Helgaas <helgaas@kernel.org>]

On Sat, Oct 05, 2019 at 02:07:18PM +0200, Heiner Kallweit wrote:
> Factor out getting the link associated with a pci_dev and use this
> helper where appropriate. In addition this helper will be used
> in a subsequent patch of this series.
> 
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> ---
> v7:
> - add as separate patch
> ---
>  drivers/pci/pcie/aspm.c | 26 +++++++++++++++++---------
>  1 file changed, 17 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
> index 574f822bf..91cfea673 100644
> --- a/drivers/pci/pcie/aspm.c
> +++ b/drivers/pci/pcie/aspm.c
> @@ -1066,19 +1066,28 @@ void pcie_aspm_powersave_config_link(struct pci_dev *pdev)
>  	up_read(&pci_bus_sem);
>  }
>  
> +static struct pcie_link_state *pcie_aspm_get_link(struct pci_dev *pdev)
> +{
> +	struct pci_dev *upstream;
> +
> +	if (pcie_downstream_port(pdev))
> +		upstream = pdev;

Is there a case where this is called for a downstream port?  After
this series is completely applied, the callers I see are:

  __pci_disable_link_state()
  pcie_aspm_enabled()
  aspm_attr_show_common()
  aspm_attr_store_common()
  clkpm_show()
  clkpm_store()
  aspm_ctrl_attrs_are_visible()

I'm pretty sure all of these only care about upstream ports, i.e., we
only call them for endpoints, switch upstream ports, etc.

What do you think about something like this?

  static struct pcie_link_state *pcie_aspm_get_link(struct pci_dev *pdev)
  {
    struct pci_dev *bridge;

    if (pci_is_pcie(pdev)) {
      bridge = pci_upstream_bridge(pdev);
      if (bridge && pci_is_pcie(bridge))
        return bridge->link_state;
    }

    return NULL;
  }

Then we could remove the pci_is_pcie() checks from
__pci_disable_link_state() and aspm_ctrl_attrs_are_visible().

> +	else
> +		upstream = pci_upstream_bridge(pdev);
> +
> +	return upstream ? upstream->link_state : NULL;
> +}
> +
>  static int __pci_disable_link_state(struct pci_dev *pdev, int state, bool sem)
>  {
> -	struct pci_dev *parent = pdev->bus->self;
>  	struct pcie_link_state *link;
>  
>  	if (!pci_is_pcie(pdev))
>  		return 0;
>  
> -	if (pcie_downstream_port(pdev))
> -		parent = pdev;
> -	if (!parent || !parent->link_state)
> +	link = pcie_aspm_get_link(pdev);
> +	if (!link)
>  		return -EINVAL;
> -
>  	/*
>  	 * A driver requested that ASPM be disabled on this device, but
>  	 * if we don't have permission to manage ASPM (e.g., on ACPI
> @@ -1095,7 +1104,6 @@ static int __pci_disable_link_state(struct pci_dev *pdev, int state, bool sem)
>  	if (sem)
>  		down_read(&pci_bus_sem);
>  	mutex_lock(&aspm_lock);
> -	link = parent->link_state;
>  	if (state & PCIE_LINK_STATE_L0S)
>  		link->aspm_disable |= ASPM_STATE_L0S;
>  	if (state & PCIE_LINK_STATE_L1)
> @@ -1188,14 +1196,14 @@ module_param_call(policy, pcie_aspm_set_policy, pcie_aspm_get_policy,
>   */
>  bool pcie_aspm_enabled(struct pci_dev *pdev)
>  {
> -	struct pci_dev *bridge = pci_upstream_bridge(pdev);
> +	struct pcie_link_state *link = pcie_aspm_get_link(pdev);
>  	bool ret;
>  
> -	if (!bridge)
> +	if (!link)
>  		return false;
>  
>  	mutex_lock(&aspm_lock);
> -	ret = bridge->link_state ? !!bridge->link_state->aspm_enabled : false;
> +	ret = link->aspm_enabled;
>  	mutex_unlock(&aspm_lock);

I'm not sure why this mutex is needed; I cc'd you on my query to
Rafael about this.  Unrelated to your patch, of course.

>  	return ret;
> -- 
> 2.23.0
> 
>