From: Bjorn Helgaas <helgaas@kernel.org>
To: "David E. Box" <david.e.box@linux.intel.com>
Cc: nirmal.patel@linux.intel.com, jonathan.derrick@linux.dev,
lorenzo.pieralisi@arm.com, hch@infradead.org, kw@linux.com,
robh@kernel.org, bhelgaas@google.com,
michael.a.bottini@linux.intel.com, rafael@kernel.org,
me@adhityamohan.in, linux-pci@vger.kernel.org,
linux-kernel@vger.kernel.org
Subject: Re: [PATCH V6 1/3] PCI/ASPM: Add pci_enable_default_link_state()
Date: Fri, 26 Aug 2022 12:01:30 -0500 [thread overview]
Message-ID: <20220826170130.GA2933762@bhelgaas> (raw)
In-Reply-To: <20220301041943.2935892-2-david.e.box@linux.intel.com>
On Mon, Feb 28, 2022 at 08:19:41PM -0800, David E. Box wrote:
> From: Michael Bottini <michael.a.bottini@linux.intel.com>
>
> Add pci_enable_default_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@linux.intel.com>
> Signed-off-by: David E. Box <david.e.box@linux.intel.com>
With minor changes below,
Acked-by: Bjorn Helgaas <bhelgaas@google.com>
> + * pci_enable_default_link_state - Clears and sets 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(). Returns 0 or a negative errno.
"Clear and set" to match imperative mood of similar comments.
Similarly "Return 0 or ...".
> + *
> + * @pdev: PCI device
> + * @state: Mask of ASPM link states to enable
> + */
> +int pci_enable_default_link_state(struct pci_dev *pdev, int state)
I think "pci_enable_link_state()" would be a better name since
"default" isn't relevant to the caller and it would be more parallel
with pci_disable_link_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_default_link_state);
> +
> 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 8253a5413d7c..fd710afe0209 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -1641,10 +1641,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_default_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);
> @@ -1653,6 +1658,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_default_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; }
> --
> 2.25.1
>
next prev parent reply other threads:[~2022-08-26 17:02 UTC|newest]
Thread overview: 13+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-03-01 4:19 [PATCH V6 0/3] PCI: vmd: Enable PCIe ASPM and LTR David E. Box
2022-03-01 4:19 ` [PATCH V6 1/3] PCI/ASPM: Add pci_enable_default_link_state() David E. Box
2022-03-01 8:13 ` Christoph Hellwig
2022-03-01 13:31 ` David E. Box
2022-03-01 13:38 ` Christoph Hellwig
2022-08-26 9:16 ` Lorenzo Pieralisi
2022-08-26 17:01 ` Bjorn Helgaas [this message]
2022-03-01 4:19 ` [PATCH V6 2/3] PCI: vmd: Add vmd_device_data David E. Box
2022-08-26 9:15 ` Lorenzo Pieralisi
2022-03-01 4:19 ` [PATCH V6 3/3] PCI: vmd: Configure PCIe ASPM and LTR David E. Box
2022-08-26 17:01 ` Bjorn Helgaas
2022-09-14 20:59 ` David E. Box
2022-03-01 19:19 ` [PATCH V6 0/3] PCI: vmd: Enable " Jonathan Derrick
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20220826170130.GA2933762@bhelgaas \
--to=helgaas@kernel.org \
--cc=bhelgaas@google.com \
--cc=david.e.box@linux.intel.com \
--cc=hch@infradead.org \
--cc=jonathan.derrick@linux.dev \
--cc=kw@linux.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-pci@vger.kernel.org \
--cc=lorenzo.pieralisi@arm.com \
--cc=me@adhityamohan.in \
--cc=michael.a.bottini@linux.intel.com \
--cc=nirmal.patel@linux.intel.com \
--cc=rafael@kernel.org \
--cc=robh@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).