Re: [PATCH 1/2] PCI/ASPM: Add ASPM BIOS override function

[Krzysztof Wilczyński]

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