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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <YaXay7MkIybYz4uI@rocinante>
Date:   Tue, 30 Nov 2021 09:03:23 +0100
From:   Krzysztof Wilczyński <kw@...ux.com>
To:     "David E. Box" <david.e.box@...ux.intel.com>
Cc:     nirmal.patel@...ux.intel.com, jonathan.derrick@...ux.dev,
        lorenzo.pieralisi@....com, robh@...nel.org, bhelgaas@...gle.com,
        michael.a.bottini@...ux.intel.com, rafael@...nel.org,
        linux-pci@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/2] PCI/ASPM: Add ASPM BIOS override function

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