[V6,1/3] PCI/ASPM: Add pci_enable_default_link_state()

Message ID	20220301041943.2935892-2-david.e.box@linux.intel.com (mailing list archive)
State	New, archived
Delegated to:	Bjorn Helgaas
Headers	show Return-Path: <linux-pci-owner@kernel.org> From: "David E. Box" <david.e.box@linux.intel.com> To: 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, david.e.box@linux.intel.com, michael.a.bottini@linux.intel.com, rafael@kernel.org, me@adhityamohan.in Cc: linux-pci@vger.kernel.org, linux-kernel@vger.kernel.org Subject: [PATCH V6 1/3] PCI/ASPM: Add pci_enable_default_link_state() Date: Mon, 28 Feb 2022 20:19:41 -0800 Message-Id: <20220301041943.2935892-2-david.e.box@linux.intel.com> In-Reply-To: <20220301041943.2935892-1-david.e.box@linux.intel.com> References: <20220301041943.2935892-1-david.e.box@linux.intel.com> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit Precedence: bulk
Series	PCI: vmd: Enable PCIe ASPM and LTR \| expand [V6,0/3] PCI: vmd: Enable PCIe ASPM and LTR [V6,1/3] PCI/ASPM: Add pci_enable_default_link_state() [V6,2/3] PCI: vmd: Add vmd_device_data [V6,3/3] PCI: vmd: Configure PCIe ASPM and LTR

David E. Box March 1, 2022, 4:19 a.m. UTC

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>
---
 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@intel.com
 

 drivers/pci/pcie/aspm.c | 54 +++++++++++++++++++++++++++++++++++++++++
 include/linux/pci.h     |  7 ++++++
 2 files changed, 61 insertions(+)

Christoph Hellwig March 1, 2022, 8:13 a.m. UTC | #1

On Mon, Feb 28, 2022 at 08:19:41PM -0800, David E. Box wrote:
> +	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));

Is there any reason the ASPM_* values aren't passed directly to this
function?

David E. Box March 1, 2022, 1:31 p.m. UTC | #2

On Tue, 2022-03-01 at 00:13 -0800, Christoph Hellwig wrote:
> On Mon, Feb 28, 2022 at 08:19:41PM -0800, David E. Box wrote:
> > +	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));
> 
> Is there any reason the ASPM_* values aren't passed directly to this
> function?

The ASPM_* macors aren't visible outside of aspm.c whereas the
PCIE_LINK_STATE_* macros are defined in pci.h. This is similar to what
is done for pci_disable_link_state().

David

Christoph Hellwig March 1, 2022, 1:38 p.m. UTC | #3

On Tue, Mar 01, 2022 at 05:31:51AM -0800, David E. Box wrote:
> > Is there any reason the ASPM_* values aren't passed directly to this
> > function?
> 
> The ASPM_* macors aren't visible outside of aspm.c whereas the
> PCIE_LINK_STATE_* macros are defined in pci.h. This is similar to what
> is done for pci_disable_link_state().

Ok.  This looks a little weird but I guess we should stick to the
existing pattern here.

Lorenzo Pieralisi Aug. 26, 2022, 9:16 a.m. UTC | #4

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>
> ---
>  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@intel.com
>  
> 
>  drivers/pci/pcie/aspm.c | 54 +++++++++++++++++++++++++++++++++++++++++
>  include/linux/pci.h     |  7 ++++++
>  2 files changed, 61 insertions(+)

This requires Bjorn's ACK, delegated to him in patchwork.

Lorenzo

> diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
> index a96b7424c9bc..b2752851b1ba 100644
> --- a/drivers/pci/pcie/aspm.c
> +++ b/drivers/pci/pcie/aspm.c
> @@ -1132,6 +1132,60 @@ int pci_disable_link_state(struct pci_dev *pdev, int state)
>  }
>  EXPORT_SYMBOL(pci_disable_link_state);
>  
> +/**
> + * 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.
> + *
> + * @pdev: PCI device
> + * @state: Mask of ASPM link states to enable
> + */
> +int pci_enable_default_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_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
> 
>

Bjorn Helgaas Aug. 26, 2022, 5:01 p.m. UTC | #5

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
>

[V6,1/3] PCI/ASPM: Add pci_enable_default_link_state()

Commit Message

Comments

Patch