| 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
| ||
|
Message-ID: <050bfe55-2b18-6df1-d6ba-14e41ac740fb@linux.intel.com>
Date: Fri, 18 Nov 2022 21:43:09 -0800
From: Sathyanarayanan Kuppuswamy
<sathyanarayanan.kuppuswamy@...ux.intel.com>
To: "David E. Box" <david.e.box@...ux.intel.com>,
nirmal.patel@...ux.intel.com, jonathan.derrick@...ux.dev,
lorenzo.pieralisi@....com, hch@...radead.org, kw@...ux.com,
robh@...nel.org, bhelgaas@...gle.com, michael.a.bottini@...el.com,
rafael@...nel.org, me@...ityamohan.in
Cc: linux-pci@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH V8 RESEND 1/4] PCI/ASPM: Add pci_enable_link_state()
Hi,
On 11/18/22 6:14 PM, David E. Box wrote:
> From: Michael Bottini <michael.a.bottini@...ux.intel.com>
>
> Add pci_enable_link_state() to allow devices to change the default BIOS
> configured states. Clears the BIOS default settings then sets the new
> states and reconfigures the link under the semaphore. Also add
> PCIE_LINK_STATE_ALL macro for convenience for callers that want to enable
> all link states.
>
> Signed-off-by: Michael Bottini <michael.a.bottini@...ux.intel.com>
> Signed-off-by: David E. Box <david.e.box@...ux.intel.com>
> Acked-by: Bjorn Helgaas <bhelgaas@...gle.com>
> ---
> V8
> - No change
>
> V7
> - Fix description as suggested by Bjorn
> - Rename function to pci_enable_link_state
>
> V6
> - No change
> V5
> - Rename to pci_enable_default_link_state and model after
> pci_disable_link_state() as suggested by Bjorn.
> - Add helper PCIE_LINK_STATE_ALL which sets bits for all links states and
> clock pm.
> - Clarify commit language to indicate the function changes the default
> link states (not ASPM policy).
> V4
> - Refactor vmd_enable_apsm() to exit early, making the lines shorter
> and more readable. Suggested by Christoph.
> V3
> - No changes
> V2
> - Use return status to print pci_info message if ASPM cannot be enabled.
> - Add missing static declaration, caught by lkp@...el.com
>
> drivers/pci/pcie/aspm.c | 54 +++++++++++++++++++++++++++++++++++++++++
> include/linux/pci.h | 7 ++++++
> 2 files changed, 61 insertions(+)
>
> diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
> index 53a1fa306e1e..339c686a5094 100644
> --- a/drivers/pci/pcie/aspm.c
> +++ b/drivers/pci/pcie/aspm.c
> @@ -1181,6 +1181,60 @@ int pci_disable_link_state(struct pci_dev *pdev, int state)
> }
> EXPORT_SYMBOL(pci_disable_link_state);
>
> +/**
> + * pci_enable_link_state - Clear and set the default device link state so that
> + * the link may be allowed to enter the specified states. Note that if the
> + * BIOS didn't grant ASPM control to the OS, this does nothing because we can't
> + * touch the LNKCTL register. Also note that this does not enable states
> + * disabled by pci_disable_link_state(). Return 0 or a negative errno.
> + *
> + * @pdev: PCI device
> + * @state: Mask of ASPM link states to enable
> + */
> +int pci_enable_link_state(struct pci_dev *pdev, int state)
> +{
> + struct pcie_link_state *link = pcie_aspm_get_link(pdev);
> +
> + if (!link)
> + return -EINVAL;
> + /*
> + * A driver requested that ASPM be enabled on this device, but
> + * if we don't have permission to manage ASPM (e.g., on ACPI
> + * systems we have to observe the FADT ACPI_FADT_NO_ASPM bit and
> + * the _OSC method), we can't honor that request.
> + */
> + if (aspm_disabled) {
> + pci_warn(pdev, "can't override BIOS ASPM; OS doesn't have ASPM control\n");
> + return -EPERM;
> + }
> +
> + down_read(&pci_bus_sem);
> + mutex_lock(&aspm_lock);
> + link->aspm_default = 0;
> + if (state & PCIE_LINK_STATE_L0S)
> + link->aspm_default |= ASPM_STATE_L0S;
> + if (state & PCIE_LINK_STATE_L1)
> + /* L1 PM substates require L1 */
> + link->aspm_default |= ASPM_STATE_L1 | ASPM_STATE_L1SS;
> + if (state & PCIE_LINK_STATE_L1_1)
> + link->aspm_default |= ASPM_STATE_L1_1;
> + if (state & PCIE_LINK_STATE_L1_2)
> + link->aspm_default |= ASPM_STATE_L1_2;
> + if (state & PCIE_LINK_STATE_L1_1_PCIPM)
> + link->aspm_default |= ASPM_STATE_L1_1_PCIPM;
> + if (state & PCIE_LINK_STATE_L1_2_PCIPM)
> + link->aspm_default |= ASPM_STATE_L1_2_PCIPM;
> + pcie_config_aspm_link(link, policy_to_aspm_state(link));
> +
> + link->clkpm_default = (state & PCIE_LINK_STATE_CLKPM) ? 1 : 0;
> + pcie_set_clkpm(link, policy_to_clkpm_state(link));
> + mutex_unlock(&aspm_lock);
> + up_read(&pci_bus_sem);
> +
> + return 0;
> +}
> +EXPORT_SYMBOL(pci_enable_link_state);
I see that this function and __pci_disable_link_state() are very similar. Can
this be merged? something like __pci_config_link_state(enable or disable)?
> +
> static int pcie_aspm_set_policy(const char *val,
> const struct kernel_param *kp)
> {
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index 2bda4a4e47e8..8c35f15e6012 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -1651,10 +1651,15 @@ extern bool pcie_ports_native;
> #define PCIE_LINK_STATE_L1_2 BIT(4)
> #define PCIE_LINK_STATE_L1_1_PCIPM BIT(5)
> #define PCIE_LINK_STATE_L1_2_PCIPM BIT(6)
> +#define PCIE_LINK_STATE_ALL (PCIE_LINK_STATE_L0S | PCIE_LINK_STATE_L1 |\
> + PCIE_LINK_STATE_CLKPM | PCIE_LINK_STATE_L1_1 |\
> + PCIE_LINK_STATE_L1_2 | PCIE_LINK_STATE_L1_1_PCIPM |\
> + PCIE_LINK_STATE_L1_2_PCIPM)
>
> #ifdef CONFIG_PCIEASPM
> int pci_disable_link_state(struct pci_dev *pdev, int state);
> int pci_disable_link_state_locked(struct pci_dev *pdev, int state);
> +int pci_enable_link_state(struct pci_dev *pdev, int state);
> void pcie_no_aspm(void);
> bool pcie_aspm_support_enabled(void);
> bool pcie_aspm_enabled(struct pci_dev *pdev);
> @@ -1663,6 +1668,8 @@ static inline int pci_disable_link_state(struct pci_dev *pdev, int state)
> { return 0; }
> static inline int pci_disable_link_state_locked(struct pci_dev *pdev, int state)
> { return 0; }
> +static inline int pci_enable_link_state(struct pci_dev *pdev, int state)
> +{ return 0; }
> static inline void pcie_no_aspm(void) { }
> static inline bool pcie_aspm_support_enabled(void) { return false; }
> static inline bool pcie_aspm_enabled(struct pci_dev *pdev) { return false; }
--
Sathyanarayanan Kuppuswamy
Linux Kernel Developer
Powered by blists - more mailing lists