Date:   Tue, 30 Nov 2021 09:03:23 +0100
From:   Krzysztof =?utf-8?Q?Wilczy=C5=84ski?= <kw@linux.com>
To:     "David E. Box" <david.e.box@linux.intel.com>
Cc:     nirmal.patel@linux.intel.com, jonathan.derrick@linux.dev,
        lorenzo.pieralisi@arm.com, robh@kernel.org, bhelgaas@google.com,
        michael.a.bottini@linux.intel.com, rafael@kernel.org,
        linux-pci@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH 1/2] PCI/ASPM: Add ASPM BIOS override function
Message-ID: <YaXay7MkIybYz4uI@rocinante>
References: <20211120015756.1396263-1-david.e.box@linux.intel.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <20211120015756.1396263-1-david.e.box@linux.intel.com>
Precedence: bulk

Hi David,

[...]
> @@ -562,11 +562,13 @@ void pcie_aspm_init_link_state(struct pci_dev *pdev);
>  void pcie_aspm_exit_link_state(struct pci_dev *pdev);
>  void pcie_aspm_pm_state_change(struct pci_dev *pdev);
>  void pcie_aspm_powersave_config_link(struct pci_dev *pdev);
> +void pcie_aspm_policy_override(struct pci_dev *dev);
>  #else
>  static inline void pcie_aspm_init_link_state(struct pci_dev *pdev) { }
>  static inline void pcie_aspm_exit_link_state(struct pci_dev *pdev) { }
>  static inline void pcie_aspm_pm_state_change(struct pci_dev *pdev) { }
>  static inline void pcie_aspm_powersave_config_link(struct pci_dev *pdev) { }
> +static inline void pcie_aspm_policy_override(struct pci_dev *dev) {}
>  #endif

A small nitpick, and a slight OCD on my part, so feel free to ignore this,
of course: a missing space between curly brackets, to keep things aligned
with previous definitions.

> @@ -1140,6 +1140,24 @@ int pci_disable_link_state(struct pci_dev *pdev, int state)
>  }
>  EXPORT_SYMBOL(pci_disable_link_state);
>  
> +void pcie_aspm_policy_override(struct pci_dev *pdev)
> +{
> +	struct pcie_link_state *link = pcie_aspm_get_link(pdev);
> +
> +	down_read(&pci_bus_sem);
> +	mutex_lock(&aspm_lock);
> +
> +	if (link) {
> +		link->aspm_default = ASPM_STATE_ALL;
> +		pcie_config_aspm_link(link, policy_to_aspm_state(link));
> +		pcie_set_clkpm(link, policy_to_clkpm_state(link));
> +	}
> +
> +	mutex_unlock(&aspm_lock);
> +	up_read(&pci_bus_sem);
> +}
> +EXPORT_SYMBOL(pcie_aspm_policy_override);

What about the following version where if we have no link (albeit, I am not
sure how often this is going to be the case?) then we don't even attempt to
get a hold on the lock and such:

	void pcie_aspm_policy_override(struct pci_dev *pdev)
	{
		struct pcie_link_state *link = pcie_aspm_get_link(pdev);
	
		if (!link)
			return;
	
		down_read(&pci_bus_sem);
		mutex_lock(&aspm_lock);
	
		link->aspm_default = ASPM_STATE_ALL;
		pcie_config_aspm_link(link, policy_to_aspm_state(link));
		pcie_set_clkpm(link, policy_to_clkpm_state(link));
	
		mutex_unlock(&aspm_lock);
		up_read(&pci_bus_sem);
	}

What do you think?  Would this make sense?

	Krzysztof